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

[Andrew Burgess <andrew.burgess@embecosm.com>]

* 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")