[v2] Support frames inlined into the outer frame

Message ID 20200331191856.31222-1-scott@scottlinder.com
State New
Headers show
Series
  • [v2] Support frames inlined into the outer frame
Related show

Commit Message

Scott Linder March 31, 2020, 7:18 p.m.
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.

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

-- 
2.17.1

Comments

Andrew Burgess April 3, 2020, 5 p.m. | #1
* 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

>
Victor Collod via Gdb-patches April 3, 2020, 7:37 p.m. | #2
Hi Scott,

I was giving this a try on aarch64-linux to see if it fixed an existing 
problem handling inline frames, but it seems to completely break GDB for 
this architecture.

The testsuite runs into a number of internal errors of this kind:

gdb/frame.c:1841: internal-error: frame_info* 
get_next_frame_sentinel_okay(frame_info*): Assertion `this_frame != 
sentinel_frame' failed.

Even basic tests like gdb.base/break.exp run into this.

I'd recommend running this through the buildbot/tryserver to catch 
regressions. This is a very delicate area that is known to break things.

On 3/31/20 4:18 PM, Scott Linder wrote:
> 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.

> 

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

> +    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));

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

>
Scott Linder April 17, 2020, 8:41 p.m. | #3
On Fri, Apr 03, 2020 at 06:00:31PM +0100, Andrew Burgess wrote:
> * 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.

> 

Thank you for the clarification, I will try to document all the
possibilities without being too verbose.
> > +    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).

> 

Ok, will do.
> > +  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

> >
Scott Linder April 17, 2020, 8:51 p.m. | #4
On Fri, Apr 03, 2020 at 04:37:10PM -0300, Luis Machado wrote:
> Hi Scott,

> 

> I was giving this a try on aarch64-linux to see if it fixed an existing

> problem handling inline frames, but it seems to completely break GDB for

> this architecture.

> 

> The testsuite runs into a number of internal errors of this kind:

> 

> gdb/frame.c:1841: internal-error: frame_info*

> get_next_frame_sentinel_okay(frame_info*): Assertion `this_frame !=

> sentinel_frame' failed.

> 

> Even basic tests like gdb.base/break.exp run into this.

> 

> I'd recommend running this through the buildbot/tryserver to catch

> regressions. This is a very delicate area that is known to break things.

> 


Hi Luis,

Thank you for testing this and giving me some more hints :)

It seems like I will have to get commit access before I can request
buildbot access, so I will start going about that and hopefully be able
to address this in the patch soon.

Thanks,
Scott
Simon Marchi June 4, 2020, 4:11 p.m. | #5
On 2020-03-31 3:18 p.m., Scott Linder wrote:
> @@ -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 should be `memcmp (...) == 0`.  Currently, the function returns true when
the frame is not an outer frame id, which is the opposite of what it is supposed
to do.

With this, the test gdb.base/break.exp on AArch64 runs fine.  I will launch a full
test run to see if there are any other problems.

You can make the new function return "bool" instead of "int", and use true/false instead
of zero/non-zero (both in the code and comments).

Simon
Simon Marchi June 4, 2020, 7:23 p.m. | #6
On 2020-06-04 12:11 p.m., Simon Marchi wrote:
> On 2020-03-31 3:18 p.m., Scott Linder wrote:

>> @@ -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 should be `memcmp (...) == 0`.  Currently, the function returns true when

> the frame is not an outer frame id, which is the opposite of what it is supposed

> to do.

> 

> With this, the test gdb.base/break.exp on AArch64 runs fine.  I will launch a full

> test run to see if there are any other problems.

> 

> You can make the new function return "bool" instead of "int", and use true/false instead

> of zero/non-zero (both in the code and comments).

> 

> Simon


With this change, the full test run on AArch64 came out clean.  I'll try a try
job on the buildbot, but I haven't had much success with it recently.

Simon

Patch

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.  */
+    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));
+  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