From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x334.google.com (mail-wm1-x334.google.com [IPv6:2a00:1450:4864:20::334]) by sourceware.org (Postfix) with ESMTPS id 0E21C3858D35 for ; Mon, 6 Jul 2020 17:43:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 0E21C3858D35 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-x334.google.com with SMTP id 17so42948452wmo.1 for ; Mon, 06 Jul 2020 10:43:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=EgtkpkOp3IJ/ZyS01uWqNpVIt3Ooo5OxthrTjsZQdZw=; b=JxXAFyuUga+pitqIEd10NzL/OJguoIR36t3Fm2MGlx/mPotI19ZXA6fkXKKiZnLpdM VkVVhLwC72zXo2Jo81/DQmOpC5DCq0FSQWu8ray1mC1Hb5UbGT0BlyOxU0qqLo6hSuGZ xNyLxwKWZuZNY125bv5U81hiyzxg0Vbu2Zy3kRJT05OFdKaVpBPWxl/FQuGHCobtXcN7 0qxrCNsO7xAMECtEyruWlTD62jc6EyXytrP71r7lu+g+UcNtcmMZZBlljeQALYwOmz6K F/5jwSVHa+Jf0KNosQ6B/CR6ONEyXVxL/gTOOWMn/SXxBl2Y5ubk/QFcPXXZzAFT16a2 2HUw== 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:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=EgtkpkOp3IJ/ZyS01uWqNpVIt3Ooo5OxthrTjsZQdZw=; b=s6plDtxglAsAJZFfZsgRBhsQ8LwgB8lXVgpQ5k3WqbJTi0tGbWCFm6durts/ge+TLK CuVy2eFMTFBIJve45f2nfH1ViCTUC2uQ73eQzOu75Kqp05b8rtgnmZ8bcg3NzmJIjjZq YeOzORWTZVVeU89PkPNxWBUjx8eq5VgK2aBENt3EdANssYq0Mq/m8YvHcHWIsrvJS2D/ AGMnoFCxXL4VXfjnrY6Avxv7pRNX8EGzVDfaQRJWd9vn1C+dI+AiLmINeVYqVI9zy+wZ 9lriGrAmWNcl+YOkD3f7RVQYWUar45QQq0P3cqXHOks/8FGOVsbX6BuS02uV0kjQUM09 ptQQ== X-Gm-Message-State: AOAM532OpG0t9qwjlt4n+XoyiRC049Ao6Emu/+PYVB9A+6/9bnOVLKCb qm7xhxqiy6tQj+Gpt5z9hisizan1leE= X-Google-Smtp-Source: ABdhPJwEZUy6PwOJJ6L6kN3GGYdShe3rWdCbjPm1hdFU1Fo97HTDdzKvUCgxKJyUxcySKT1mkgzbpQ== X-Received: by 2002:a05:600c:294a:: with SMTP id n10mr288100wmd.38.1594057385473; Mon, 06 Jul 2020 10:43:05 -0700 (PDT) Received: from localhost (host109-148-134-178.range109-148.btcentralplus.com. [109.148.134.178]) by smtp.gmail.com with ESMTPSA id a22sm233062wmj.9.2020.07.06.10.43.04 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Jul 2020 10:43:04 -0700 (PDT) Date: Mon, 6 Jul 2020 18:43:03 +0100 From: Andrew Burgess To: gdb-patches@sourceware.org Subject: Re: [PATCH 5/5] gdb: Fix Python unwinders and inline frames Message-ID: <20200706174303.GY2737@embecosm.com> References: <20200622154723.GK2737@embecosm.com> <20200702210703.GT2737@embecosm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200702210703.GT2737@embecosm.com> X-Operating-System: Linux/5.6.15-200.fc31.x86_64 (x86_64) X-Uptime: 18:42:43 up 28 days, 7:49, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-9.9 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: Mon, 06 Jul 2020 17:43:09 -0000 * Andrew Burgess [2020-07-02 22:07:03 +0100]: > Ping! > > Unless anyone has any feedback I plan to push this series in the next > couple of days. I've now pushed this series. Thanks, Andrew > > 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")