From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 7A7B7385DC00 for ; Mon, 8 Jun 2020 16:01:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 7A7B7385DC00 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark@simark.ca Received: from [172.16.0.95] (192-222-181-218.qc.cable.ebox.net [192.222.181.218]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id AE9401E791; Mon, 8 Jun 2020 12:01:33 -0400 (EDT) Subject: Re: [PATCH v2] [gdb] Support frames inlined into the outer frame To: Luis Machado , Scott Linder , gdb-patches@sourceware.org References: <41baeec4e477b1287e331e58adb9abf4@scottlinder.com> <20200331191856.31222-1-scott@scottlinder.com> <9e2832f3-c3c4-76da-9c40-85b8055bdee5@simark.ca> <6273d9f5-5c42-620b-daff-c332ab148623@simark.ca> From: Simon Marchi Message-ID: Date: Mon, 8 Jun 2020 12:01:33 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/mixed; boundary="------------E02068E6FD71ABE9D1EFD739" Content-Language: tl X-Spam-Status: No, score=-13.8 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, 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: Mon, 08 Jun 2020 16:01:36 -0000 This is a multi-part message in MIME format. --------------E02068E6FD71ABE9D1EFD739 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit On 2020-06-08 8:00 a.m., Luis Machado wrote: > > I don't see the same, even with the fixup of memcmp. Though gdb.base/break.exp has full passes with the change, the following tests internal error with the patch... > > gdb.mi/mi-nonstop.exp > gdb.threads/clone-thread_db.exp > gdb.threads/current-lwp-dead.exp > gdb.threads/hand-call-in-threads.exp > gdb.threads/linux-dp.exp > gdb.threads/local-watch-wrong-thread.exp > gdb.threads/queue-signal.exp > gdb.threads/schedlock.exp > gdb.threads/thread_check.exp > gdb.threads/tls.exp > > #1  0x0000ffffb7fa1088 in start_thread () from /lib/aarch64-linux-gnu/libpthread.so.0 > ../../../repos/binutils-gdb/gdb/frame.c:551: internal-error: void compute_frame_id(frame_info*): Assertion `frame_id_p (fi->this_id.value)' failed. > > Scott, could you please send a v3 so I can make sure I tested the right version? I was initially slightly confused with what version Simon was talking about since I had already tested v2. I don't see these tests failing. Can you please share your test setup? Maybe we'll find what's different between us. I've attached the patch I am testing. It is just patch v2 with `== 0` added after the memcmp call. I am testing on top of commit 7d8b91fda9fed423b91d4d43b19dd068457fe555. I am testing on machine gcc117 of the compile farm, which is running Debian 9.12 (stretch). The gcc version there is 6.3.0. Simon --------------E02068E6FD71ABE9D1EFD739 Content-Type: text/x-patch; charset=UTF-8; name="0001-Support-frames-inlined-into-the-outer-frame.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0001-Support-frames-inlined-into-the-outer-frame.patch" >From 26297062a61b3dd7bd1a3e55005aaa932713c7da Mon Sep 17 00:00:00 2001 From: Scott Linder Date: Tue, 31 Mar 2020 15:18:56 -0400 Subject: [PATCH] Support frames inlined into the outer frame 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. Change-Id: I8aa129c667dccc31590ffdf426586418493a6ebe --- 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 ff27b9f00e..d03d6faed3 100644 --- a/gdb/frame.c +++ b/gdb/frame.c @@ -700,11 +700,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="); @@ -728,14 +724,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. @@ -771,6 +768,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)) == 0; + 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 e835d49f9c..5e6690b2a1 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. */ @@ -254,6 +260,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.11.0 --------------E02068E6FD71ABE9D1EFD739--