From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x331.google.com (mail-wm1-x331.google.com [IPv6:2a00:1450:4864:20::331]) by sourceware.org (Postfix) with ESMTPS id 064FC385B834 for ; Tue, 24 Mar 2020 10:22:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 064FC385B834 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wm1-x331.google.com with SMTP id a81so2704649wmf.5 for ; Tue, 24 Mar 2020 03:22:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=O7e3BaxoDFC7mHWucuAnrpSwUK/uCEu0Z05o7t5VVsk=; b=g30FL11u0/j1KCA8QnOBfD7tJdMpQUMsx5n/aGXV51MBzhRWOg5EhB+N+lC5fukPzf W5KYEe8vHMVc5ID5FwZoCIdJ9xc5wyqVXHazmsusMUJ0+abHmd6JCmbCIPVYYrSBcPoq tbquv/6CdZKd7EGPPIDqz6iiw+tKSM0ddc8CtPZxay3jSrKYGBs0o85sk9EKnhO+Aa79 vUXfhD6HI28hDl305ay59fOoY7nJnhCDBuWjXb5OWMU+XpMOIFSk+/746xGQ2+CD+cs5 APc1EHi0hhWpVrbPKdBWMXko/MD64rYJ+n9eboj5ytGkbhvgkmrXGT6NM2D0CiU71XgL NG/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=O7e3BaxoDFC7mHWucuAnrpSwUK/uCEu0Z05o7t5VVsk=; b=K+KeZJ2d/uboKd23r3HZBZifD5JztcbkR+38DVnY1LDjwkzAFsZRsV1JrDxz35cbW8 7k4hepnRCvhTweWHjV/WdE6I/8olFaBPzZR5dL+xQRDffQ5NTh17t2GQSpt/qIxUnFmq ylhHiEitxE6/XLfLJNUN0B0lqmCZo6UxvfaQlvrPkdxh1Exn/uTYelclxnCZ8oxo+QrC GoGDTcQMHEPC455mN8+pUcNjJjoNfh171zivMygMmN8/Y6O4OdrpwsifU34wiKVbm2C/ IXs0mu45tSCg1EXOH7lOoyQwLeYQFAG5zBznE7s6/Gm6zwbGdRaOPA+MZQWh9tXeaNIZ iffA== X-Gm-Message-State: ANhLgQ1AnP+oC60myzJjOTx6if0tHBKoOr0AHMPxK3YctI1cC+bv6K5w XdsaSuCH49DjYHtB5Ncp2bGVgQ== X-Google-Smtp-Source: ADFU+vuTyAaFA2npgdFqjRYXpaxDE1HUWgpH+pwG8WigHeZrsxq+jsVid3H9+SuZccMT8F5L16epkA== X-Received: by 2002:a1c:4054:: with SMTP id n81mr4563679wma.114.1585045351954; Tue, 24 Mar 2020 03:22:31 -0700 (PDT) Received: from localhost (host86-186-80-207.range86-186.btcentralplus.com. [86.186.80.207]) by smtp.gmail.com with ESMTPSA id 71sm13036747wrc.53.2020.03.24.03.22.31 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 24 Mar 2020 03:22:31 -0700 (PDT) Date: Tue, 24 Mar 2020 10:22:30 +0000 From: Andrew Burgess To: Scott Linder Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] [gdb] Support frames inlined into the outer frame Message-ID: <20200324102230.GU3317@embecosm.com> References: <09367f43-a9a5-cfb1-f854-8084653f0c70@simark.ca> <20200318221119.14811-1-scott@scottlinder.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200318221119.14811-1-scott@scottlinder.com> X-Operating-System: Linux/4.18.19-100.fc27.x86_64 (x86_64) X-Uptime: 09:33:30 up 38 days, 21:02, X-Fortune: the daemons! the daemons! the terrible daemons! X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] User-Agent: Mutt/1.9.2 (2017-12-15) X-Spam-Status: No, score=-26.4 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: Tue, 24 Mar 2020 10:22:52 -0000 Scott, Thanks for looking into this. * Scott Linder [2020-03-18 18:11:19 -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. I'm curious as to which target you're working on seeing this issue. > > 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. > > * frame.h (outer_frame_id): Update comment. > (frame_id_p): Update comment. > > * inline-frame.c (inline_frame_this_id): Remove assert that prevents > inline frame ids in outer frame. ChangeLog entries don't have blank lines between entries. > > Change-Id: I8aa129c667dccc31590ffdf426586418493a6ebe > --- > gdb/frame.c | 11 ++++++----- > gdb/frame.h | 7 ++++--- > gdb/inline-frame.c | 4 ---- > 3 files changed, 10 insertions(+), 12 deletions(-) > > diff --git a/gdb/frame.c b/gdb/frame.c > index d74d1d5c7c..b62d68f12a 100644 > --- a/gdb/frame.c > +++ b/gdb/frame.c > @@ -694,8 +694,8 @@ frame_id_p (struct frame_id l) > > /* 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) > + /* outer_frame_id and functions inlined into it are also valid. */ > + if (!p && l.special_addr_p) It's not immediately obvious how the comment relates to the code below. I wonder if changing the code to: if (!p && is_outer_frame_p (l)) would be better, where is_outer_frame_p is a new function. My motivation here is that we previously checked all fields of l, not just the special_addr_p field - I wonder if we should restore more of that checking? Specifically, the value in the special_addr field? My understanding is that if (!p) then (special_addr == 0) - so maybe we can assert that. > p = 1; > if (frame_debug) > { > @@ -722,12 +722,13 @@ frame_id_eq (struct frame_id l, struct frame_id r) > > 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 > + /* The outermost frame marker, and any inline frame markers > + derived from it, are equal to themselves. 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. */ I think we need to rewrite more of this comment. The part that says "...since between execution steps we might step into another function..." now seems ambiguous. The point this is trying to make is that if we step into a totally different function that also identifies as outer_frame_id then we can't tell the difference between the original function and the new one. The problem is if we step into an inline function then we can (now) tell them apart. Maybe something like: The outermost frame marker, and any inilne 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 = 1; > + 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. > diff --git a/gdb/frame.h b/gdb/frame.h > index cfc15022ed..d394382903 100644 > --- a/gdb/frame.h > +++ b/gdb/frame.h > @@ -195,7 +195,8 @@ 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 is only special_addr_p, and possibly > + artificial_depth, set. */ > extern const struct frame_id outer_frame_id; I'd drop the "possibly" from this comment, as the value is always valid, right? 0 means no inline frames, and is just as set as a value of 1 or more. Saying possible risks that someone might think they need to figure out if artificial_depth is set or not, when this is not the case. The implementation has special_addr set to 0, special_addr_p set to 1, and artificial_depth set to 0 or greater. > > /* Flag to control debugging. */ > @@ -237,8 +238,8 @@ extern struct frame_id > extern struct frame_id frame_id_build_wild (CORE_ADDR stack_addr); > > /* Returns non-zero when L is a valid frame (a valid frame has a > - non-zero .base). The outermost frame is valid even without an > - ID. */ > + non-zero .base). The outermost frame and any frames inlined into it > + are valid even without an ID. */ > extern int frame_id_p (struct frame_id l); I personally wouldn't make this change. You're broadening the definition of outer_frame_id, so the original comment is still valid, anything else is detail. > > /* Returns non-zero when L is a valid frame representing a frame made up by GDB > 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 > It would be great if we could come up for some tests for this new code, but I can't immediately see how you might setup something like this on any of the targets I'm most familiar with. Also do you have a copyright assignment in place as I think one would be need to take this patch. Thanks, Andrew