Re: [RFC PATCH v2] gdb: Add python support for demangling register unwind values

[Craig Blackmore <craig.blackmore@embecosm.com>]

Hi Andrew,

Thanks for the review and apologies for the very slow reply as it took 
longer to get back to this than I'd hoped.

I've responded inline with how I've addressed your comments and I will 
post a v3 patch shortly.

On 12/05/2025 14:15, Andrew Burgess wrote:
> Craig Blackmore <craig.blackmore@embecosm.com> writes:
>
>> This patch addresses the scenario where a register value is saved in
>> some mangled form on the target and gdb does not have sufficient
>> information to demangle it.
> I'm assuming this is some kind of security related thing, e.g. return
> address is hashed or "encrypted" in some way?  Some of my thoughts below
> are based on this assumption.
Yes it's security related.
>> After getting a register value from an unwinder, call out to extension
>> languages to allow the value to be demangled.  This avoids having to
>> write a new unwinder and allows the standard unwinders to be used.
>>
>> This approach is purely for demangling values that have been read, there
>> is no similar support provided for mangling values being written back to
>> the target as this was not something that I needed to be able to do.
> OK, but, presumably, if you did try to modify one of these demangled
> registers, and then resumed the inferior, things would not continue as
> expected?
Attempting to modify a demangled register results in an "Attempt to 
assign to an unmodifiable value" error which is checked for in 
py-unwind-demangler.exp.
> If this is true, and right now you don't need to write the value back.
> Would it not be worth designing an API now that _could_ support this
> functionality?
>
> I have no problem, in theory, with having this functionality in the API,
> the problem is, once things are added to the Python API, we're usually
> pretty conservative about changing them, so we can only add new
> features.
>
> Now, sure, if, in the future, we want to support writing registers, we
> can add a whole new, parallel, API that supports the writing case.  But
> wouldn't it be nice if we spent a little time thinking, up front, what
> that writing case might look like, even if it's not implemented?  Maybe
> we can just document in this commit message what that support would look
> like, and how it would fit into the existing API.
I've changed `gdb.unwinder.UnwindRegisterValueDemangler` to 
`gdb.unwinder.UnwindRegisterValueTransformer` which is a base class for 
transformers that provide methods for transforming unwind register 
values.  Instead of calling `__call__` to demangle we now call 
`demangle`.  In future, other transform methods such as `mangle` could 
be added.
>> Only one demangler can be registered at any one time.  In future, the
>> implementation could be extended to support multiple demanglers executed
>> in order of priority.
> Again, have you thought about how this might look?  For the same reasons
> as before, while for changes to GDB's C++ it's fine to do "just enough",
> the Python API has a nasty habit of becoming locked in.
>
> I notice, for example, that register demanglers are created and
> registered by creating a UnwindRegisterValueDemangler instance.  In
> contrast, instruct, many other similar functionality is done via a
> gdb.register_* or gdb.*.register_ function which take a locus that
> allows handlers to be attached to specific objfiles or program spaces.
>
> I think it might be a good idea if you switched to using this same
> approach.  Currently, these register functions accept a locus of `None`
> to indicate that this should be a global registration.  You could, for
> example, throw an error if a locus other than `None` is used.  But this
> would then leave the door open, in future, to support other locus, which
> would allow per-program-space registrations for example.
I have changed the registration to be similar to python unwinder 
registration so that registering multiple transformers and registering 
to other loci can be supported in future.
> I also wonder if you should be more inline with the existing register
> functions when it comes to supporting multiple handlers, while at the
> same time, not actually changing what you support?  For example, other
> register functions take a `replace` flag which controls whether existing
> handlers are replaced.  You could just support replacing a handler with
> the same name, or adding a single handler, anything else: raise an
> error.
Whilst changing the registration approach, I added support for the 
`replace` flag with the behaviour you suggested.
> This would be a little more work up front, but not significantly, and in
> the future.  The benefit is the API is consistent, and future proof.
> I'm not sure how you'd extend the existing API to handle
> per-program-space registrations, which I think should be a minimum
> requirement.
>
>> The demangler is registered globally.  In future, the implementation
>> could be extended to register per program space and object file too.
> See above.  Or maybe outline how you'd see that future design being.
See above.
>> gdbpy_get_register_descriptor is now externally visible as it is used
>> to create a gdb.RegisterDescriptor object to pass to the demangler.
>>
>> This patch adds gdb.Value.is_lval_{register,memory} variables so that
>> the demangler can choose to skip modifying values that did not come
>> directly from the target and may not need demangling, for example,
>> values taken directly from DWARF.
> I do wonder if this is the right approach for dealing with lval types.
> If we ever want to handle different lval types in the future we're now
> stuck adding yet more is_lval_* methods.
>
> Wouldn't a better approach be to add a read-only lval type attribute?  I
> don't think you'd even need to support all the types necessarily, you
> could add constants for maybe just: not_lval, lval_memory,
> lval_register, and map anything not caught be the above to not_lval, and
> document that other lval types might be added in the future.  Though I'm
> happy to be convinced that separate flags is right way to go...
I agree that's a better approach. I've made this change.
>> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
>> index 7bb650347f7..7fd0ba4f83b 100644
>> --- a/gdb/doc/python.texi
>> +++ b/gdb/doc/python.texi
>> @@ -938,6 +938,16 @@ fetched when the value is needed, or when the @code{fetch_lazy}
>>   method is invoked.
>>   @end defvar
>>   
>> +@defvar Value.is_lval_register
>> +The value of this read-only boolean attribute is @code{True} if this
>> +@code{gdb.Value} is from a register on the inferior.
>> +@end defvar
>> +
>> +@defvar Value.is_lval_memory
>> +The value of this read-only boolean attribute is @code{True} if this
>> +@code{gdb.Value} is from inferior memory.
>> +@end defvar
>> +
>>   @defvar Value.bytes
>>   The value of this attribute is a @code{bytes} object containing the
>>   bytes that make up this @code{Value}'s complete value in little endian
>> @@ -3160,6 +3170,44 @@ the matching unwinders are enabled.  The @code{enabled} field of each
>>   matching unwinder is set to @code{True}.
>>   @end table
>>   
>> +@subheading Unwind Register Value Demangler
>> +
>> +After getting a register value from an unwinder, @value{GDBN} will call
>> +out to extension language demanglers to allow them to modify the value.
>> +This is useful in case a register value needs demangling before
>> +@value{GDBN} uses it and avoids the need to write a new unwinder.
>> +
>> +Currently, only one demangler can be registered at any one time and it
>> +is registered globally.
>> +
>> +@subheading Skeleton Code for a Register Value Demangler
>> +
>> +Here is an example of how to structure a user created demangler:
>> +
>> +@smallexample
>> +from gdb.unwinder import UnwindRegisterValueDemangler
>> +
>> +class MyUnwindRegisterValueDemangler(UnwindRegisterValueDemangler):
>> +    def __init__(self):
>> +        # Register self as the one and only demangler
>> +        super().__init__("MyUnwinder_Name")
>> +
>> +    def __call__(self, frame_type, reg, value):
> I'll add this comment here just because: I think it is a mistake to pass
> through the `frame_type` here.  I think you'd be better off passing the
> gdb.Frame object itself, then let the demangled grab the frame type if
> that's what it needs.  This is a pretty small change, but gives the API
> greater flexibility for the future.
Done.
>> +        """
>> +        Return new value if demangled, otherwise None.
>> +        """
>> +        if frame_type != <frame we need to handle>:
>> +          return None
>> +
>> +        if reg.name == <reg that needs demangling>:
>> +          new_value = ... compute demangled value ...
>> +          return new_value
>> +
>> +        return None
>> +
>> +my_demangler = MyUnwindRegisterValueDemangler()
>> +@end smallexample
>> +
>>   @node Xmethods In Python
>>   @subsubsection Xmethods In Python
>>   @cindex xmethods in Python
>
>> diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
>> index d43d7e97d99..6ec239664f5 100644
>> --- a/gdb/python/py-unwind.c
>> +++ b/gdb/python/py-unwind.c
>> @@ -30,6 +30,7 @@
>>   #include "stack.h"
>>   #include "charset.h"
>>   #include "block.h"
>> +#include "value.h"
>>   
>>   
>>   /* Debugging of Python unwinders.  */
>> @@ -1181,3 +1182,82 @@ PyTypeObject unwind_info_object_type =
>>     0,                              /* tp_init */
>>     0,                              /* tp_alloc */
>>   };
>> +
>> +/* Call demangler and handle the result.  */
>> +
>> +enum ext_lang_rc
>> +gdbpy_demangle_unwind_register_value (frame_info_ptr frame, int regnum,
>> +				      struct value **val)
>> +{
>> +  if (!gdb_python_initialized)
>> +    return EXT_LANG_RC_ERROR;
> If you checkout gdbpy_print_insn you'll notice that it uses this:
>
>    if (!gdb_python_initialized || !python_print_insn_enabled)
>      return {};
>
> The `python_print_insn_enabled` is a C++ global that is set from Python
> code when any handlers are registered.
>
> The point here is that for the common case, where no handler is
> registered, GDB doesn't need to enter the Python environment, or create,
> access, or setup, any Python state at all.
>
> This is a pretty easy thing to add, but my thinking at the time, is that
> this will give a small improvement (of no Python work) for the common
> case.  Do you think you could look at how hard it would be to do
> something similar in this case too please?
Done. I've followed a similar approach to how 
`python_print_insn_enabled` is set / used.
>
>> +
>> +  gdbarch *gdbarch = get_frame_arch (frame);
>> +  gdbpy_enter enter_py (gdbarch);
>> +
>> +  gdbpy_ref<> py_frame_type
>> +    = gdb_py_object_from_longest (get_frame_type (frame));
>> +
>> +  gdbpy_ref<> py_reg_obj (gdbpy_get_register_descriptor (gdbarch, regnum));
>> +  if (py_reg_obj == NULL)
> Remember s/NULL/nullptr/ throughout this patch.
Done.
>> +    {
>> +      gdbpy_print_stack ();
>> +      return EXT_LANG_RC_ERROR;
>> +    }
>> +
>> +  gdbpy_ref<> py_value_obj (value_to_value_object (*val));
>> +  if (py_value_obj == NULL)
>> +    {
>> +      gdbpy_print_stack ();
>> +      return EXT_LANG_RC_ERROR;
>> +    }
>> +
>> +  /* Call demangler.  */
>> +  const char *func_name = "_execute_unwind_register_value_demangler";
>> +  if (gdb_python_module == NULL
>> +      || ! PyObject_HasAttrString (gdb_python_module, func_name))
>> +    {
>> +      PyErr_SetString (PyExc_NameError,
>> +	"Installation error: gdb._execute_unwind_register_value_demangler "
>> +	"function is missing");
>> +      gdbpy_print_stack ();
>> +      return EXT_LANG_RC_ERROR;
>> +    }
>> +  gdbpy_ref<> pyo_execute (
>> +    PyObject_GetAttrString (gdb_python_module, func_name));
>> +  if (pyo_execute == nullptr)
>> +    {
>> +      gdbpy_print_stack ();
>> +      return EXT_LANG_RC_ERROR;
>> +    }
>> +
>> +  /* Demangler should return a gdb.Value if value has been changed, otherwise
>> +     None.  */
>> +  gdbpy_ref<> pyo_execute_ret
>> +    (PyObject_CallFunctionObjArgs (pyo_execute.get (), py_frame_type.get (),
>> +				   py_reg_obj.get (), py_value_obj.get (),
>> +				   NULL));
>> +  if (pyo_execute_ret == nullptr)
>> +    {
>> +      gdbpy_print_stack ();
>> +      return EXT_LANG_RC_ERROR;
>> +    }
>> +  /* Use original value.  */
>> +  if (pyo_execute_ret == Py_None)
>> +    return EXT_LANG_RC_NOP;
>> +
>> +  /* Verify the return value is a gdb.Value.  */
>> +  struct value *new_val = convert_value_from_python (pyo_execute_ret.get ());
>> +
>> +  if (new_val == nullptr)
>> +    {
>> +      gdbpy_print_stack ();
>> +      error (_("An unwind register value demangler should return a gdb.Value "
>> +	       "instance if the value is changed, otherwise None"));
>> +      return EXT_LANG_RC_ERROR;
> There's no need for a return after a call to error().  I also notice
> that this error message doesn't show up in the testsuite, so it would
> appear that this case isn't being tested maybe?

I've removed the return and added a testcase.

Thanks,

Craig

> Thanks,
> Andrew
>
>> +    }
>> +  /* Finally update val to the demangled value.  */
>> +  *val = new_val;
>> +
>> +  return EXT_LANG_RC_OK;
>> +}