From: Andrew Burgess Date: Mon, 8 Jun 2020 10:36:13 +0000 (+0100) Subject: gdb: Python unwinders, inline frames, and tail-call frames X-Git-Url: http://git.efficios.com/?p=deliverable%2Fbinutils-gdb.git;a=commitdiff_plain;h=9fc501fdfe5dc82b5e5388cde4ac2ab70ed69d75 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/ChangeLog b/gdb/ChangeLog index 98096bf8cd..c86d7e4647 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,13 @@ +2020-07-06 Andrew Burgess + + 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. + 2020-07-06 Andrew Burgess * NEWS: Mention additions to Python API. diff --git a/gdb/dwarf2/frame-tailcall.c b/gdb/dwarf2/frame-tailcall.c index 16dba2b201..2d219f13f9 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 c7cd31ce1a..7e9dab567f 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 ff27b9f00e..ac1016b083 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 e835d49f9c..cfc15022ed 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/ChangeLog b/gdb/testsuite/ChangeLog index 63fb5e667e..8eae1ab782 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,12 @@ +2020-07-06 Andrew Burgess + + 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. + 2020-07-06 Andrew Burgess * gdb.python/py-arch-reg-groups.exp: New file. 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 0000000000..0cfe06dd27 --- /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 0000000000..f7c65f6afc --- /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 0000000000..7206a0b283 --- /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")