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

[Luis Machado <luis.machado@linaro.org>]

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.

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. :-)

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