[PATCH 5/5] gdb: Fix Python unwinders and inline frames
Andrew Burgess
andrew.burgess@embecosm.com
Thu Jul 2 21:07:03 GMT 2020
Ping!
Unless anyone has any feedback I plan to push this series in the next
couple of days.
Thanks,
Andrew
* Andrew Burgess <andrew.burgess@embecosm.com> [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 <andrew.burgess@embecosm.com>
> 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 <http://www.gnu.org/licenses/>. */
> +
> +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 <http://www.gnu.org/licenses/>.
> +
> +# 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 <http://www.gnu.org/licenses/>.
> +
> +# 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")
More information about the Gdb-patches
mailing list