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

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

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.	

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