From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qv1-xf2a.google.com (mail-qv1-xf2a.google.com [IPv6:2607:f8b0:4864:20::f2a]) by sourceware.org (Postfix) with ESMTPS id 0E2443947C3A for ; Tue, 23 Jun 2020 14:16:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0E2443947C3A Received: by mail-qv1-xf2a.google.com with SMTP id fc4so9727755qvb.1 for ; Tue, 23 Jun 2020 07:16:41 -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:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=S0PH0HTolteqn7lYx0dRzmFPR2ZnYHItnEyjbkmbZlg=; b=fg+gDQpRKyNsLEzQc0TKAcwl+Bocit0khr4x+yjs487tO4z0R6dvz5ScpDbjFmOYpz 9FLIsL5uDpB7X831AHn25vUVp5oqiLNwYQZ8Q5w5CwKEIfR4CiSevuJ/Dm5pOLGKsCSY aI/lvS92wuWQVHS0DY9VcBEKTERhBnbFQyZn3Zt/KpG5r9XdrDeFvaUlFV0kNfAFavsK Xi2qnoRXCC8MrVeiyeho/lFRbbSqK/28EHltmmChdIfHIjvqdL038w7uFmUaDCozchV8 JKfckJynRGnmoM9T3Z3wbMJClLGJAmukI9PIE7CDMYCHugjMh5gJ7YyUxHeq1wZXkWiT YBYQ== X-Gm-Message-State: AOAM533JcEM0eL+jM/DE+LLSMwLPy9phlGX1E/spnEX1VV6WKN0VumHY vBEtdHruV2QYu2ePQoNZ1kqf6WniIe4= X-Google-Smtp-Source: ABdhPJxsmwO6+drIheVyhAjNd5u575ECwmGr4xKu+31BzKZVDNGXC9SiSWF6bfjerxa5Y5rh1CgmMg== X-Received: by 2002:a05:6214:1269:: with SMTP id r9mr26951631qvv.85.1592921800069; Tue, 23 Jun 2020 07:16:40 -0700 (PDT) Received: from [192.168.0.185] ([179.177.236.228]) by smtp.gmail.com with ESMTPSA id q38sm723494qtb.74.2020.06.23.07.16.36 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 23 Jun 2020 07:16:39 -0700 (PDT) Subject: Re: [PATCH 5/5] gdb: Fix Python unwinders and inline frames To: Andrew Burgess , gdb-patches@sourceware.org Cc: Tom Tromey References: <20200622154723.GK2737@embecosm.com> From: Luis Machado Message-ID: <687e2fa2-c04f-8245-d24a-060653d07663@linaro.org> Date: Tue, 23 Jun 2020 11:16:34 -0300 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: <20200622154723.GK2737@embecosm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-9.7 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.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, 23 Jun 2020 14:16:43 -0000 Hi Andrew, On 6/22/20 12:47 PM, Andrew Burgess wrote: > 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 Thanks for the very detailed analysis. The situation is clearer now. I gave this a try on aarch64-linux and it looks good. I think special-casing things for inline frames is a good compromise, though it looks slightly out of place on value_of-register_lazy. Thinking about it further, I was wondering if there were other cases of attempting to manipulate things on frames without having a frame id set yet. I suppose the inline frames will always have to ask a non-inline frame for register information, so we may be safe for most (if not all) cases. > > --- > > 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") >