From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net [217.70.183.201]) by sourceware.org (Postfix) with ESMTPS id 7A727385B835 for ; Fri, 17 Apr 2020 20:41:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 7A727385B835 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=scottlinder.com Authentication-Results: sourceware.org; spf=none smtp.mailfrom=scott@scottlinder.com X-Originating-IP: 165.204.55.211 Received: from SVL-L10-SLINDER1.amd.com (unknown [165.204.55.211]) (Authenticated sender: scott@scottlinder.com) by relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 8EC011BF204; Fri, 17 Apr 2020 20:41:36 +0000 (UTC) Date: Fri, 17 Apr 2020 16:41:33 -0400 From: Scott Linder To: Andrew Burgess Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v2] [gdb] Support frames inlined into the outer frame Message-ID: <20200417204132.GC10358@SVL-L10-SLINDER1.amd.com> References: <41baeec4e477b1287e331e58adb9abf4@scottlinder.com> <20200331191856.31222-1-scott@scottlinder.com> <20200403170031.GB103248@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200403170031.GB103248@embecosm.com> User-Agent: Mutt/1.9.4 (2018-02-28) X-Spam-Status: No, score=-26.0 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 17 Apr 2020 20:41:40 -0000 On Fri, Apr 03, 2020 at 06:00:31PM +0100, Andrew Burgess wrote: > * Scott Linder [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 > > > > * 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 > >