From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x443.google.com (mail-wr1-x443.google.com [IPv6:2a00:1450:4864:20::443]) by sourceware.org (Postfix) with ESMTPS id 7417F385BF81 for ; Thu, 2 Jul 2020 21:07:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 7417F385BF81 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-wr1-x443.google.com with SMTP id z15so18815301wrl.8 for ; Thu, 02 Jul 2020 14:07:07 -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=AtVXrtemaxpZLCfXsiuYUaH0xvIbg7yBWdjhAJBWV60=; b=XyLhKlB4f2NeAHojThwDPLYvx4GBtPdXt1+oPZ0ODrMUTnDoNBXkTLAyZJxjgJcUTI TC6hEw/PWLfnu8MCGD7lNe5eTh1TMnwZG/oRQeRUkEI/fDuWlS2u6FfMxW1bG2j4DqWD 8H0U3YUISa/H2QIZcCNFp+vA9DcSmbfv/OOVGzMwVssMp01/oTJDmINMfwalp6V551Uc FfbFjqesHLVEt7NnWOQoTQ3cYlwPbfxfIxdfgw3QVNzT9iQWpd2C23WYUdtuFPYZq1r9 cNZIaiqPeXiuoHKeVLMzCTHRhNsO6w2Vos08vZhUn+KirrLiwHYnhZfpDsi5rUaD0b1B cFsA== 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=AtVXrtemaxpZLCfXsiuYUaH0xvIbg7yBWdjhAJBWV60=; b=J1A+Nxr4hLFK+ZOzXrmpHLaybyG69X5t3meXubmnh+QC3w/d51W4ighdDgs4t1M/Zd V8poKJwQJg6MY+duuJeqVav3PVJVlj1FpCXlb0O/u1U9vZ2saetMWDNxQSN5osxrx220 QuipL4xIMnxe5Oy+RWacp89pYLH6XSsjThGK82zIUVKndomgG7KCKSBSD3o2vWNSE+8P Hc+1mMJ6063y11cHpELbtbzdRi5t7HJdb5JuoDyMh0dBGxq0iB8lxmgTP7NCHRQ1i0lb Xn6oI4zPj15kNDXHomG3smu4HOO2Lh70oRn8lHtHxVd8YVN8iphOe38Fq+3rCfsR2tyW xWrQ== X-Gm-Message-State: AOAM531wFs23Cb9hGnRgJX3ETb6oM9nv3mUjd3//mK+NfY1Y6OZiHYp1 z2xhMVoCItDzQVL2gb1whnHS2WUZulI= X-Google-Smtp-Source: ABdhPJzwaU++1hktxIkJXSculrC1PfthDzwR3ZtYb4mgjCLqK1kNVV86N27Ya4pWXTIhghrvOUHHww== X-Received: by 2002:adf:ff90:: with SMTP id j16mr14860851wrr.364.1593724025984; Thu, 02 Jul 2020 14:07:05 -0700 (PDT) Received: from localhost (host109-154-72-253.range109-154.btcentralplus.com. [109.154.72.253]) by smtp.gmail.com with ESMTPSA id i19sm12711133wrb.56.2020.07.02.14.07.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 02 Jul 2020 14:07:04 -0700 (PDT) Date: Thu, 2 Jul 2020 22:07:03 +0100 From: Andrew Burgess To: gdb-patches@sourceware.org Cc: Luis Machado , Tom Tromey Subject: Re: [PATCH 5/5] gdb: Fix Python unwinders and inline frames Message-ID: <20200702210703.GT2737@embecosm.com> References: <20200622154723.GK2737@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200622154723.GK2737@embecosm.com> X-Operating-System: Linux/5.6.15-200.fc31.x86_64 (x86_64) X-Uptime: 22:06:15 up 24 days, 11:13, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-8.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_BARRACUDACENTRAL, 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: Thu, 02 Jul 2020 21:07:10 -0000 Ping! Unless anyone has any feedback I plan to push this series in the next couple of days. Thanks, Andrew * Andrew Burgess [2020-06-22 16:47:23 +0100]: > After feedback from Luis, and then discussion here: > > https://sourceware.org/pipermail/gdb-patches/2020-June/169679.html > > this is a completely new version of this patch that addresses the > original issue with Python unwinders, as well as addressing the issues > brought up in the discussion above. > > Thanks, > Andrew > > --- > > commit c66679e65c2f247a75d84a0166b07fed352879e1 > Author: Andrew Burgess > Date: Mon Jun 8 11:36:13 2020 +0100 > > gdb: Python unwinders, inline frames, and tail-call frames > > This started with me running into the bug described in python/22748, > in summary, if the frame sniffing code accessed any registers within > an inline frame then GDB would crash with this error: > > gdb/frame.c:579: internal-error: frame_id get_frame_id(frame_info*): Assertion `fi->level == 0' failed. > > The problem is that, when in the Python unwinder I write this: > > pending_frame.read_register ("register-name") > > This is translated internally into a call to `value_of_register', > which in turn becomes a call to `value_of_register_lazy'. > > Usually this isn't a problem, `value_of_register_lazy' requires the > next frame (more inner) to have a valid frame_id, which will be the > case (if we're sniffing frame #1, then frame #0 will have had its > frame-id figured out). > > Unfortunately if frame #0 is inline within frame #1, then the frame-id > for frame #0 can't be computed until we have the frame-id for #1. As > a result we can't create a lazy register for frame #1 when frame #0 is > inline. > > Initially I proposed a solution inline with that proposed in bugzilla, > changing value_of_register to avoid creating a lazy register value. > However, when this was discussed on the mailing list I got this reply: > > https://sourceware.org/pipermail/gdb-patches/2020-June/169633.html > > Which led me to look at these two patches: > > [1] https://sourceware.org/pipermail/gdb-patches/2020-April/167612.html > [2] https://sourceware.org/pipermail/gdb-patches/2020-April/167930.html > > When I considered patches [1] and [2] I saw that all of the issues > being addressed here were related, and that there was a single > solution that could address all of these issues. > > First I wrote the new test gdb.opt/inline-frame-tailcall.exp, which > shows that [1] and [2] regress the inline tail-call unwinder, the > reason for this is that these two patches replace a call to > gdbarch_unwind_pc with a call to get_frame_register, however, this is > not correct. The previous call to gdbarch_unwind_pc takes THIS_FRAME > and returns the $pc value in the previous frame. In contrast > get_frame_register takes THIS_FRAME and returns the value of the $pc > in THIS_FRAME; these calls are not equivalent. > > The reason these patches appear (or do) fix the regressions listed in > [1] is that the tail call sniffer depends on identifying the address > of a caller and a callee, GDB then looks for a tail-call sequence that > takes us from the caller address to the callee, if such a series is > found then tail-call frames are added. > > The bug that was being hit, and which was address in patch [1] is that > in order to find the address of the caller, GDB ended up creating a > lazy register value for an inline frame with to frame-id. The > solution in patch [1] is to instead take the address of the callee and > treat this as the address of the caller. Getting the address of the > callee works, but we then end up looking for a tail-call series from > the callee to the callee, which obviously doesn't return any sane > results, so we don't insert any tail call frames. > > The original patch [1] did cause some breakage, so patch [2] undid > patch [1] in all cases except those where we had an inline frame with > no frame-id. It just so happens that there were no tests that fitted > this description _and_ which required tail-call frames to be > successfully spotted, as a result patch [2] appeared to work. > > The new test inline-frame-tailcall.exp, exposes the flaw in patch [2]. > > This commit undoes patch [1] and [2], and replaces them with a new > solution, which is also different to the solution proposed in the > python/22748 bug report. > > In this solution I propose that we introduce some special case logic > to value_of_register_lazy. To understand what this logic is we must > first look at how inline frames unwind registers, this is very simple, > they do this: > > static struct value * > inline_frame_prev_register (struct frame_info *this_frame, > void **this_cache, int regnum) > { > return get_frame_register_value (this_frame, regnum); > } > > And remember: > > struct value * > get_frame_register_value (struct frame_info *frame, int regnum) > { > return frame_unwind_register_value (frame->next, regnum); > } > > So in all cases, unwinding a register in an inline frame just asks the > next frame to unwind the register, this makes sense, as an inline > frame doesn't really exist, when we unwind a register in an inline > frame, we're really just asking the next frame for the value of the > register in the previous, non-inline frame. > > So, if we assume that we only get into the missing frame-id situation > when we try to unwind a register from an inline frame during the frame > sniffing process, then we can change value_of_register_lazy to not > create lazy register values for an inline frame. > > Imagine this stack setup, where #1 is inline within #2. > > #3 -> #2 -> #1 -> #0 > \______/ > inline > > Now when trying to figure out the frame-id for #1, we need to compute > the frame-id for #2. If the frame sniffer for #2 causes a lazy > register read in #2, either due to a Python Unwinder, or for the > tail-call sniffer, then we call value_of_register_lazy passing in > frame #2. > > In value_of_register_lazy, we grab the next frame, which is #1, and we > used to then ask for the frame-id of #1, which was not computed, and > this was our bug. > > Now, I propose we spot that #1 is an inline frame, and so lookup the > next frame of #1, which is #0. As #0 is not inline it will have a > valid frame-id, and so we create a lazy register value using #0 as the > next-frame-id. This will give us the exact same result we had > previously (thanks to the code we inspected above). > > Encoding into value_of_register_lazy the knowledge that reading an > inline frame register will always just forward to the next frame > feels.... not ideal, but this seems like the cleanest solution to this > recursive frame-id computation/sniffing issue that appears to crop > up. > > The following two commits are fully reverted with this commit, these > correspond to patches [1] and [2] respectively: > > commit 5939967b355ba6a940887d19847b7893a4506067 > Date: Tue Apr 14 17:26:22 2020 -0300 > > Fix inline frame unwinding breakage > > commit 991a3e2e9944a4b3a27bd989ac03c18285bd545d > Date: Sat Apr 25 00:32:44 2020 -0300 > > Fix remaining inline/tailcall unwinding breakage for x86_64 > > gdb/ChangeLog: > > PR python/22748 > * dwarf2/frame-tailcall.c (dwarf2_tailcall_sniffer_first): Remove > special handling for inline frames. > * findvar.c (value_of_register_lazy): Skip inline frames when > creating lazy register values. > * frame.c (frame_id_computed_p): Delete definition. > * frame.h (frame_id_computed_p): Delete declaration. > > gdb/testsuite/ChangeLog: > > PR python/22748 > * gdb.opt/inline-frame-tailcall.c: New file. > * gdb.opt/inline-frame-tailcall.exp: New file. > * gdb.python/py-unwind-inline.c: New file. > * gdb.python/py-unwind-inline.exp: New file. > * gdb.python/py-unwind-inline.py: New file. > > diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c > index 16dba2b201a..2d219f13f9d 100644 > --- a/gdb/dwarf2/frame-tailcall.c > +++ b/gdb/dwarf2/frame-tailcall.c > @@ -384,43 +384,8 @@ dwarf2_tailcall_sniffer_first (struct frame_info *this_frame, > > prev_gdbarch = frame_unwind_arch (this_frame); > > - /* The dwarf2 tailcall sniffer runs early, at the end of populating the > - dwarf2 frame cache for the current frame. If there exists inline > - frames inner (next) to the current frame, there is a good possibility > - of that inline frame not having a computed frame id yet. > - > - This is because computing such a frame id requires us to walk through > - the frame chain until we find the first normal frame after the inline > - frame and then compute the normal frame's id first. > - > - Some architectures' compilers generate enough register location > - information for a dwarf unwinder to fetch PC without relying on inner > - frames (x86_64 for example). In this case the PC is retrieved > - according to dwarf rules. > - > - But others generate less strict dwarf data for which assumptions are > - made (like interpreting DWARF2_FRAME_REG_UNSPECIFIED as > - DWARF2_FRAME_REG_SAME_VALUE). For such cases, GDB may attempt to > - create lazy values for registers, and those lazy values must be > - created with a valid frame id, but we potentially have no valid id. > - > - So, to avoid breakage, if we see a dangerous situation with inline > - frames without a computed id, use safer functions to retrieve the > - current frame's PC. Otherwise use the provided dwarf rules. */ > - frame_info *next_frame = get_next_frame (this_frame); > - > /* Simulate frame_unwind_pc without setting this_frame->prev_pc.p. */ > - if (next_frame != nullptr && get_frame_type (next_frame) == INLINE_FRAME > - && !frame_id_computed_p (next_frame)) > - { > - /* The next frame is an inline frame and its frame id has not been > - computed yet. */ > - get_frame_register (this_frame, gdbarch_pc_regnum (prev_gdbarch), > - (gdb_byte *) &prev_pc); > - prev_pc = gdbarch_addr_bits_remove (prev_gdbarch, prev_pc); > - } > - else > - prev_pc = gdbarch_unwind_pc (prev_gdbarch, this_frame); > + prev_pc = gdbarch_unwind_pc (prev_gdbarch, this_frame); > > /* call_site_find_chain can throw an exception. */ > chain = call_site_find_chain (prev_gdbarch, prev_pc, this_pc); > diff --git a/gdb/findvar.c b/gdb/findvar.c > index c7cd31ce1a6..7e9dab567f6 100644 > --- a/gdb/findvar.c > +++ b/gdb/findvar.c > @@ -292,6 +292,14 @@ value_of_register_lazy (struct frame_info *frame, int regnum) > > next_frame = get_next_frame_sentinel_okay (frame); > > + /* In some cases NEXT_FRAME may not have a valid frame-id yet. This can > + happen if we end up trying to unwind a register as part of the frame > + sniffer. The only time that we get here without a valid frame-id is > + if NEXT_FRAME is an inline frame. If this is the case then we can > + avoid getting into trouble here by skipping past the inline frames. */ > + while (get_frame_type (next_frame) == INLINE_FRAME) > + next_frame = get_next_frame_sentinel_okay (next_frame); > + > /* We should have a valid next frame. */ > gdb_assert (frame_id_p (get_frame_id (next_frame))); > > diff --git a/gdb/frame.c b/gdb/frame.c > index ff27b9f00e9..ac1016b083f 100644 > --- a/gdb/frame.c > +++ b/gdb/frame.c > @@ -687,14 +687,6 @@ frame_id_build_wild (CORE_ADDR stack_addr) > return id; > } > > -bool > -frame_id_computed_p (struct frame_info *frame) > -{ > - gdb_assert (frame != nullptr); > - > - return frame->this_id.p != 0; > -} > - > int > frame_id_p (struct frame_id l) > { > diff --git a/gdb/frame.h b/gdb/frame.h > index e835d49f9ca..cfc15022ed5 100644 > --- a/gdb/frame.h > +++ b/gdb/frame.h > @@ -236,10 +236,6 @@ extern struct frame_id > as the special identifier address are set to indicate wild cards. */ > extern struct frame_id frame_id_build_wild (CORE_ADDR stack_addr); > > -/* Returns true if FRAME's id has been computed. > - Returns false otherwise. */ > -extern bool frame_id_computed_p (struct frame_info *frame); > - > /* 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. */ > diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.c b/gdb/testsuite/gdb.python/py-unwind-inline.c > new file mode 100644 > index 00000000000..0cfe06dd273 > --- /dev/null > +++ b/gdb/testsuite/gdb.python/py-unwind-inline.c > @@ -0,0 +1,37 @@ > +/* Copyright 2019-2020 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 . */ > + > +volatile int global_var; > + > +int __attribute__ ((noinline)) > +bar () > +{ > + return global_var; > +} > + > +static inline int __attribute__ ((always_inline)) > +foo () > +{ > + return bar (); > +} > + > +int > +main () > +{ > + int ans; > + global_var = 0; > + ans = foo (); > + return ans; > +} > diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.exp b/gdb/testsuite/gdb.python/py-unwind-inline.exp > new file mode 100644 > index 00000000000..f7c65f6afc8 > --- /dev/null > +++ b/gdb/testsuite/gdb.python/py-unwind-inline.exp > @@ -0,0 +1,49 @@ > +# Copyright (C) 2020 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 script tests GDB's handling of using a Python unwinder in the > +# presence of inlined frames. > + > +load_lib gdb-python.exp > + > +standard_testfile > + > +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile} \ > + debug] } { > + return -1 > +} > + > +# Skip all tests if Python scripting is not enabled. > +if { [skip_python_tests] } { continue } > + > +# The following tests require execution. > +if ![runto_main] then { > + fail "can't run to main" > + return 0 > +} > + > +set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py] > + > +gdb_breakpoint "foo" > + > +gdb_test "source ${pyfile}" "Python script imported" \ > + "import python scripts" > + > +gdb_continue_to_breakpoint "foo" > + > +gdb_test_sequence "backtrace" "backtrace with dummy unwinder" { > + "\\r\\n#0 foo \\(\\)" > + "\\r\\n#1 main \\(\\)" > +} > diff --git a/gdb/testsuite/gdb.python/py-unwind-inline.py b/gdb/testsuite/gdb.python/py-unwind-inline.py > new file mode 100644 > index 00000000000..7206a0b2830 > --- /dev/null > +++ b/gdb/testsuite/gdb.python/py-unwind-inline.py > @@ -0,0 +1,71 @@ > +# Copyright (C) 2020 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 . > + > +# A dummy stack unwinder used for testing the Python unwinders when we > +# have inline frames. This unwinder will never claim any frames, > +# instead, all it does it try to read all registers possible target > +# registers as part of the frame sniffing process.. > + > +import gdb > +from gdb.unwinder import Unwinder > + > +apb_global = None > + > +class dummy_unwinder (Unwinder): > + """ A dummy unwinder that looks at a bunch of registers as part of > + the unwinding process.""" > + > + class frame_id (object): > + """ Basic frame id.""" > + > + def __init__ (self, sp, pc): > + """ Create the frame id.""" > + self.sp = sp > + self.pc = pc > + > + def __init__ (self): > + """Create the unwinder.""" > + Unwinder.__init__ (self, "dummy stack unwinder") > + self.void_ptr_t = gdb.lookup_type("void").pointer() > + self.regs = None > + > + def get_regs (self, pending_frame): > + """Return a list of register names that should be read. Only > + gathers the list once, then caches the result.""" > + if (self.regs != None): > + return self.regs > + > + # Collect the names of all registers to read. > + self.regs = list (pending_frame.architecture () > + .register_names ()) > + > + return self.regs > + > + def __call__ (self, pending_frame): > + """Actually performs the unwind, or at least sniffs this frame > + to see if the unwinder should claim it, which is never does.""" > + try: > + for r in (self.get_regs (pending_frame)): > + v = pending_frame.read_register (r).cast (self.void_ptr_t) > + except: > + print ("Dummy unwinder, exception") > + raise > + > + return None > + > +# Register the ComRV stack unwinder. > +gdb.unwinder.register_unwinder (None, dummy_unwinder (), True) > + > +print ("Python script imported")