Re: [PATCH] [gdb] Support frames inlined into the outer frame

[scott@scottlinder.com]

Andrew,

Thank you for the review! I'm sorry for taking so long to respond.

I also apologize if my email client munges anything; I had intended to 
set up and learn how to use Mutt or something that would play nicer with 
lists, but I haven't had the chance yet.

On 2020-03-24 06:22, Andrew Burgess wrote:
> Scott,
> 
> Thanks for looking into this.
> 
> * Scott Linder <scott@scottlinder.com> [2020-03-18 18:11:19 -0400]:
> 
>> Broaden the definition of `outer_frame_id` to effectively create a new
>> class of "invalid" IDs to represent frames inlined into the outer 
>> frame.
>> These new IDs behave like the outer frame, in that they are "invalid",
>> yet return true from `frame_id_p` and compare equal to themselves.
> 
> I'm curious as to which target you're working on seeing this issue.
> 

I tried to give a little more exposition in the first email in the 
thread, but in short we encounter this with AMD GPU code objects.  The 
entry function really is at the bottom of the device stack, there is no 
equivalent to e.g. a crt _start.

>> 
>> 2020-03-18  Scott Linder  <scott@scottlinder.com>
>> 
>> 	* frame.c (frame_id_p): Consider functions inlined into outer frame
>> 	as valid.
>> 	(frame_id_eq): Consider functions inlined into outer frame with same
>> 	artificial_depth as equal.
>> 
>> 	* frame.h (outer_frame_id): Update comment.
>> 	(frame_id_p): Update comment.
>> 
>> 	* inline-frame.c (inline_frame_this_id): Remove assert that prevents
>> 	inline frame ids in outer frame.
> 
> ChangeLog entries don't have blank lines between entries.
> 

I will fix this, I had misunderstood what the pages the wiki referenced 
were saying.  Thank you for bearing with me!

>> 
>> Change-Id: I8aa129c667dccc31590ffdf426586418493a6ebe
>> ---
>>  gdb/frame.c        | 11 ++++++-----
>>  gdb/frame.h        |  7 ++++---
>>  gdb/inline-frame.c |  4 ----
>>  3 files changed, 10 insertions(+), 12 deletions(-)
>> 
>> diff --git a/gdb/frame.c b/gdb/frame.c
>> index d74d1d5c7c..b62d68f12a 100644
>> --- a/gdb/frame.c
>> +++ b/gdb/frame.c
>> @@ -694,8 +694,8 @@ frame_id_p (struct frame_id l)
>> 
>>    /* The frame is valid iff it has a valid stack address.  */
>>    p = l.stack_status != FID_STACK_INVALID;
>> -  /* outer_frame_id is also valid.  */
>> -  if (!p && memcmp (&l, &outer_frame_id, sizeof (l)) == 0)
>> +  /* outer_frame_id and functions inlined into it are also valid.  */
>> +  if (!p && l.special_addr_p)
> 
> It's not immediately obvious how the comment relates to the code
> below.  I wonder if changing the code to:
> 
>   if (!p && is_outer_frame_p (l))
> 
> would be better, where is_outer_frame_p is a new function.  My
> motivation here is that we previously checked all fields of l, not
> just the special_addr_p field - I wonder if we should restore more of
> that checking?  Specifically, the value in the special_addr field?  My
> understanding is that if (!p) then (special_addr == 0) - so maybe we
> can assert that.
> 

I agree that a new is_outer_frame_p makes sense, although at that point 
I question whether the comment is really useful.  I also don't think the 
comment above is very helpful, and even before the patch the use of 
`iff` was a lie because of the exception for outer frames.  Would it 
make sense to just delete these and let the comment for frame_id_p in 
frame.h guide the reader, especially considering how short the body of 
the function is?

>>      p = 1;
>>    if (frame_debug)
>>      {
>> @@ -722,12 +722,13 @@ frame_id_eq (struct frame_id l, struct frame_id 
>> r)
>> 
>>    if (l.stack_status == FID_STACK_INVALID && l.special_addr_p
>>        && r.stack_status == FID_STACK_INVALID && r.special_addr_p)
>> -    /* The outermost frame marker is equal to itself.  This is the
>> -       dodgy thing about outer_frame_id, since between execution 
>> steps
>> +    /* The outermost frame marker, and any inline frame markers
>> +       derived from it, are equal to themselves.  This is the dodgy
>> +       thing about outer_frame_id, since between execution steps
>>         we might step into another function - from which we can't
>>         unwind either.  More thought required to get rid of
>>         outer_frame_id.  */
> 
> I think we need to rewrite more of this comment.  The part that says
> "...since between execution steps we might step into another
> function..." now seems ambiguous.  The point this is trying to make is
> that if we step into a totally different function that also identifies
> as outer_frame_id then we can't tell the difference between the
> original function and the new one.
> 
> The problem is if we step into an inline function then we can (now)
> tell them apart.  Maybe something like:
> 
>   The outermost frame marker, and any inilne frame markers derived
>   from it (with artificial_depth > 0), are equal to themselves.  The
>   problem with outer_frame_id is that, if between execution steps, we
>   step into a completely separate function (not an inlined function)
>   that also identifies as outer_frame_id, then we can't distinguish
>   between the previous frame and the new frame.  More thought is
>   required to get rid of outer_frame_id.
> 

Isn't it still the case that we can get confused if we step into another 
function that is outer_frame_id *and* we end up in a different inline 
frame of the same depth?  Or is your point that we will always stop in 
the non-inlined frame first, so we can't ever hit this?  I don't know 
that I understand how one could construct any of these cases, though; 
how could you step from a function that is the "outer frame" into 
another function that is also the "outer frame"?

>> -    eq = 1;
>> +    eq = l.artificial_depth == r.artificial_depth;
>>    else if (l.stack_status == FID_STACK_INVALID
>>  	   || r.stack_status == FID_STACK_INVALID)
>>      /* Like a NaN, if either ID is invalid, the result is false.
>> diff --git a/gdb/frame.h b/gdb/frame.h
>> index cfc15022ed..d394382903 100644
>> --- a/gdb/frame.h
>> +++ b/gdb/frame.h
>> @@ -195,7 +195,8 @@ extern const struct frame_id sentinel_frame_id;
>> 
>>  /* This means "there is no frame ID, but there is a frame".  It 
>> should be
>>     replaced by best-effort frame IDs for the outermost frame, 
>> somehow.
>> -   The implementation is only special_addr_p set.  */
>> +   The implementation is only special_addr_p, and possibly
>> +   artificial_depth, set.  */
>>  extern const struct frame_id outer_frame_id;
> 
> I'd drop the "possibly" from this comment, as the value is always
> valid, right? 0 means no inline frames, and is just as set as a value
> of 1 or more.  Saying possible risks that someone might think they
> need to figure out if artificial_depth is set or not, when this is not
> the case.
> 
>   The implementation has special_addr set to 0, special_addr_p set
>   to 1, and artificial_depth set to 0 or greater.
> 

That makes sense to me, I'll update this to be more precise.  I think 
the original comment here is similarly incorrect, depending on how you 
define "set", because all the fields of the outer_frame_id must have 
been required to be set for the use of memcmp elsewhere to work.

>> 
>>  /* Flag to control debugging.  */
>> @@ -237,8 +238,8 @@ extern struct frame_id
>>  extern struct frame_id frame_id_build_wild (CORE_ADDR stack_addr);
>> 
>>  /* Returns non-zero when L is a valid frame (a valid frame has a
>> -   non-zero .base).  The outermost frame is valid even without an
>> -   ID.  */
>> +   non-zero .base).  The outermost frame and any frames inlined into 
>> it
>> +   are valid even without an ID.  */
>>  extern int frame_id_p (struct frame_id l);
> 
> I personally wouldn't make this change.  You're broadening the
> definition of outer_frame_id, so the original comment is still valid,
> anything else is detail.
> 

I changed this because in my understanding the outer_frame_id 
implementation detail is distinct from the term "outermost frame".  The 
patch broadens the definition of outer_frame_id to represent a class of 
IDs, but to me the phrasing "outermost frame" still refers only to the 
non-artificial frame that is actually at the bottom of the call stack.  
Maybe I could just change this to say "outer_frame_id is valid even 
without an ID"?

>> 
>>  /* Returns non-zero when L is a valid frame representing a frame made 
>> up by GDB
>> diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c
>> index c650195e57..a187630840 100644
>> --- a/gdb/inline-frame.c
>> +++ b/gdb/inline-frame.c
>> @@ -171,10 +171,6 @@ inline_frame_this_id (struct frame_info 
>> *this_frame,
>>       frame").  This will take work.  */
>>    gdb_assert (frame_id_p (*this_id));
>> 
>> -  /* For now, require we don't match outer_frame_id either (see
>> -     comment above).  */
>> -  gdb_assert (!frame_id_eq (*this_id, outer_frame_id));
>> -
>>    /* Future work NOTE: Alexandre Oliva applied a patch to GCC 4.3
>>       which generates DW_AT_entry_pc for inlined functions when
>>       possible.  If this attribute is available, we should use it
>> --
>> 2.17.1
>> 
> 
> It would be great if we could come up for some tests for this new
> code, but I can't immediately see how you might setup something like
> this on any of the targets I'm most familiar with.
> 

I can try to come up with something, but I'm also not familiar with any 
of the existing targets supported by GDB well enough to know off-hand 
how to construct a test.

> Also do you have a copyright assignment in place as I think one would
> be need to take this patch.
> 

AMD owns the copyright; would you be able to check if AMD has an 
appropriate copyright assignment on file, and if not let me know so I 
can make sure we get one filed?

> Thanks,
> Andrew

Thanks,
Scott