[PATCH v2] [gdb] Support frames inlined into the outer frame
Scott Linder
scott@scottlinder.com
Fri Apr 17 20:41:33 GMT 2020
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
> >
More information about the Gdb-patches
mailing list