[PATCH v2] [gdb] Support frames inlined into the outer frame

[Andrew Burgess]

* Scott Linder <scott@scottlinder.com> [2020-03-31 15:18:56 -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.
> 
> 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.
> 	(outer_frame_id_p): New.
> 	* frame.h (outer_frame_id): Update comment.
> 	(outer_frame_id_p): New.
> 	* inline-frame.c (inline_frame_this_id): Remove assert that prevents
> 	inline frame ids in outer frame.

Thanks, this looks much great.  I have a couple of tiny suggestions,
described inline.

Thanks,
Andrew


> 
> Change-Id: I8aa129c667dccc31590ffdf426586418493a6ebe
> ---
> Changes since v1:
> * Fix ChangeLog formatting.
> * Add outer_frame_id_p to capture new definition of outer_frame_id in
>   one place and to restore checks for all members.
> * Reword some comments to make them more precise, borrowing a lot of
>   wording from Andrew Burgess.
> * Remove some comments describing what is now obvious.
> * Undo update to frame_id_p comment which exposes implementation details.
> 
>  gdb/frame.c        | 41 ++++++++++++++++++++++++++++-------------
>  gdb/frame.h        | 12 +++++++++++-
>  gdb/inline-frame.c |  4 ----
>  3 files changed, 39 insertions(+), 18 deletions(-)
> 
> diff --git a/gdb/frame.c b/gdb/frame.c
> index d74d1d5c7c..c6154c2d9c 100644
> --- a/gdb/frame.c
> +++ b/gdb/frame.c
> @@ -692,11 +692,7 @@ frame_id_p (struct frame_id l)
>  {
>    int p;
>  
> -  /* 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)
> -    p = 1;
> +  p = l.stack_status != FID_STACK_INVALID || outer_frame_id_p (l);
>    if (frame_debug)
>      {
>        fprintf_unfiltered (gdb_stdlog, "{ frame_id_p (l=");
> @@ -720,14 +716,15 @@ frame_id_eq (struct frame_id l, struct frame_id r)
>  {
>    int eq;
>  
> -  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
> -       we might step into another function - from which we can't
> -       unwind either.  More thought required to get rid of
> -       outer_frame_id.  */
> -    eq = 1;
> +  if (outer_frame_id_p (l) && outer_frame_id_p (r))
> +    /* The outermost frame marker, and any inline 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.  */

In a previous email, about this comment you wrote:

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

Yes, I agree with you, and I hadn't considered this case.  The problem
with outer_frame_id before was that if you stepped into a different
function that was also outer_frame_id then you couldn't tell.  After
your patch if you step into another function that is outer_frame_id
*and* the artificial_depth is the same, then you can't tell.

Do feel free to rewrite the above as you see fit.  I agree that it's a
pretty unlikely case, but if we're going to document a known
limitation we might as well try to be accurate.

> +    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.
> @@ -763,6 +760,24 @@ frame_id_eq (struct frame_id l, struct frame_id r)
>    return eq;
>  }
>  
> +int
> +outer_frame_id_p (struct frame_id l)
> +{
> +  int p;
> +
> +  /* The artificial_depth can vary so we ignore it when checking if this is
> +     an outer_frame_id.  */
> +  l.artificial_depth = 0;
> +  p = memcmp (&l, &outer_frame_id, sizeof (outer_frame_id));

This function should can be static within this file, and should return
a bool (and p should change type to match).

> +  if (frame_debug)
> +    {
> +      fprintf_unfiltered (gdb_stdlog, "{ outer_frame_id_p (l=");
> +      fprint_frame_id (gdb_stdlog, l);
> +      fprintf_unfiltered (gdb_stdlog, ") -> %d }\n", p);
> +    }
> +  return p;
> +}
> +
>  /* Safety net to check whether frame ID L should be inner to
>     frame ID R, according to their stack addresses.
>  
> diff --git a/gdb/frame.h b/gdb/frame.h
> index cfc15022ed..66f19c91dc 100644
> --- a/gdb/frame.h
> +++ b/gdb/frame.h
> @@ -195,7 +195,13 @@ 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 has stack_status set to FID_STACK_INVALID,
> +   special_addr_p set to 1, artificial_depth set to 0 or greater, and all other
> +   members set to 0. For the non-inline outer frame artificial_depth remains
> +   set to 0 and for frames inlined into it the artificial_depth is set in the
> +   typical way.  Checking if a frame marker is an outer_frame_id should be done
> +   with outer_frame_id_p.  */
>  extern const struct frame_id outer_frame_id;
>  
>  /* Flag to control debugging.  */
> @@ -250,6 +256,10 @@ extern int frame_id_artificial_p (struct frame_id l);
>     either L or R have a zero .func, then the same frame base.  */
>  extern int frame_id_eq (struct frame_id l, struct frame_id r);
>  
> +/* Returns non-zero when L is an outer frame marker or any inline frame marker
> +   derived from it.  */
> +extern int outer_frame_id_p (struct frame_id l);
> +
>  /* Write the internal representation of a frame ID on the specified
>     stream.  */
>  extern void fprint_frame_id (struct ui_file *file, struct frame_id id);
> 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
>