This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix gdb.trace/mi-traceframe-changed.exp to check for target trace support


On 01/10/2014 08:41 PM, Sergio Durigan Junior wrote:
> On Friday, January 10 2014, Pedro Alves wrote:
> 
>> Hmm, OK, I think I see.  I don't think the register block
>> size in the header needs to be correct for this test -- tfile.c
>> portability sounds like a red herring to me.  (I don't think 500
>> is correct for x86/x86_64 either?)  There's no register block
>> in the trace file anyway.  Only memory blocks.
>>
>> tfile knows to infer the PC from the tracepoint's address if
>> the PC wasn't collected (tfile_fetch_registers) but,
>> (guessing) the issue is that PC register in s390 is a
>> pseudo-register.  tfile_fetch_registers guesses the PC from the
>> tracepoint's address and stores it in the regcache, but
>> when reading a (non-readonly) pseudo register from a regcache,
>> we never use the stored value in the cache -- we always
>> recompute it.  In fact, writing the pseudo PC to the regcache
>> in tfile_fetch_registers with regcache_raw_supply seems reachable
>> when regnum==-1, and I'm surprised this isn't triggering the assertion
>> that makes sure the regnum being written to is a raw register
>> (as the regcache's buffer only holds the raw registers -- see
>> regcache_xmalloc_1 and regcache_raw_write).
> 
> Thanks for the investigation.
> 
> By debugging it a little more, I see that we don't write the pseudo PC
> to the regcache.  You are probably talking about this loop:
> 
>   /* We get here if no register data has been found.  Mark registers
>      as unavailable.  */
>   for (regn = 0; regn < gdbarch_num_regs (gdbarch); regn++)
>     regcache_raw_supply (regcache, regn, NULL);

No.  Below that, when regno==-1.  Note my patch adds an early
return _after_ that loop.

  /* We get here if no register data has been found.  Mark registers
     as unavailable.  */
  for (regn = 0; regn < gdbarch_num_regs (gdbarch); regn++)
    regcache_raw_supply (regcache, regn, NULL);

  /* We can often usefully guess that the PC is going to be the same
     as the address of the tracepoint.  */
  pc_regno = gdbarch_pc_regnum (gdbarch);
  if (pc_regno >= 0 && (regno == -1 || regno == pc_regno))
                        ^^^^^^^^^^^
    {
      struct tracepoint *tp = get_tracepoint (tracepoint_number);

      if (tp && tp->base.loc)
	{
	  /* But don't try to guess if tracepoint is multi-location...  */
	  if (tp->base.loc->next)
	    {
	      warning (_("Tracepoint %d has multiple "
			 "locations, cannot infer $pc"),
		       tp->base.number);
	      return;
	    }
	  /* ... or does while-stepping.  */
	  if (tp->step_count > 0)
	    {
	      warning (_("Tracepoint %d does while-stepping, "
			 "cannot infer $pc"),
		       tp->base.number);
	      return;
	    }

	  store_unsigned_integer (regs, register_size (gdbarch, pc_regno),
				  gdbarch_byte_order (gdbarch),
				  tp->base.loc->address);
	  regcache_raw_supply (regcache, pc_regno, regs);
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


	}
    }
}


regno==-1 means "retrieve all raw registers".  It just
seems like no such call happens to trigger in this case.

> The pseudo PC register number on s390x is 90, and "gdbarch_num_regs
> (gdbarch)" returns exactly 90.
> 
> What happens is that when one requests the pseudo PC on s390x, it reads
> S390_PSWM_REGNUM, which is 1:

...

> Because regno == 1 and pc_regno == 90.  Maybe you knew all that, but I
> thought it would be better to explicitly mention.

Right.

>> I.e., this is not a bug specific to s390, but to the
>> tracing/unavailable machinery itself, for all targets
>> whose PC is a pseudo register.
> 
> I see.  I haven't had the chance to test on other targets where PC is a
> pseud register.
> 
>> It's fine with me to skip the test on targets with pseudo
>> register PCs for now, though probably we should record this
>> info somewhere, probably in the code, like in the patch below.
> 
> Nice, I like the comments, though your patch doesn't really change
> anything for s390x because of what I explained above (regno == 1 and
> pc_regno == 90), but I like to make things more explicit and your patch
> does that.
> 
> I can push both our patches if you wish, btw.

I've pushed mine in now.  I'd really prefer to avoid
hardcoding knowledge of which targets support tracing
in the testsuite.  Exposing the host-side bits to
all targets helps identify design bugs, just like
this very case of pseudo-register PCs.

But even if GDB doesn't know how to infer the value
of PC, saying the current frame is level -1 is a bug:

  ^done,found="1",tracepoint="1",traceframe="0",frame={level="-1",addr="<unavailable>",func="??",args=[]}^M
                                                       ^^^^^^^^^

that's of course, the sentinel frame peeking through.
This is caused by the s390's heuristic unwinder accepting the
frame (the fallback heuristic unwinders _always_ accept the
frame), but then unwind->this_id method throws that
"PC not available\n" error.
IOW, the issue is in the target's unwinding mechanism not
having been adjusted to handle unavailable register
values gracefully (which can happen with e.g., a trimmed
core file too).  Could you please try the patch below?
You should see something like:

(gdb) tfind
Found trace frame 0, tracepoint 1
#0  <unavailable> in ?? ()

That is, frame #0 instead of -1.

This is just the minimal necessary for
<unavailable> frames.  We could get better info out
of "info frame" (this will show "outermost"), but
this would still be necessary.  I believe mi-traceframe-changed.exp
should pass with this patch.

---
 gdb/s390-linux-tdep.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
index b75c86a..8b71e78 100644
--- a/gdb/s390-linux-tdep.c
+++ b/gdb/s390-linux-tdep.c
@@ -2039,7 +2039,9 @@ static struct s390_unwind_cache *
 s390_frame_unwind_cache (struct frame_info *this_frame,
 			 void **this_prologue_cache)
 {
+  volatile struct gdb_exception ex;
   struct s390_unwind_cache *info;
+
   if (*this_prologue_cache)
     return *this_prologue_cache;

@@ -2050,10 +2052,15 @@ s390_frame_unwind_cache (struct frame_info *this_frame,
   info->frame_base = -1;
   info->local_base = -1;

-  /* Try to use prologue analysis to fill the unwind cache.
-     If this fails, fall back to reading the stack backchain.  */
-  if (!s390_prologue_frame_unwind_cache (this_frame, info))
-    s390_backchain_frame_unwind_cache (this_frame, info);
+  TRY_CATCH (ex, RETURN_MASK_ERROR)
+    {
+      /* Try to use prologue analysis to fill the unwind cache.
+	 If this fails, fall back to reading the stack backchain.  */
+      if (!s390_prologue_frame_unwind_cache (this_frame, info))
+	s390_backchain_frame_unwind_cache (this_frame, info);
+    }
+  if (ex.reason < 0 && ex.error != NOT_AVAILABLE_ERROR)
+    throw_exception (ex);

   return info;
 }
-- 
1.7.11.7


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]