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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Monday, January 13 2014, Pedro Alves wrote:

> On 01/10/2014 08:41 PM, Sergio Durigan Junior wrote:

>> 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.

OK.

>>> 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.

OK, makes sense.

> 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.

Yes, confirming that it passes with the patch.  Sorry for taking long to
test, I had issues trying to get another s390x machine to test it.

Thanks for the patch and the further investigation.

> ---
>  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

-- 
Sergio