Re: [PATCH 5/5] gdb: Fix Python unwinders and inline frames

Andrew Burgess
2020-07-06T17:43:07Z

* Andrew Burgess <andrew.burgess@embecosm.com> [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 <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")