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

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

* Luis Machado <luis.machado@linaro.org> [2020-06-18 07:29:53 -0300]:

> On 6/18/20 5:25 AM, Andrew Burgess wrote:
> > * Luis Machado <luis.machado@linaro.org> [2020-06-17 18:14:26 -0300]:
> > 
> > > On 6/17/20 2:38 PM, Andrew Burgess wrote:
> > > > I observed that when making use of a Python frame unwinder, 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 _is_ same as the frame-id for frame #1 - the frame-id for
> > > > frame #0 requires us to first compute the frame-id for frame #1.  As a
> > > > result it is not same to call `value_of_register_lazy' before the
> > > > frame-id of the current frame is known.
> > > > 
> > > > The solution I propose here is that `value_of_register' stops making a
> > > > lazy register value (the step that requires the frame-id, and thus
> > > > causes the problems), and instead calls `get_frame_register_value'.
> > > > This avoids the need to calculate the frame-id and so avoids the
> > > > problem.
> > > 
> > > I believe this situation is similar to the one I was investigating a while
> > > ago with the tailcall/inline frame breakage, and it is a bit messy.
> > > 
> > > The values returned from get_frame_register* functions (what you call now)
> > > and frame_unwind_register* functions (I believe this is what
> > > value_of_register_lazy ends up calling) may not be the same in all
> > > situations.
> > 
> > I find that hard to believe given that:
> > 
> >    struct value *
> >    get_frame_register_value (struct frame_info *frame, int regnum)
> >    {
> >      return frame_unwind_register_value (frame->next, regnum);
> >    }
> > 
> > Previously, a call to value_of_register would result in:
> > 
> > * value_of_register - creates a lazy register value with a call to
> >    value_of_register_lazy, and then immediately resolves it with a call
> >    to value_fetch_lazy.
> > 
> > * value_fetch_lazy would call value_fetch_lazy_register, which would
> >    then call frame_unwind_register_value to unwind the register value.
> > 
> > Now we have:
> > 
> > * value_of_register calls get_frame_register_value.
> > 
> > * get_frame_register_value calls frame_unwind_register_value.
> 
> Looking at the code again, I agree that they do converge. The difference is
> mostly that get_frame_register* starts right off the bat by passing the next
> frame id, whereas value_of_register gets this from value_of_register_lazy.
> So you're dealing with unwinding from the next frame.
> 
> Take, for example, default_unwind_pc () (it calls frame_unwind_register* for
> the current frame), which is what I was dealing with at the time. It will
> try to unwind PC from the current frame first. Only then GDB will ask the
> next frame for PC. So there is a subtle difference compared to
> get_frame_register*, which calls frame_unwind_register for the next frame.
> 
> The regressions commit 5939967b355ba6a940887d19847b7893a4506067 created
> (defaulted to get_frame_register) for x86_64 were fixed by
> 991a3e2e9944a4b3a27bd989ac03c18285bd545d (added the conditional block).
> 
> It may be the case that value_of_register has always fetched things from the
> next frame, without considering the current frame's idea of what the
> register value is. We may not be seeing failures because we are not
> exercising this situation.

Just in case you'd not spotted I followed up to the thread for your
original patch 5939967b355ba6a940887d19847b7893a4506067.

Thanks,
Andrew


> 
> In any case, I just thought I'd mention this, as it is a bit of a trap when
> you're dealing with the unwinding code.
> 
> > 
> > The two code paths have converged.
> > 
> > Additionally I don't see anywhere in value_fetch_lazy, or
> > value_fetch_lazy_register that GDB could muck around with the value
> > returned such that it might give us back a different value.
> > 
> > BUT, there is one difference.  Calling frame_unwind_register_value
> > might itself return a lazy value (for example a lazy memory value for
> > a register spilled to the stack), value_fetch_lazy ensures that these
> > are resolved.
> > 
> > This makes sense, after calling value_fetch_lazy I really should have
> > a non-lazy value.  However, I believe it should be fine if
> > value_of_register returns a lazy value.  That it didn't before does
> > worry me _slightly_, but until I see a good reason why it _must_
> > return a non-lazy value I'm inclined to take the position that
> > returning a lazy value is fine, and anyone who calls that function
> > should only be using the value API to get the value contents, and
> > anyone who does that will automatically resolve the lazy value at that
> > point.
> > 
> > > 
> > > The difference comes from the fact that some targets have enough CFI
> > > information to compute a register's value according to DWARF rules (which
> > > frame_unwind_register* functions use), without requiring information from
> > > the previous frame.
> > > 
> > > Some other targets just assume "SAME_VALUE" and end up fetching the register
> > > from the previous frame, which requires a valid frame_id. In this last case,
> > > calling get_frame_register* will yield the same result, since it will fetch
> > > the data from the previous frame anyway.
> > > 
> > > I'd pay special attention to x86_64, which is one I noticed has enough CFI
> > > information in some cases, whereas AArch64 doesn't.
> > > 
> > > In summary, the functions are not equivalent and don't take the same paths
> > > all the time. The documentation is not too clear unfortunately, so there's
> > > room for falling into traps. :-)
> > 
> > I'll try to track down the thread your talking about.  It sounds
> > ... concerning ... that two APIs to read registers from the same frame
> > should return different values.  You kind of touched on _how_ that
> > might happen, but I don't feel I really understand _why_, so I'd
> > certainly be interested to learn more.
> > 
> > 
> > Thanks,
> > Andrew
> > 
> > > 
> > > An alternative solution would be to only call get_frame_register_value if
> > > the frame id is not yet computed, which is certain to drive us into an
> > > assertion in that case.
> > > 
> > > > 
> > > > This bug has crept in because we allowed calls to the function
> > > > `value_of_register_lazy' at a time when the frame-id of the frame
> > > > passed to the function didn't have its frame-id calculated.  This is
> > > > only ever a problem for inline frames.  To prevent bugs like this
> > > > getting into GDB in the future I've added an assert to the function
> > > > `value_of_register_lazy' to require the current frame have its id
> > > > calculated.
> > > > 
> > > > If we are calling from outside of the frame sniffer then this will
> > > > _always_ be true, so is not a problem.  If we are calling this
> > > > function from within a frame sniffer then it will always trigger.
> > > > 
> > > > gdb/ChangeLog:
> > > > 
> > > > 	* findvar.c (value_of_register): Use get_frame_register_value
> > > > 	rather than value_of_register_lazy.
> > > > 	(value_of_register_lazy): Add new assert, and update comments.
> > > > 
> > > > gdb/testsuite/ChangeLog:
> > > > 
> > > > 	* gdb.python/py-unwind-inline.c: New file.
> > > > 	* gdb.python/py-unwind-inline.exp: New file.
> > > > 	* gdb.python/py-unwind-inline.py: New file.
> > > > ---
> > > >    gdb/ChangeLog                                 |  6 ++
> > > >    gdb/findvar.c                                 | 28 ++++++--
> > > >    gdb/testsuite/ChangeLog                       |  6 ++
> > > >    gdb/testsuite/gdb.python/py-unwind-inline.c   | 37 ++++++++++
> > > >    gdb/testsuite/gdb.python/py-unwind-inline.exp | 49 +++++++++++++
> > > >    gdb/testsuite/gdb.python/py-unwind-inline.py  | 71 +++++++++++++++++++
> > > >    6 files changed, 192 insertions(+), 5 deletions(-)
> > > >    create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.c
> > > >    create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.exp
> > > >    create mode 100644 gdb/testsuite/gdb.python/py-unwind-inline.py
> > > > 
> > > > diff --git a/gdb/findvar.c b/gdb/findvar.c
> > > > index c7cd31ce1a6..e5445b34930 100644
> > > > --- a/gdb/findvar.c
> > > > +++ b/gdb/findvar.c
> > > > @@ -263,16 +263,13 @@ struct value *
> > > >    value_of_register (int regnum, struct frame_info *frame)
> > > >    {
> > > >      struct gdbarch *gdbarch = get_frame_arch (frame);
> > > > -  struct value *reg_val;
> > > >      /* User registers lie completely outside of the range of normal
> > > >         registers.  Catch them early so that the target never sees them.  */
> > > >      if (regnum >= gdbarch_num_cooked_regs (gdbarch))
> > > >        return value_of_user_reg (regnum, frame);
> > > > -  reg_val = value_of_register_lazy (frame, regnum);
> > > > -  value_fetch_lazy (reg_val);
> > > > -  return reg_val;
> > > > +  return get_frame_register_value (frame, regnum);
> > > >    }
> > > >    /* Return a `value' with the contents of (virtual or cooked) register
> > > > @@ -292,7 +289,28 @@ value_of_register_lazy (struct frame_info *frame, int regnum)
> > > >      next_frame = get_next_frame_sentinel_okay (frame);
> > > > -  /* We should have a valid next frame.  */
> > > > +  /* If NEXT_FRAME is inline within FRAME, then calculating the frame-id
> > > > +     for the next frame will require that FRAME has a valid frame-id.
> > > > +     Usually this is not a problem, however, if this function is ever
> > > > +     called from a frame sniffer, the small window of time when not all
> > > > +     frames have yet had their frame-id calculated, this function will
> > > > +     trigger an assert within get_frame_id that a frame at a level > 0
> > > > +     doesn't have a frame id.
> > > > +
> > > > +     Instead of waiting for an inline frame to reveal an invalid use of
> > > > +     this function, we instead demand that FRAME must have had its frame-id
> > > > +     calculated before we use this function.
> > > > +
> > > > +     The one time it is safe to call this function when FRAME does not yet
> > > > +     have a frame id is when FRAME is at level 0, in this case the
> > > > +     assertion in get_frame_id doesn't fire, and instead get_frame_id will
> > > > +     correctly compute the frame id for us.  */
> > > > +  gdb_assert (frame_relative_level (frame) == 0
> > > > +	      || frame_id_computed_p (frame));
> > > > +
> > > > +  /* We should have a valid next frame too.  Given that FRAME is more
> > > > +     outer than NEXT_FRAME, and we know that FRAME has its ID calculated,
> > > > +     then this must always be true.  */
> > > >      gdb_assert (frame_id_p (get_frame_id (next_frame)));
> > > >      reg_val = allocate_value_lazy (register_type (gdbarch, regnum));
> > > > 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")
> > > >