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

[Andrew Burgess <aburgess@redhat.com>]

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.

>
> 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?

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.

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

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.

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

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

> +        """
> +        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?


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

> +    {
> +      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?

Thanks,
Andrew

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