From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x330.google.com (mail-wm1-x330.google.com [IPv6:2a00:1450:4864:20::330]) by sourceware.org (Postfix) with ESMTPS id 825DE3855008 for ; Mon, 23 Aug 2021 09:42:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 825DE3855008 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=embecosm.com Received: by mail-wm1-x330.google.com with SMTP id j14-20020a1c230e000000b002e748b9a48bso1724730wmj.0 for ; Mon, 23 Aug 2021 02:42:02 -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; bh=Gs7/O4rxKlt050Ry977BLYnHr/RuRSbroPLsFj94mtw=; b=WMWdBr54kjyDkrTR6beL+lsGBnlTfZVWX2woxk3Cp+K/K5ERnE67WKO+s4OZ968U+G R1HRvY0THcpwej/UXwtUoPsTBqyBw1L0jRXDKPXU/Z3jcPz6SHYIFtectMyNCQmP0tsL Jehqh9HeGBF9DlR1/IKTCF1Xrt/AlHMuQfeeKSfteGq3OY8WwJ32l4QP6NGu5Hi1HouL sgtjso2tzZiujEk7rY/FaMapD8REC3rVa+Rl454FqJkKBUjqxe4/ipnRp1zc3p4tanGR 7F31vzzTs9fgfW5uFjjdd6kmZHH1TOasIsjuuqpqgL80+qn9cPGnJc6XLpWdLJSwIopD 0AFQ== 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; bh=Gs7/O4rxKlt050Ry977BLYnHr/RuRSbroPLsFj94mtw=; b=jRhik82OsK4365R8ZEYn3L7klPdrv2CfZjmcWpibOu726Wfw16j8hqaTi0olE1rqOf NoX2cpkMdsAybQ28WMlqes7GqaGU+TA8YPsRNd9nciwOx2Fr2GXVf5CNyiuXJ2bxH/Bn a2liidZpWFgOajCo8ccuHKg1aAAj+sIJKv+SmBISmMNyoDTS/O+/moVj187s2ahU62r/ wVdT3bzksO8pd54K9yeuebSfmJPk2awFotBBzIcgUfpEZ3WoeB1enBRvJHdfj7Ge5WOQ /d8opkweO8LPFeH1vujCZcB/be+Z49FtwmUFImm3KbL0U+sp0osAkJKGL2QiEW/P6KBw jXbA== X-Gm-Message-State: AOAM532sXdmd+16T7OZ52Z7g8VCo28qzxHOhgf6PtyKA5yOwCHcm82nW 7hj0gc9cItHoAD2rhwdFAx0LbaGH/zsZRg== X-Google-Smtp-Source: ABdhPJwNTPUtSNo1nhjjLqUxxxgbt5tHCZu7JlQIlC4rQ7E1LnqwNiW3omd4tDP3Z52ob/8fQBgw1g== X-Received: by 2002:a05:600c:3594:: with SMTP id p20mr8444450wmq.128.1629711721154; Mon, 23 Aug 2021 02:42:01 -0700 (PDT) Received: from localhost (host86-188-49-44.range86-188.btcentralplus.com. [86.188.49.44]) by smtp.gmail.com with ESMTPSA id e4sm14819956wrw.74.2021.08.23.02.42.00 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 23 Aug 2021 02:42:00 -0700 (PDT) Date: Mon, 23 Aug 2021 10:41:59 +0100 From: Andrew Burgess To: gdb-patches@sourceware.org Subject: Re: [PATCHv5] gdb: prevent an assertion when computing the frame_id for an inline frame Message-ID: <20210823094159.GC2581@embecosm.com> References: <20210727101003.2910993-1-andrew.burgess@embecosm.com> <20210809154122.3468792-1-andrew.burgess@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210809154122.3468792-1-andrew.burgess@embecosm.com> X-Operating-System: Linux/5.8.18-100.fc31.x86_64 (x86_64) X-Uptime: 10:38:09 up 5 days, 22:34, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-12.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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, 23 Aug 2021 09:42:13 -0000 Ping! Simon, what are your thoughts on this approach? From what I recall of our discussion on IRC your concerns with my original patch were that after my change the function implementation no longer matched with the function name, this made GDB's internal APIs confusing and inconsistent. With this patch I've tried to avoid this by renaming the functions to hopefully make the API clearer. I'm just guessing, but I suspect it was the use of exceptions that Pedro wasn't happy with, so maybe if you're happy with this latest patch we could merge this change, given Pedro already approved the pre-exception patch. Thanks, Andrew * Andrew Burgess [2021-08-09 16:41:22 +0100]: > I've not heard anything from Pedro, but I'd like to move this patch > forward. > > My assumption is that Pedro doesn't like using exceptions to pass > around information for a case that maybe isn't that exceptional. As > Pedro was happy with v2, this new patch removes the use of exceptions. > > Simon's concerns with v1/v2 were, I think, summarised as: > > 1. What get_prev_frame_if_no_cycle does no longer matches the > function name, and > > 2. get_prev_frame_if_no_cycle was trying to figure out the callers > intention in order to change its behaviour. > > To try and address these two things I have: > > 1. Renamed get_prev_frame_if_no_cycle to hopefully make it clearer > that its result is not so clear cut, and > > 2. Changed the behaviour of get_prev_frame_if_no_cycle based on a > passed in parameter instead of "peeking" at various bits of state. > > My hope is that this might be more acceptable to everyone, but I'd > love to hear any thoughts, > > Thanks, > Andrew > > --- > > I ran into this assertion while GDB was trying to unwind the stack: > > gdb/inline-frame.c:173: internal-error: void inline_frame_this_id(frame_info*, void**, frame_id*): Assertion `frame_id_p (*this_id)' failed. > > That is, when building the frame_id for an inline frame, GDB asks for > the frame_id of the previous frame. Unfortunately, no valid frame_id > was returned for the previous frame, and so the assertion triggers. > > What is happening is this, I had a stack that looked something like > this (the arrows '->' point from caller to callee): > > normal_frame -> inline_frame > > However, for whatever reason (e.g. broken debug information, or > corrupted stack contents in the inferior), when GDB tries to unwind > "normal_frame", it ends up getting back effectively the same frame, > thus the call stack looks like this to GDB: > > .-> normal_frame -> inline_frame > | | > '-----' > > Given such a situation we would expect GDB to terminate the stack with > an error like this: > > Backtrace stopped: previous frame identical to this frame (corrupt stack?) > > However, the inline_frame causes a problem, and here's why: > > When unwinding we start from the sentinel frame and call > get_prev_frame. We eventually end up in get_prev_frame_if_no_cycle, > in here we create a raw frame, and as this is frame #0 we immediately > return. > > However, eventually we will try to unwind the stack further. When we > do this we inevitably needing to know the frame_id for frame #0, and > so, eventually, we end up in compute_frame_id. > > In compute_frame_id we first find the right unwinder for this frame, > in our case (i.e. for inline_frame) the $pc is within the function > normal_frame, but also within a block associated with the inlined > function inline_frame, as such the inline frame unwinder claims this > frame. > > Back in compute_frame_id we next compute the frame_id, for our > inline_frame this means a call to inline_frame_this_id. > > The ID of an inline frame is based on the id of the previous frame, so > from inline_frame_this_id we call get_prev_frame_always, this > eventually calls get_prev_frame_if_no_cycle again, which creates > another raw frame and calls compute_frame_id (for frames other than > frame 0 we immediately compute the frame_id). > > In compute_frame_id we again identify the correct unwinder for this > frame. Our $pc is unchanged, however, the fact that the next frame is > of type INLINE_FRAME prevents the inline frame unwinder from claiming > this frame again, and so, the standard DWARF frame unwinder claims > normal_frame. > > We return to compute_frame_id and call the standard DWARF function to > build the frame_id for normal_frame. > > With the frame_id of normal_frame figured out we return to > compute_frame_id, and then to get_prev_frame_if_no_cycle, where we add > the ID for normal_frame into the frame_id cache, and return the frame > back to inline_frame_this_id. > > From inline_frame_this_id we build a frame_id for inline_frame and > return to compute_frame_id, and then to get_prev_frame_if_no_cycle, > which adds the frame_id for inline_frame into the frame_id cache. > > So far, so good. > > However, as we are trying to unwind the complete stack, we eventually > ask for the previous frame of normal_frame, remember, at this point > GDB doesn't know the stack is corrupted (with a cycle), GDB still > needs to figure that out. > > So, we eventually end up in get_prev_frame_if_no_cycle where we create > a raw frame and call compute_frame_id, remember, this is for the frame > before normal_frame. > > The first task for compute_frame_id is to find the unwinder for this > frame, so all of the frame sniffers are tried in order, this includes > the inline frame sniffer. > > The inline frame sniffer asks for the $pc, this request is sent up the > stack to normal_frame, which, due to its cyclic behaviour, tells GDB > that the $pc in the previous frame was the same as the $pc in > normal_frame. > > GDB spots that this $pc corresponds to both the function normal_frame > and also the inline function inline_frame. As the next frame is not > an INLINE_FRAME then GDB figures that we have not yet built a frame to > cover inline_frame, and so the inline sniffer claims this new frame. > Our stack is now looking like this: > > inline_frame -> normal_frame -> inline_frame > > But, we have not yet computed the frame id for the outer most (on the > left) inline_frame. After the frame sniffer has claimed the inline > frame GDB returns to compute_frame_id and calls inline_frame_this_id. > > In here GDB calls get_prev_frame_always, which eventually ends up > in get_prev_frame_if_no_cycle again, where we create a raw frame and > call compute_frame_id. > > Just like before, compute_frame_id tries to find an unwinder for this > new frame, it sees that the $pc is within both normal_frame and > inline_frame, but the next frame is, again, an INLINE_FRAME, so, just > like before the standard DWARF unwinder claims this frame. Back in > compute_frame_id we again call the standard DWARF function to build > the frame_id for this new copy of normal_frame. > > At this point the stack looks like this: > > normal_frame -> inline_frame -> normal_frame -> inline_frame > > After compute_frame_id we return to get_prev_frame_if_no_cycle, where > we try to add the frame_id for the new normal_frame into the frame_id > cache, however, unlike before, we fail to add this frame_id as it is > a duplicate of the previous normal_frame frame_id. Having found a > duplicate get_prev_frame_if_no_cycle unlinks the new frame from the > stack, and returns nullptr, the stack now looks like this: > > inline_frame -> normal_frame -> inline_frame > > The nullptr result from get_prev_frame_if_no_cycle is fed back to > inline_frame_this_id, which forwards this to get_frame_id, which > immediately returns null_frame_id. As null_frame_id is not considered > a valid frame_id, this is what triggers the assertion. > > In summary then: > > - inline_frame_this_id currently assumes that as the inline frame > exists, we will always get a valid frame back from > get_prev_frame_always, > > - get_prev_frame_if_no_cycle currently assumes that it is safe to > return nullptr when it sees a cycle. > > Notice that in frame.c:compute_frame_id, this code: > > fi->this_id.value = outer_frame_id; > fi->unwind->this_id (fi, &fi->prologue_cache, &fi->this_id.value); > gdb_assert (frame_id_p (fi->this_id.value)); > > The assertion makes it clear that the this_id function must always > return a valid frame_id (e.g. null_frame_id is not a valid return > value), and similarly in inline_frame.c:inline_frame_this_id this > code: > > *this_id = get_frame_id (get_prev_frame_always (this_frame)); > /* snip comment */ > gdb_assert (frame_id_p (*this_id)); > > Makes it clear that every inline frame expects to be able to get a > previous frame, which will have a valid frame_id. > > As I have discussed above, these assumptions don't currently hold in > all cases. > > One possibility would be to move the call to get_prev_frame_always > forward from inline_frame_this_id to inline_frame_sniffer, however, > this falls foul of (in frame.c:frame_cleanup_after_sniffer) this > assertion: > > /* No sniffer should extend the frame chain; sniff based on what is > already certain. */ > gdb_assert (!frame->prev_p); > > This assert prohibits any sniffer from trying to get the previous > frame, as getting the previous frame is likely to depend on the next > frame, I can understand why this assertion is a good thing, and I'm in > no rush to alter this rule. > > In a previous version of this patch: > > https://sourceware.org/pipermail/gdb-patches/2021-June/180208.html > > I proposed adding a special case to get_prev_frame_if_no_cycle, such > that, if we find a cycle, and we know we are fetching the previous > frame as a result of computing the frame_id for the next frame, which > is an INLINE_FRAME, then, instead of returning nullptr, do still > return the frame. > > The idea here was to make adding the "normal_frame -> inline_frame ->" > to the frame list more of an atomic(-ish) operation, we would defer > removing normal_frame if we know is needed to support inline_frame, > and inline_frame will disconnect both if appropriate. > > This approach was liked by some: > > https://sourceware.org/pipermail/gdb-patches/2021-July/180651.html > > But not by everyone: > > https://sourceware.org/pipermail/gdb-patches/2021-July/180663.html > > Based on Simon's feedback, I then proposed an alternative patch: > > https://sourceware.org/pipermail/gdb-patches/2021-July/181029.html > > With this approach, in inline_frame_this_id, GDB spots when the call > to get_prev_frame_always returns nullptr. This nullptr indicates that > we failed to get the previous frame for some reason. > > We can then call get_frame_unwind_stop_reason on the current frame. > The stop reason will have been updated to indicate why we couldn't > find a previous frame. > > In the specific case that the unwind stop reason is UNWIND_SAME_ID > then we know that the normal_frame had a duplicate frame-id. For this > case we throw a new exception type (INLINE_FRAME_ID_SAME_ID_ERROR). > For any other stop reason we throw an existing more generic > error (NOT_FOUND_ERROR) to indicate the frame-id (of the inline frame) > was not found. > > The other part is to catch INLINE_FRAME_ID_SAME_ID_ERROR in > get_prev_frame_if_no_cycle. We can then push the UNWIND_SAME_ID stop > reason up the frame stack. > > Unfortunately, this approach wasn't liked either: > > https://sourceware.org/pipermail/gdb-patches/2021-July/181035.html > > So now I'm proposing a new patch, which is closer to the first patch > again. > > The concern with the first patch was that that changes I made to > get_prev_frame_if_no_cycle, broke the defined API, it was more, return > the previous frame if there's no cycle, except in some cases where we > do still want the previous frame. > > And, this criticism about the function no longer doing what the name > suggests is fair, however, while is patch 2 I tried to solve this by > not changing how the function behaves, in this patch I just lean into > this, and rename the function to > get_prev_frame_with_optional_cycle_detection, now, I suggest, the > function does exactly what its name implies. > > In this version I have taken a slightly different approach than in the > first patch, get_prev_frame_if_no_cycle (as it was called) is only > called from get_prev_frame_always_1 (twice), one of these calls > handles the inline frame case, while the other call handles all other > frame types. I now pass a parameter through from each call site to > control the behaviour of the new > get_prev_frame_with_optional_cycle_detection, but the general idea is > the same as with patch 1. > > When we ask for the previous frame of an inline frame we will not > reject a candidate frame just because it has a duplicate frame_id. We > assume that if the previous frame has a duplicate frame_id then the > inline frame will also have a duplicate frame_id, and thus will be > rejected. > --- > gdb/frame.c | 56 ++++++- > gdb/inline-frame.c | 5 +- > .../gdb.base/inline-frame-cycle-unwind.c | 58 +++++++ > .../gdb.base/inline-frame-cycle-unwind.exp | 145 ++++++++++++++++++ > .../gdb.base/inline-frame-cycle-unwind.py | 85 ++++++++++ > 5 files changed, 343 insertions(+), 6 deletions(-) > create mode 100644 gdb/testsuite/gdb.base/inline-frame-cycle-unwind.c > create mode 100644 gdb/testsuite/gdb.base/inline-frame-cycle-unwind.exp > create mode 100644 gdb/testsuite/gdb.base/inline-frame-cycle-unwind.py > > diff --git a/gdb/frame.c b/gdb/frame.c > index 4d7505f7ae3..397e3c664d6 100644 > --- a/gdb/frame.c > +++ b/gdb/frame.c > @@ -2044,13 +2044,54 @@ frame_register_unwind_location (struct frame_info *this_frame, int regnum, > outermost, with UNWIND_SAME_ID stop reason. Unlike the other > validity tests, that compare THIS_FRAME and the next frame, we do > this right after creating the previous frame, to avoid ever ending > - up with two frames with the same id in the frame chain. */ > + up with two frames with the same id in the frame chain. > + > + There is however, one case where this cycle detection is not desirable, > + when asking for the previous frame of an inline frame, in this case, if > + the previous frame is a duplicate and we return nullptr then we will be > + unable to calculate the frame_id of the inline frame, this in turn > + causes inline_frame_this_id() to fail. So for inline frames (and only > + for inline frames) it is acceptable to pass CYCLE_DETECTION_P as false, > + in that case the previous frame will always be returned, even when it > + has a duplicate frame_id. We're not worried about cycles in the frame > + chain as, if the previous frame returned here has a duplicate frame_id, > + then the frame_id of the inline frame, calculated based off the frame_id > + of the previous frame, should also be a duplicate. */ > > static struct frame_info * > -get_prev_frame_if_no_cycle (struct frame_info *this_frame) > +get_prev_frame_with_optional_cycle_detection (struct frame_info *this_frame, > + bool cycle_detection_p) > { > struct frame_info *prev_frame; > > + /* This assert primarily checks that CYCLE_DETECTION_P is only false for > + inline frames. However, this assertion also makes some claims about > + what the state of GDB should be when we enter this function and > + THIS_FRAME is an inline frame. > + > + If frame #0 is an inline frame then we put off calculating the > + frame_id until we specifically make a call to get_frame_id(). As a > + result we can enter this function in two possible states. If GDB > + asked for the previous frame of frame #0 then THIS_FRAME will be frame > + #0 (an inline frame), and the frame_id will be in the NOT_COMPUTED > + state. However, if GDB asked for the frame_id of frame #0, then, as > + getting the frame_id of an inline frame requires us to get the > + frame_id of the previous frame, we will still end up in here, and the > + frame_id status will be COMPUTING. > + > + If we consider an inline frame at a level greater than #0 then things > + are simpler. For these frames we immediately compute the frame_id, > + and so, for those frames, we will always reenter this function with > + the frame_id status of COMPUTING. */ > + gdb_assert (cycle_detection_p > + || (get_frame_type (this_frame) == INLINE_FRAME > + && ((this_frame->level > 0 > + && (this_frame->this_id.p > + == frame_id_status::COMPUTING)) > + || (this_frame->level == 0 > + && (this_frame->this_id.p > + != frame_id_status::COMPUTED))))); > + > prev_frame = get_prev_frame_raw (this_frame); > > /* Don't compute the frame id of the current frame yet. Unwinding > @@ -2070,7 +2111,12 @@ get_prev_frame_if_no_cycle (struct frame_info *this_frame) > try > { > compute_frame_id (prev_frame); > - if (!frame_stash_add (prev_frame)) > + > + /* We must do the CYCLE_DETECTION_P check after attempting to add > + PREV_FRAME into the cache; if PREV_FRAME is unique then we do want > + it in the cache, but if it is a duplicate and CYCLE_DETECTION_P is > + false, then we don't want to unlink it. */ > + if (!frame_stash_add (prev_frame) && cycle_detection_p) > { > /* Another frame with the same id was already in the stash. We just > detected a cycle. */ > @@ -2147,7 +2193,7 @@ get_prev_frame_always_1 (struct frame_info *this_frame) > until we have unwound all the way down to the previous non-inline > frame. */ > if (get_frame_type (this_frame) == INLINE_FRAME) > - return get_prev_frame_if_no_cycle (this_frame); > + return get_prev_frame_with_optional_cycle_detection (this_frame, false); > > /* If this_frame is the current frame, then compute and stash its > frame id prior to fetching and computing the frame id of the > @@ -2248,7 +2294,7 @@ get_prev_frame_always_1 (struct frame_info *this_frame) > } > } > > - return get_prev_frame_if_no_cycle (this_frame); > + return get_prev_frame_with_optional_cycle_detection (this_frame, true); > } > > /* Return a "struct frame_info" corresponding to the frame that called > diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c > index c98af1842a6..8fb5eb05609 100644 > --- a/gdb/inline-frame.c > +++ b/gdb/inline-frame.c > @@ -163,7 +163,10 @@ inline_frame_this_id (struct frame_info *this_frame, > function, there must be previous frames, so this is safe - as > long as we're careful not to create any cycles. See related > comments in get_prev_frame_always_1. */ > - *this_id = get_frame_id (get_prev_frame_always (this_frame)); > + frame_info *prev_frame = get_prev_frame_always (this_frame); > + if (prev_frame == nullptr) > + error ("failed to find previous frame when computing inline frame id"); > + *this_id = get_frame_id (prev_frame); > > /* We need a valid frame ID, so we need to be based on a valid > frame. FSF submission NOTE: this would be a good assertion to > diff --git a/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.c b/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.c > new file mode 100644 > index 00000000000..183c40928b6 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.c > @@ -0,0 +1,58 @@ > +/* This testcase is part of GDB, the GNU debugger. > + > + Copyright 2021 Free Software Foundation, Inc. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +static void inline_func (void); > +static void normal_func (void); > + > +volatile int global_var; > +volatile int level_counter; > + > +static void __attribute__((noinline)) > +normal_func (void) > +{ > + /* Do some work. */ > + ++global_var; > + > + /* Now the inline function. */ > + --level_counter; > + inline_func (); > + ++level_counter; > + > + /* Do some work. */ > + ++global_var; > +} > + > +static inline void __attribute__((__always_inline__)) > +inline_func (void) > +{ > + if (level_counter > 1) > + { > + --level_counter; > + normal_func (); > + ++level_counter; > + } > + else > + ++global_var; /* Break here. */ > +} > + > +int > +main () > +{ > + level_counter = 6; > + normal_func (); > + return 0; > +} > diff --git a/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.exp b/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.exp > new file mode 100644 > index 00000000000..2801b683a03 > --- /dev/null > +++ b/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.exp > @@ -0,0 +1,145 @@ > +# Copyright (C) 2021 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see . > + > +# This test checks for an edge case when unwinding inline frames which > +# occur towards the older end of the stack when the stack ends with a > +# cycle. Consider this well formed stack: > +# > +# main -> normal_frame -> inline_frame > +# > +# Now consider that, for whatever reason, the stack unwinding of > +# "normal_frame" becomes corrupted, such that the stack appears to be > +# this: > +# > +# .-> normal_frame -> inline_frame > +# | | > +# '------' > +# > +# When confronted with such a situation we would expect GDB to detect > +# the stack frame cycle and terminate the backtrace at the first > +# instance of "normal_frame" with a message: > +# > +# Backtrace stopped: previous frame identical to this frame (corrupt stack?) > +# > +# However, at one point there was a bug in GDB's inline frame > +# mechanism such that the fact that "inline_frame" was inlined into > +# "normal_frame" would cause GDB to trigger an assertion. > +# > +# This text makes use of a Python unwinder which can fake the cyclic > +# stack cycle, further the test sets up multiple levels of normal and > +# inline frames. At the point of testing the stack looks like this: > +# > +# main -> normal_func -> inline_func -> normal_func -> inline_func -> normal_func -> inline_func > +# > +# Where "normal_func" is a normal frame, and "inline_func" is an inline frame. > +# > +# The python unwinder is then used to force a stack cycle at each > +# "normal_func" frame in turn, we then check that GDB can successfully unwind > +# the stack. > + > +standard_testfile > + > +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}]} { > + return -1 > +} > + > +# Skip this test if Python scripting is not enabled. > +if { [skip_python_tests] } { continue } > + > +if ![runto_main] then { > + fail "can't run to main" > + return 0 > +} > + > +set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py] > + > +# Run to the breakpoint where we will carry out the test. > +gdb_breakpoint [gdb_get_line_number "Break here"] > +gdb_continue_to_breakpoint "stop at test breakpoint" > + > +# Load the script containing the unwinder, this must be done at the > +# testing point as the script will examine the stack as it is loaded. > +gdb_test_no_output "source ${pyfile}"\ > + "import python scripts" > + > +# Check the unbroken stack. > +gdb_test_sequence "bt" "backtrace when the unwind is left unbroken" { > + "\\r\\n#0 \[^\r\n\]* inline_func \\(\\) at " > + "\\r\\n#1 \[^\r\n\]* normal_func \\(\\) at " > + "\\r\\n#2 \[^\r\n\]* inline_func \\(\\) at " > + "\\r\\n#3 \[^\r\n\]* normal_func \\(\\) at " > + "\\r\\n#4 \[^\r\n\]* inline_func \\(\\) at " > + "\\r\\n#5 \[^\r\n\]* normal_func \\(\\) at " > + "\\r\\n#6 \[^\r\n\]* main \\(\\) at " > +} > + > +with_test_prefix "cycle at level 5" { > + # Arrange to introduce a stack cycle at frame 5. > + gdb_test_no_output "python stop_at_level=5" > + gdb_test "maint flush register-cache" \ > + "Register cache flushed\\." > + gdb_test_lines "bt" "backtrace when the unwind is broken at frame 5" \ > + [multi_line \ > + "#0 \[^\r\n\]* inline_func \\(\\) at \[^\r\n\]+" \ > + "#1 \[^\r\n\]* normal_func \\(\\) at \[^\r\n\]+" \ > + "#2 \[^\r\n\]* inline_func \\(\\) at \[^\r\n\]+" \ > + "#3 \[^\r\n\]* normal_func \\(\\) at \[^\r\n\]+" \ > + "#4 \[^\r\n\]* inline_func \\(\\) at \[^\r\n\]+" \ > + "#5 \[^\r\n\]* normal_func \\(\\) at \[^\r\n\]+" \ > + "Backtrace stopped: previous frame identical to this frame \\(corrupt stack\\?\\)"] > +} > + > +with_test_prefix "cycle at level 3" { > + # Arrange to introduce a stack cycle at frame 3. > + gdb_test_no_output "python stop_at_level=3" > + gdb_test "maint flush register-cache" \ > + "Register cache flushed\\." > + gdb_test_lines "bt" "backtrace when the unwind is broken at frame 3" \ > + [multi_line \ > + "#0 \[^\r\n\]* inline_func \\(\\) at \[^\r\n\]+" \ > + "#1 \[^\r\n\]* normal_func \\(\\) at \[^\r\n\]+" \ > + "#2 \[^\r\n\]* inline_func \\(\\) at \[^\r\n\]+" \ > + "#3 \[^\r\n\]* normal_func \\(\\) at \[^\r\n\]+" \ > + "Backtrace stopped: previous frame identical to this frame \\(corrupt stack\\?\\)"] > +} > + > +with_test_prefix "cycle at level 1" { > + # Arrange to introduce a stack cycle at frame 1. > + gdb_test_no_output "python stop_at_level=1" > + gdb_test "maint flush register-cache" \ > + "Register cache flushed\\." > + gdb_test_lines "bt" "backtrace when the unwind is broken at frame 1" \ > + [multi_line \ > + "#0 \[^\r\n\]* inline_func \\(\\) at \[^\r\n\]+" \ > + "#1 \[^\r\n\]* normal_func \\(\\) at \[^\r\n\]+" \ > + "Backtrace stopped: previous frame identical to this frame \\(corrupt stack\\?\\)"] > +} > + > +# Flush the register cache (which also flushes the frame cache) so we > +# get a full backtrace again, then switch on frame debugging and try > +# to back trace. At one point this triggered an assertion. > +gdb_test "maint flush register-cache" \ > + "Register cache flushed\\." "" > +gdb_test_no_output "set debug frame 1" > +gdb_test_multiple "bt" "backtrace with debugging on" { > + -re "^$gdb_prompt $" { > + pass $gdb_test_name > + } > + -re "\[^\r\n\]+\r\n" { > + exp_continue > + } > +} > +gdb_test "p 1 + 2 + 3" " = 6" \ > + "ensure GDB is still alive" > diff --git a/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.py b/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.py > new file mode 100644 > index 00000000000..99c571f973c > --- /dev/null > +++ b/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.py > @@ -0,0 +1,85 @@ > +# Copyright (C) 2021 Free Software Foundation, Inc. > + > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 3 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see . > + > +import gdb > +from gdb.unwinder import Unwinder > + > +# Set this to the stack level the backtrace should be corrupted at. > +# This will only work for frame 1, 3, or 5 in the test this unwinder > +# was written for. > +stop_at_level = None > + > +# Set this to the stack frame size of frames 1, 3, and 5. These > +# frames will all have the same stack frame size as they are the same > +# function called recursively. > +stack_adjust = None > + > + > +class FrameId(object): > + def __init__(self, sp, pc): > + self._sp = sp > + self._pc = pc > + > + @property > + def sp(self): > + return self._sp > + > + @property > + def pc(self): > + return self._pc > + > + > +class TestUnwinder(Unwinder): > + def __init__(self): > + Unwinder.__init__(self, "stop at level") > + > + def __call__(self, pending_frame): > + global stop_at_level > + global stack_adjust > + > + if stop_at_level is None or pending_frame.level() != stop_at_level: > + return None > + > + if stack_adjust is None: > + raise gdb.GdbError("invalid stack_adjust") > + > + if not stop_at_level in [1, 3, 5]: > + raise gdb.GdbError("invalid stop_at_level") > + > + sp_desc = pending_frame.architecture().registers().find("sp") > + sp = pending_frame.read_register(sp_desc) + stack_adjust > + pc = (gdb.lookup_symbol("normal_func"))[0].value().address > + unwinder = pending_frame.create_unwind_info(FrameId(sp, pc)) > + > + for reg in pending_frame.architecture().registers("general"): > + val = pending_frame.read_register(reg) > + unwinder.add_saved_register(reg, val) > + return unwinder > + > + > +gdb.unwinder.register_unwinder(None, TestUnwinder(), True) > + > +# When loaded, it is expected that the stack looks like: > +# > +# main -> normal_func -> inline_func -> normal_func -> inline_func -> normal_func -> inline_func > +# > +# Compute the stack frame size of normal_func, which has inline_func > +# inlined within it. > +f0 = gdb.newest_frame() > +f1 = f0.older() > +f2 = f1.older() > +f0_sp = f0.read_register("sp") > +f2_sp = f2.read_register("sp") > +stack_adjust = f2_sp - f0_sp > -- > 2.25.4 >