Re: [PATCH 1/5] gdb/python: handle saving user registers in a frame unwinder

Andrew Burgess
2021-06-21T19:41:33Z

I've pushed this patch.

Thanks,
Andrew

* Andrew Burgess <andrew.burgess@embecosm.com> [2021-05-29 21:57:10 +0100]:

> This patch came about because I wanted to write a frame unwinder that
> would corrupt the backtrace in a particular way. In order to achieve
> what I wanted I ended up trying to write an unwinder like this:
>
> class FrameId(object):
> .... snip class definition ....
>
> class TestUnwinder(Unwinder):
> def __init__(self):
> Unwinder.__init__(self, "some name")
>
> def __call__(self, pending_frame):
> pc_desc = pending_frame.architecture().registers().find("pc")
> pc = pending_frame.read_register(pc_desc)
>
> sp_desc = pending_frame.architecture().registers().find("sp")
> sp = pending_frame.read_register(sp_desc)
>
> # ... snip code to decide if this unwinder applies or not.
>
> fid = FrameId(pc, sp)
> unwinder = pending_frame.create_unwind_info(fid)
> unwinder.add_saved_register(pc_desc, pc)
> unwinder.add_saved_register(sp_desc, sp)
> return unwinder
>
> The important things here are the two calls:
>
> unwinder.add_saved_register(pc_desc, pc)
> unwinder.add_saved_register(sp_desc, sp)
>
> On x86-64 these would fail with an assertion error:
>
> gdb/regcache.c:168: internal-error: int register_size(gdbarch*, int): Assertion `regnum >= 0 && regnum < gdbarch_num_cooked_regs (gdbarch)' failed.
>
> What happens is that in unwind_infopy_add_saved_register (py-unwind.c)
> we call register_size, as register_size should only be called on
> cooked (real or pseudo) registers, and 'pc' and 'sp' are implemented
> as user registers (at least on x86-64), we trigger the assertion.
>
> A simple fix would be to check in unwind_infopy_add_saved_register if
> the register number we are handling is a cooked register or not, if
> not we can throw a 'Bad register' error back to the Python code.
>
> However, I think we can do better.
>
> Consider that at the CLI we can do this:
>
> (gdb) set $pc=0x1234
>
> This works because GDB first evaluates '$pc' to get a register value,
> then evaluates '0x1234' to create a value encapsulating the
> immediate. The contents of the immediate value are then copied back
> to the location of the register value representing '$pc'.
>
> The value location for a user-register will (usually) be the location
> of the real register that was accessed, so on x86-64 we'd expect this
> to be $rip.
>
> So, in this patch I propose that in the unwinder code, when
> add_saved_register is called, if it is passed a
> user-register (i.e. non-cooked) then we first fetch the register,
> extract the real register number from the value's location, and use
> that new register number when handling the add_saved_register call.
>
> If either the value location that we get for the user-register is not
> a cooked register then we can throw a 'Bad register' error back to the
> Python code, but in most cases this will not happen.
>
> gdb/ChangeLog:
>
> * python/py-unwind.c (unwind_infopy_add_saved_register): Handle
> saving user registers.
>
> gdb/testsuite/ChangeLog:
>
> * gdb.python/py-unwind-user-regs.c: New file.
> * gdb.python/py-unwind-user-regs.exp: New file.
> * gdb.python/py-unwind-user-regs.py: New file.
> ---
> gdb/ChangeLog | 5 +
> gdb/python/py-unwind.c | 21 ++++
> gdb/testsuite/ChangeLog | 6 ++
> .../gdb.python/py-unwind-user-regs.c | 37 +++++++
> .../gdb.python/py-unwind-user-regs.exp | 98 +++++++++++++++++++
> .../gdb.python/py-unwind-user-regs.py | 72 ++++++++++++++
> 6 files changed, 239 insertions(+)
> create mode 100644 gdb/testsuite/gdb.python/py-unwind-user-regs.c
> create mode 100644 gdb/testsuite/gdb.python/py-unwind-user-regs.exp
> create mode 100644 gdb/testsuite/gdb.python/py-unwind-user-regs.py
>
> diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
> index 7c195eb539d..d6e2f85dbc1 100644
> --- a/gdb/python/py-unwind.c
> +++ b/gdb/python/py-unwind.c
> @@ -27,6 +27,7 @@
> #include "python-internal.h"
> #include "regcache.h"
> #include "valprint.h"
> +#include "user-regs.h"
>
> /* Debugging of Python unwinders. */
>
> @@ -265,6 +266,26 @@ unwind_infopy_add_saved_register (PyObject *self, PyObject *args)
> PyErr_SetString (PyExc_ValueError, "Bad register");
> return NULL;
> }
> +
> + /* If REGNUM identifies a user register then *maybe* we can convert this
> + to a real (i.e. non-user) register. The maybe qualifier is because we
> + don't know what user registers each target might add, however, the
> + following logic should work for the usual style of user registers,
> + where the read function just forwards the register read on to some
> + other register with no adjusting the value. */
> + if (regnum >= gdbarch_num_cooked_regs (pending_frame->gdbarch))
> + {
> + struct value *user_reg_value
> + = value_of_user_reg (regnum, pending_frame->frame_info);
> + if (VALUE_LVAL (user_reg_value) == lval_register)
> + regnum = VALUE_REGNUM (user_reg_value);
> + if (regnum >= gdbarch_num_cooked_regs (pending_frame->gdbarch))
> + {
> + PyErr_SetString (PyExc_ValueError, "Bad register");
> + return NULL;
> + }
> + }
> +
> {
> struct value *value;
> size_t data_size;
> diff --git a/gdb/testsuite/gdb.python/py-unwind-user-regs.c b/gdb/testsuite/gdb.python/py-unwind-user-regs.c
> new file mode 100644
> index 00000000000..8d1efd1a85d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-unwind-user-regs.c
> @@ -0,0 +1,37 @@
> +/* This test program is part of GDB, the GNU debugger.
> +
> + Copyright 2021 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;
> +
> +void __attribute__ ((noinline))
> +foo (void)
> +{
> + ++global_var; /* Break here. */
> +}
> +
> +void __attribute__ ((noinline))
> +bar (void)
> +{
> + foo ();
> +}
> +
> +int
> +main (void)
> +{
> + bar ();
> + return 0;
> +}
> diff --git a/gdb/testsuite/gdb.python/py-unwind-user-regs.exp b/gdb/testsuite/gdb.python/py-unwind-user-regs.exp
> new file mode 100644
> index 00000000000..7ae3a5bb19f
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-unwind-user-regs.exp
> @@ -0,0 +1,98 @@
> +# Copyright (C) 2021 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/>.
> +
> +# Setup an unwinder that uses gdb.UnwindInfo.add_saved_register with
> +# the register's 'pc' and 'sp'. On some (all?) targets, these
> +# registers are implemented as user-registers, and so can't normally
> +# be written to directly.
> +#
> +# The Python unwinder now includes code similar to how the expression
> +# evaluator would handle something like 'set $pc=0x1234', we fetch the
> +# value of '$pc', and then use the value's location to tell us which
> +# register to write to.
> +#
> +# The unwinder defined here deliberately breaks the unwind by setting
> +# the unwound $pc and $sp to be equal to the current frame's $pc and
> +# $sp. GDB will spot this as a loop in the backtrace and terminate
> +# the unwind.
> +#
> +# However, by the time the unwind terminates we have already shown
> +# that it is possible to call add_saved_register with a user-register,
> +# so the test is considered passed.
> +#
> +# For completeness this test checks two cases, calling
> +# add_saved_register with a gdb.RegisterDescriptor and calling
> +# add_saved_register with a string containing the register name.
> +
> +load_lib gdb-python.exp
> +
> +standard_testfile
> +
> +if { [prepare_for_testing "failed to prepare" ${testfile} ${srcfile}] } {
> + return -1
> +}
> +
> +# Skip all tests if Python scripting is not enabled.
> +if { [skip_python_tests] } { continue }
> +
> +if ![runto_main] then {
> + fail "can't run to main"
> + return 0
> +}
> +
> +set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
> +
> +gdb_breakpoint [gdb_get_line_number "Break here"]
> +gdb_continue_to_breakpoint "stop at test breakpoint"
> +
> +# Load the script containing the unwinders. There are actually two
> +# unwinders defined here that will catch the same function, so we
> +# immediately disable one of the unwinders.
> +gdb_test_no_output "source ${pyfile}"\
> + "import python scripts"
> +gdb_test "disable unwinder global \"break unwinding using strings\"" \
> + "1 unwinder disabled" "disable the unwinder that uses strings"
> +
> +# At this point we are using the unwinder that passes a
> +# gdb.RegisterDescriptor to add_saved_register.
> +gdb_test_sequence "bt" "Backtrace corrupted by descriptor based unwinder" {
> + "\\r\\n#0 \[^\r\n\]* foo \\(\\) at "
> + "\\r\\n#1 \[^\r\n\]* bar \\(\\) at "
> + "Backtrace stopped: previous frame inner to this frame \\(corrupt stack\\?\\)"
> +}
> +
> +# Disable the unwinder that calls add_saved_register with a
> +# gdb.RegisterDescriptor, and enable the unwinder that calls
> +# add_saved_register with a string (containing the register name).
> +gdb_test "disable unwinder global \"break unwinding using descriptors\"" \
> + "1 unwinder disabled" "disable the unwinder that uses descriptors"
> +gdb_test "enable unwinder global \"break unwinding using strings\"" \
> + "1 unwinder enabled" "enable the unwinder that uses strings"
> +gdb_test_sequence "bt" "Backtrace corrupted by string based unwinder" {
> + "\\r\\n#0 \[^\r\n\]* foo \\(\\) at "
> + "\\r\\n#1 \[^\r\n\]* bar \\(\\) at "
> + "Backtrace stopped: previous frame inner to this frame \\(corrupt stack\\?\\)"
> +}
> +
> +# Just for completeness, disable the string unwinder again (neither of
> +# our special unwinders are now enabled), and check the backtrace. We
> +# now get the complete stack back to main.
> +gdb_test "disable unwinder global \"break unwinding using strings\"" \
> + "1 unwinder disabled" "disable the unwinder that uses strings again"
> +gdb_test_sequence "bt" "Backtrace not corrupted when using no unwinder" {
> + "\\r\\n#0 \[^\r\n\]* foo \\(\\) at "
> + "\\r\\n#1 \[^\r\n\]* bar \\(\\) at "
> + "\\r\\n#2 \[^\r\n\]* main \\(\\) at "
> +}
> diff --git a/gdb/testsuite/gdb.python/py-unwind-user-regs.py b/gdb/testsuite/gdb.python/py-unwind-user-regs.py
> new file mode 100644
> index 00000000000..e5edd7cbd9c
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-unwind-user-regs.py
> @@ -0,0 +1,72 @@
> +# Copyright (C) 2021 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/>.
> +
> +import gdb
> +from gdb.unwinder import Unwinder
> +
> +
> +class FrameId(object):
> + def __init__(self, sp, pc):
> + self._sp = sp
> + self._pc = pc
> +
> + @property
> + def sp(self):
> + return self._sp
> +
> + @property
> + def pc(self):
> + return self._pc
> +
> +
> +class TestUnwinder(Unwinder):
> + def __init__(self, use_descriptors):
> + if use_descriptors:
> + tag = "using descriptors"
> + else:
> + tag = "using strings"
> +
> + Unwinder.__init__(self, "break unwinding %s" % tag)
> + self._use_descriptors = use_descriptors
> +
> + def __call__(self, pending_frame):
> + pc_desc = pending_frame.architecture().registers().find("pc")
> + pc = pending_frame.read_register(pc_desc)
> +
> + sp_desc = pending_frame.architecture().registers().find("sp")
> + sp = pending_frame.read_register(sp_desc)
> +
> + block = gdb.block_for_pc(int(pc))
> + if block == None:
> + return None
> + func = block.function
> + if func == None:
> + return None
> + if str(func) != "bar":
> + return None
> +
> + fid = FrameId(pc, sp)
> + unwinder = pending_frame.create_unwind_info(fid)
> + if self._use_descriptors:
> + unwinder.add_saved_register(pc_desc, pc)
> + unwinder.add_saved_register(sp_desc, sp)
> + else:
> + unwinder.add_saved_register("pc", pc)
> + unwinder.add_saved_register("sp", sp)
> + return unwinder
> +
> +
> +gdb.unwinder.register_unwinder(None, TestUnwinder(True), True)
> +gdb.unwinder.register_unwinder(None, TestUnwinder(False), True)
> --
> 2.25.4
>