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

Message ID c8d1850c5fc83e02acf404c71ea6d28940489fec.1622321523.git.andrew.burgess@embecosm.com
State New
Headers show
Series
  • Fix for an assertion when unwinding with inline frames
Related show

Commit Message

Andrew Burgess May 29, 2021, 8:57 p.m.
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

-- 
2.25.4

Comments

Tom Tromey June 7, 2021, 2:50 p.m. | #1
>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:


Andrew> So, in this patch I propose that in the unwinder code, when
Andrew> add_saved_register is called, if it is passed a
Andrew> user-register (i.e. non-cooked) then we first fetch the register,
Andrew> extract the real register number from the value's location, and use
Andrew> that new register number when handling the add_saved_register call.

Andrew> If either the value location that we get for the user-register is not
Andrew> a cooked register then we can throw a 'Bad register' error back to the
Andrew> Python code, but in most cases this will not happen.

I was worried that requesting this would require unwinding the register,
which is what is currently being done.  But I suppose the idea is that
the value is normally lazy, so it isn't actually unwound?

Tom
Andrew Burgess June 7, 2021, 4:10 p.m. | #2
* Tom Tromey <tom@tromey.com> [2021-06-07 08:50:56 -0600]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

> 

> Andrew> So, in this patch I propose that in the unwinder code, when

> Andrew> add_saved_register is called, if it is passed a

> Andrew> user-register (i.e. non-cooked) then we first fetch the register,

> Andrew> extract the real register number from the value's location, and use

> Andrew> that new register number when handling the add_saved_register call.

> 

> Andrew> If either the value location that we get for the user-register is not

> Andrew> a cooked register then we can throw a 'Bad register' error back to the

> Andrew> Python code, but in most cases this will not happen.

> 

> I was worried that requesting this would require unwinding the register,

> which is what is currently being done.  But I suppose the idea is that

> the value is normally lazy, so it isn't actually unwound?


You are correct, accessing any register from an unwinder is going to
give us a lazy register value, which we'd expect (in this case) to
have the real register number for $pc, and the frame-id of the next
frame.

When you open with "I was worried that requesting this would require
unwinding the register...", was our concern just that this unwinding
might be expensive?  Or are you worried that trying to do that might
break in some way?

Remember, the user can already do:

  PendingFrame.read_register('pc')

which is going to fully fetch the register value (so create the same
lazy register value as above, and then make it unlazy), so I don't
think there should be any technical problems here, even if it turns
out that the register value is not lazy.

I guess what I'm saying is, that yes it will be lazy, but I hadn't
really considered that an important part of the system.

Thanks,
Andrew
Philippe Waroquiers via Gdb-patches June 7, 2021, 5:07 p.m. | #3
Hi,

I just have a minor stylistic remark in the python code in the test:

> […]

> +

> +    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:


When looking for None, it is usually prefered to use 'is None' instead
of '== None'.  The result is the same unless there is a strange overload
of __eq__.

This pattern can also be seen in patch 3 and 4 of your series (patch 4
using both '==' and 'is' to check for None).

Lancelot.

> +            return None

> +        func = block.function

> +        if func == None:

> +            return None
Philippe Waroquiers via Gdb-patches June 7, 2021, 5:20 p.m. | #4
On 2021-06-07 1:07 p.m., Lancelot SIX via Gdb-patches wrote:
> Hi,

> 

> I just have a minor stylistic remark in the python code in the test:

> 

>> […]

>> +

>> +    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:

> 

> When looking for None, it is usually prefered to use 'is None' instead

> of '== None'.  The result is the same unless there is a strange overload

> of __eq__.

> 

> This pattern can also be seen in patch 3 and 4 of your series (patch 4

> using both '==' and 'is' to check for None).


I agree, that's the convention in Python.  It is not in our coding
standards, but I suggest using flake8 to check the Python code, it
reports this (and much more):


    $ flake8 testsuite/gdb.python/py-unwind-user-regs.py
    testsuite/gdb.python/py-unwind-user-regs.py:52:18: E711 comparison to None should be 'if cond is None:'
    testsuite/gdb.python/py-unwind-user-regs.py:55:17: E711 comparison to None should be 'if cond is None:'

Simon
Philippe Waroquiers via Gdb-patches June 7, 2021, 6:01 p.m. | #5
On Mon, Jun 07, 2021 at 01:20:33PM -0400, Simon Marchi wrote:
> On 2021-06-07 1:07 p.m., Lancelot SIX via Gdb-patches wrote:

> > Hi,

> > 

> > I just have a minor stylistic remark in the python code in the test:

> > 

> >> […]

> >> +

> >> +    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:

> > 

> > When looking for None, it is usually prefered to use 'is None' instead

> > of '== None'.  The result is the same unless there is a strange overload

> > of __eq__.

> > 

> > This pattern can also be seen in patch 3 and 4 of your series (patch 4

> > using both '==' and 'is' to check for None).

> 

> I agree, that's the convention in Python.  It is not in our coding

> standards, but I suggest using flake8 to check the Python code, it

> reports this (and much more):


Hi,

Actually, this is mentioned in the PEP-8[1][2], which states in the
“Programming Recommandations” section:

    Comparisons to singletons like None should always be done with is or
		is not, never the equality operators.

This leads me to an annex question. Given that I still lack a lot of
experience with the overall codebase, I tend to pick this kind of small
stylistic details more easily than design and logic problems.  I do not
always point out those I see when I read the ML, but I can totally
understand those isolated stylistic comments can be considered as noise.
If so, please let me know!

> 

> 

>     $ flake8 testsuite/gdb.python/py-unwind-user-regs.py

>     testsuite/gdb.python/py-unwind-user-regs.py:52:18: E711 comparison to None should be 'if cond is None:'

>     testsuite/gdb.python/py-unwind-user-regs.py:55:17: E711 comparison to None should be 'if cond is None:'


I am currently running the testsuite against a patch that fixes those I
found. I’ll try to post it later tonight.

Lancelot.

> 

> Simon


[1] https://www.python.org/dev/peps/pep-0008/#programming-recommendations
[2] https://sourceware.org/gdb/wiki/Internals%20GDB-Python-Coding-Standards

-- 
Lancelot SIX
Philippe Waroquiers via Gdb-patches June 7, 2021, 6:09 p.m. | #6
> Hi,

> 

> Actually, this is mentioned in the PEP-8[1][2], which states in the

> “Programming Recommandations” section:

> 

>     Comparisons to singletons like None should always be done with is or

> 		is not, never the equality operators.

> 

> This leads me to an annex question. Given that I still lack a lot of

> experience with the overall codebase, I tend to pick this kind of small

> stylistic details more easily than design and logic problems.  I do not

> always point out those I see when I read the ML, but I can totally

> understand those isolated stylistic comments can be considered as noise.

> If so, please let me know!


I think it's perfectly OK.  That's how you begin and then you grow from
there, as your understanding of how things interact in the code base
grows.  I am pretty sure the first patches I reviewed were pointing out
small and easy things.

And even if someone has more experience in GDB, they can still learn
from what you mentioned above.

>>     $ flake8 testsuite/gdb.python/py-unwind-user-regs.py

>>     testsuite/gdb.python/py-unwind-user-regs.py:52:18: E711 comparison to None should be 'if cond is None:'

>>     testsuite/gdb.python/py-unwind-user-regs.py:55:17: E711 comparison to None should be 'if cond is None:'

> 

> I am currently running the testsuite against a patch that fixes those I

> found. I’ll try to post it later tonight.


Great, thanks!

Simon
Andrew Burgess June 7, 2021, 8:12 p.m. | #7
* Lancelot SIX <lsix@lancelotsix.com> [2021-06-07 19:01:31 +0100]:

> On Mon, Jun 07, 2021 at 01:20:33PM -0400, Simon Marchi wrote:

> > On 2021-06-07 1:07 p.m., Lancelot SIX via Gdb-patches wrote:

> > > Hi,

> > > 

> > > I just have a minor stylistic remark in the python code in the test:

> > > 

> > >> […]

> > >> +

> > >> +    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:

> > > 

> > > When looking for None, it is usually prefered to use 'is None' instead

> > > of '== None'.  The result is the same unless there is a strange overload

> > > of __eq__.

> > > 

> > > This pattern can also be seen in patch 3 and 4 of your series (patch 4

> > > using both '==' and 'is' to check for None).

> > 

> > I agree, that's the convention in Python.  It is not in our coding

> > standards, but I suggest using flake8 to check the Python code, it

> > reports this (and much more):

> 

> Hi,

> 

> Actually, this is mentioned in the PEP-8[1][2], which states in the

> “Programming Recommandations” section:

> 

>     Comparisons to singletons like None should always be done with is or

> 		is not, never the equality operators.

> 

> This leads me to an annex question. Given that I still lack a lot of

> experience with the overall codebase, I tend to pick this kind of small

> stylistic details more easily than design and logic problems.  I do not

> always point out those I see when I read the ML, but I can totally

> understand those isolated stylistic comments can be considered as noise.

> If so, please let me know!


I agree with Simon that any constructive feedback is great.

And specifically, thanks for pointing this issue out to me.

I've updated all of the patches locally to use 'is None' now.

Thanks again,
Andrew
Tom Tromey June 7, 2021, 8:38 p.m. | #8
>> I was worried that requesting this would require unwinding the register,

>> which is what is currently being done.  But I suppose the idea is that

>> the value is normally lazy, so it isn't actually unwound?


Andrew> You are correct, accessing any register from an unwinder is going to
Andrew> give us a lazy register value, which we'd expect (in this case) to
Andrew> have the real register number for $pc, and the frame-id of the next
Andrew> frame.

Andrew> When you open with "I was worried that requesting this would require
Andrew> unwinding the register...", was our concern just that this unwinding
Andrew> might be expensive?  Or are you worried that trying to do that might
Andrew> break in some way?

I was concerned that it would break in some way, by trying to unwind a
register while in the process of unwinding.

If it works, and is reasonably robust, then I'm fine.

Andrew> I guess what I'm saying is, that yes it will be lazy, but I hadn't
Andrew> really considered that an important part of the system.

I think I see.  Thanks.

Tom
Andrew Burgess June 21, 2021, 7:41 p.m. | #9
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

>

Patch

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)