From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk1-x744.google.com (mail-qk1-x744.google.com [IPv6:2607:f8b0:4864:20::744]) by sourceware.org (Postfix) with ESMTPS id 84232388A02A for ; Fri, 3 Apr 2020 19:37:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 84232388A02A Received: by mail-qk1-x744.google.com with SMTP id y3so4523635qky.8 for ; Fri, 03 Apr 2020 12:37:15 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=xUVY9+vwg7FgCmPDKAC9gGnSc8aUOfyJkc+k/MHmTNo=; b=q3gFcz2UkXFxortA1S0Z+BSLs5W2TBeNSrscpP72j6JZFjDZP1Uz2sZsB+QC1vL8Nk rl2FDnRQRhog6haZE2SakXqDX1cb/NgV2swCzao/HwM0kocub4TfvvhfSbr/NuIVyuMD ieCnDpJgfq0JkHYKjMnk1cwcG24Rt+PYMyaI/Uk+95kxwm/nvE2Gl/BSveIbpp/sKlWF javKPU9Yxjk7z0+r3blPCRGbtWJCLExWoePszr400++tLJczMqNZ2WYKL4kH1r+iIkRi gbEfBY1tbJVhIH8iyRYemEskdMS5i6gpaPpTCgCVRoY1m/mYQ1pXyAYl1I0em0mJ3U5u fPcg== X-Gm-Message-State: AGi0PuaK9iqH3stfq4e72+b+0OniDdD5xWKuAUPRjvJqRleLBswwwvUp Wtw2n4l1qtRfXjCnKr2X91feGRNL34k= X-Google-Smtp-Source: APiQypITttC3dD2GxaczpRBAgRFpTYUdJZb21hc52KhTq6YUYmEDx9yal+3mrCnZIzoe3fibQym7CQ== X-Received: by 2002:a37:4b97:: with SMTP id y145mr10900911qka.167.1585942634610; Fri, 03 Apr 2020 12:37:14 -0700 (PDT) Received: from [192.168.0.181] ([179.185.146.158]) by smtp.gmail.com with ESMTPSA id w204sm7254808qkb.133.2020.04.03.12.37.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 03 Apr 2020 12:37:13 -0700 (PDT) Subject: Re: [PATCH v2] [gdb] Support frames inlined into the outer frame To: Scott Linder , gdb-patches@sourceware.org References: <41baeec4e477b1287e331e58adb9abf4@scottlinder.com> <20200331191856.31222-1-scott@scottlinder.com> From: Luis Machado Message-ID: <6b20b512-e9d7-79ec-7dd3-513ec3e2003d@linaro.org> Date: Fri, 3 Apr 2020 16:37:10 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: <20200331191856.31222-1-scott@scottlinder.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-26.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, 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: Fri, 03 Apr 2020 19:37:17 -0000 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 > > * 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 >