[PATCH 0/2] Allow gdb.RegisterDescriptors to be used in read_register calls

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/2] Allow gdb.RegisterDescriptors to be used in read_register calls

Andrew Burgess
Now that we have Python RegisterDescriptor objects available we can
improve an ugly dependency on GDBs internal register numbering that is
currently exposed in the Python API.  See patch #2 for more details.

Thanks,
Andrew

---

Andrew Burgess (2):
  gdb: Add a find method for RegisterDescriptorIterator
  gdb/python: make more use of RegisterDescriptors

 gdb/ChangeLog                                 | 24 +++++
 gdb/doc/ChangeLog                             | 11 +++
 gdb/doc/python.texi                           | 50 +++++++---
 gdb/python/py-frame.c                         | 22 +++--
 gdb/python/py-registers.c                     | 92 ++++++++++++++++++-
 gdb/python/py-unwind.c                        | 36 +-------
 gdb/python/python-internal.h                  | 19 ++++
 gdb/testsuite/ChangeLog                       |  9 ++
 .../gdb.python/py-arch-reg-names.exp          | 15 +++
 gdb/testsuite/gdb.python/py-unwind.py         | 11 ++-
 10 files changed, 231 insertions(+), 58 deletions(-)

--
2.25.4

Reply | Threaded
Open this post in threaded view
|

[PATCH 1/2] gdb: Add a find method for RegisterDescriptorIterator

Andrew Burgess
Adds a new method 'find' to the gdb.RegisterDescriptorIterator class,
this allows gdb.RegisterDescriptor objects to be looked up directly by
register name rather than having to iterate over all registers.

This will be of use for a later commit.

I've documented the new function in the manual, but I don't think a
NEWS entry is required here, as, since the last release, the whole
register descriptor mechanism is new, and is already mentioned in the
NEWS file.

gdb/ChangeLog:

        * python/py-registers.c: Add 'user-regs.h' include.
        (register_descriptor_iter_find): New function.
        (register_descriptor_iterator_object_methods): New static global
        methods array.
        (register_descriptor_iterator_object_type): Add pointer to methods
        array.

gdb/testsuite/ChangeLog:

        * gdb.python/py-arch-reg-names.exp: Add additional tests.

gdb/doc/ChangeLog:

        * python.texi (Registers In Python): Document new find function.
---
 gdb/ChangeLog                                 |  9 ++++
 gdb/doc/ChangeLog                             |  4 ++
 gdb/doc/python.texi                           |  9 ++++
 gdb/python/py-registers.c                     | 44 ++++++++++++++++++-
 gdb/testsuite/ChangeLog                       |  4 ++
 .../gdb.python/py-arch-reg-names.exp          | 15 +++++++
 6 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 4fb994ca6d9..4c0432372ab 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -5755,6 +5755,15 @@
 The name of this register.
 @end defvar
 
+It is also possible to lookup a register descriptor based on its name
+using the following @code{gdb.RegisterDescriptorIterator} function:
+
+@defun RegisterDescriptorIterator.find ( @var{name} )
+Returns a @code{gdb.RegisterDescriptor} for the register with name
+@var{name}, or @code{None} if there is no register with that name.
+@end defun
+
+
 Python code can also request from a @code{gdb.Architecture}
 information about the set of register groups available on a given
 architecture
diff --git a/gdb/python/py-registers.c b/gdb/python/py-registers.c
index f64ca3c401b..fffe3ecb1e6 100644
--- a/gdb/python/py-registers.c
+++ b/gdb/python/py-registers.c
@@ -23,6 +23,7 @@
 #include "disasm.h"
 #include "reggroups.h"
 #include "python-internal.h"
+#include "user-regs.h"
 #include <unordered_map>
 
 /* Token to access per-gdbarch data related to register descriptors.  */
@@ -337,6 +338,38 @@ gdbpy_register_descriptor_iter_next (PyObject *self)
   while (true);
 }
 
+/* Implement:
+
+   gdb.RegisterDescriptorIterator.find (self, name) -> gdb.RegisterDescriptor
+
+   Look up a descriptor for register with NAME.  If no matching register is
+   found then return None.  */
+
+static PyObject *
+register_descriptor_iter_find (PyObject *self, PyObject *args, PyObject *kw)
+{
+  static const char *keywords[] = { "name", NULL };
+  const char *register_name = NULL;
+
+  register_descriptor_iterator_object *iter_obj
+    = (register_descriptor_iterator_object *) self;
+  struct gdbarch *gdbarch = iter_obj->gdbarch;
+
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "s", keywords,
+ &register_name))
+    return NULL;
+
+  if (register_name != NULL && *register_name != '\0')
+    {
+      int regnum = user_reg_map_name_to_regnum (gdbarch, register_name,
+ strlen (register_name));
+      if (regnum >= 0)
+ return gdbpy_get_register_descriptor (gdbarch, regnum).release ();
+    }
+
+  Py_RETURN_NONE;
+}
+
 /* Initializes the new Python classes from this file in the gdb module.  */
 
 int
@@ -377,6 +410,15 @@ gdbpy_initialize_registers ()
    (PyObject *) &register_descriptor_iterator_object_type));
 }
 
+static PyMethodDef register_descriptor_iterator_object_methods [] = {
+  { "find", (PyCFunction) register_descriptor_iter_find,
+    METH_VARARGS | METH_KEYWORDS,
+    "registers (name) -> gdb.RegisterDescriptor.\n\
+Return a register descriptor for the register NAME, or None if no register\n\
+with that name exists in this iterator." },
+  {NULL}  /* Sentinel */
+};
+
 PyTypeObject register_descriptor_iterator_object_type = {
   PyVarObject_HEAD_INIT (NULL, 0)
   "gdb.RegisterDescriptorIterator",   /*tp_name*/
@@ -405,7 +447,7 @@ PyTypeObject register_descriptor_iterator_object_type = {
   0,  /*tp_weaklistoffset */
   gdbpy_register_descriptor_iter,  /*tp_iter */
   gdbpy_register_descriptor_iter_next,  /*tp_iternext */
-  0  /*tp_methods */
+  register_descriptor_iterator_object_methods /*tp_methods */
 };
 
 static gdb_PyGetSetDef gdbpy_register_descriptor_getset[] = {
diff --git a/gdb/testsuite/gdb.python/py-arch-reg-names.exp b/gdb/testsuite/gdb.python/py-arch-reg-names.exp
index 8dd34ef5fd2..891da1b6af5 100644
--- a/gdb/testsuite/gdb.python/py-arch-reg-names.exp
+++ b/gdb/testsuite/gdb.python/py-arch-reg-names.exp
@@ -104,3 +104,18 @@ gdb_py_test_silent_cmd \
  "    raise gdb.GdbError (\"miss-matched objects\")" \
  "\004" ] \
     "check names and objects match" 1
+
+# Ensure that the '.find' method on the iterator returns the same
+# Python object as we got from the iterator's list of descriptors.
+gdb_py_test_silent_cmd \
+    [multi_line \
+ "python" \
+ "def check_regs (arch, regs):" \
+ "   for r in (regs):" \
+ "     if (arch.registers ().find (r.name) != r):" \
+ "       raise gdb.GdbError (\"object miss-match\")" \
+ "end" ] \
+    "build check_obj function" 0
+gdb_py_test_silent_cmd \
+    "python check_regs (arch, arch.registers (\"general\"))" \
+    "ensure find gets expected descriptors" 1
--
2.25.4

Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2] gdb/python: make more use of RegisterDescriptors

Andrew Burgess
In reply to this post by Andrew Burgess
This commit unifies all of the Python register lookup code (used by
Frame.read_register, PendingFrame.read_register, and
gdb.UnwindInfo.add_saved_register), and adds support for using a
gdb.RegisterDescriptor for register lookup.

Currently the register unwind code (PendingFrame and UnwindInfo) allow
registers to be looked up either by name, or by GDB's internal
number.  I suspect the number was added for performance reasons, when
unwinding we don't want to repeatedly map from name to number for
every unwind.  However, this kind-of sucks, it means Python scripts
could include GDB's internal register numbers, and if we ever change
this numbering in the future users scripts will break in unexpected
ways.

Meanwhile, the Frame.read_register method only supports accessing
registers using a string, the register name.

This commit unifies all of the register to register-number lookup code
in our Python bindings, and adds a third choice into the mix, the use
of gdb.RegisterDescriptor.

The register descriptors can be looked up by name, but once looked up,
they contain GDB's register number, and so provide all of the
performance benefits of using a register number directly.  However, as
they are looked up by name we are no longer tightly binding the Python
API to GDB's internal numbering scheme.

As we may already have scripts in the wild that are using the register
numbers directly I have kept support for this in the API, but I have
listed this method last in the manual, and I have tried to stress that
this is NOT a good method to use and that users should use either a
string or register descriptor approach.

After this commit all existing Python code should function as before,
but users now have new options for how to identify registers.

gdb/ChangeLog:

        * python/py-frame.c: Remove 'user-regs.h' include.
        (frapy_read_register): Rewrite to make use of
        gdbpy_parse_register_id.
        * python/py-registers.c (gdbpy_parse_register_id): New function,
        moved here from python/py-unwind.c.  Updated the return type, and
        also accepts register descriptor objects.
        * python/py-unwind.c: Remove 'user-regs.h' include.
        (pyuw_parse_register_id): Moved to python/py-registers.c.
        (unwind_infopy_add_saved_register): Update to use
        gdbpy_parse_register_id.
        (pending_framepy_read_register): Likewise.
        * python/python-internal.h (gdbpy_parse_register_id): Declare.

gdb/testsuite/ChangeLog:

        * gdb.python/py-unwind.py: Update to make use of a register
        descriptor.

gdb/doc/ChangeLog:

        * python.texi (Unwinding Frames in Python): Update descriptions
        for PendingFrame.read_register and
        gdb.UnwindInfo.add_saved_register.
        (Frames In Python): Update description of Frame.read_register.
---
 gdb/ChangeLog                         | 15 +++++++++
 gdb/doc/ChangeLog                     |  7 ++++
 gdb/doc/python.texi                   | 41 ++++++++++++++++-------
 gdb/python/py-frame.c                 | 22 ++++++------
 gdb/python/py-registers.c             | 48 +++++++++++++++++++++++++++
 gdb/python/py-unwind.c                | 36 ++------------------
 gdb/python/python-internal.h          | 19 +++++++++++
 gdb/testsuite/ChangeLog               |  5 +++
 gdb/testsuite/gdb.python/py-unwind.py | 11 +++++-
 9 files changed, 147 insertions(+), 57 deletions(-)

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 4c0432372ab..d0a8b8c8a0d 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -2458,12 +2458,11 @@
 
 @defun PendingFrame.read_register (reg)
 This method returns the contents of the register @var{reg} in the
-frame as a @code{gdb.Value} object.  @var{reg} can be either a
-register number or a register name; the values are platform-specific.
-They are usually found in the corresponding
-@file{@var{platform}-tdep.h} file in the @value{GDBN} source tree.  If
-@var{reg} does not name a register for the current architecture, this
-method will throw an exception.
+frame as a @code{gdb.Value} object.  For a description of the
+acceptable values of @var{reg} see
+@ref{gdbpy_frame_read_register,,Frame.read_register}.  If @var{reg}
+does not name a register for the current architecture, this method
+will throw an exception.
 
 Note that this method will always return a @code{gdb.Value} for a
 valid register name.  This does not mean that the value will be valid.
@@ -2532,8 +2531,8 @@
 specify caller registers that have been saved in this frame:
 
 @defun gdb.UnwindInfo.add_saved_register (reg, value)
-@var{reg} identifies the register.  It can be a number or a name, just
-as for the @code{PendingFrame.read_register} method above.
+@var{reg} identifies the register, for a description of the acceptable
+values see @ref{gdbpy_frame_read_register,,Frame.read_register}.
 @var{value} is a register value (a @code{gdb.Value} object).
 @end defun
 
@@ -4687,10 +4686,28 @@
 
 @anchor{gdbpy_frame_read_register}
 @defun Frame.read_register (register)
-Return the value of @var{register} in this frame.  The @var{register}
-argument must be a string (e.g., @code{'sp'} or @code{'rax'}).
-Returns a @code{Gdb.Value} object.  Throws an exception if @var{register}
-does not exist.
+Return the value of @var{register} in this frame.  Returns a
+@code{Gdb.Value} object.  Throws an exception if @var{register} does
+not exist.  The @var{register} argument must be one of the following:
+@enumerate
+@item
+A string that is the name of a valid register (e.g., @code{'sp'} or
+@code{'rax'}).
+@item
+A @code{gdb.RegisterDescriptor} object, see @ref{Registers In Python}
+for further information.
+@item
+A @value{GDBN} internal, platform specific number.  Using these
+numbers is supported for historic reasons, but is not recommended as
+future changes to @value{GDBN} could change the mapping between numbers
+and the registers they represent, breaking any Python code.  The
+numbers are usually found in the corresponding
+@file{@var{platform}-tdep.h} file in the @value{GDBN} source tree.
+@end enumerate
+Using a string to access registers will be slightly slower than the
+other two methods as @value{GDBN} must lookup the mapping between name
+and internal register number.  If performance is critical consider
+looking up and caching a @code{gdb.RegisterDescriptor} object.
 @end defun
 
 @defun Frame.read_var (variable @r{[}, block@r{]})
diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index 3fd1e7fabcc..e121afb222d 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -27,7 +27,6 @@
 #include "python-internal.h"
 #include "symfile.h"
 #include "objfiles.h"
-#include "user-regs.h"
 
 typedef struct {
   PyObject_HEAD
@@ -242,12 +241,11 @@ frapy_pc (PyObject *self, PyObject *args)
 static PyObject *
 frapy_read_register (PyObject *self, PyObject *args)
 {
-  const char *regnum_str;
+  PyObject *pyo_reg_id;
   struct value *val = NULL;
 
-  if (!PyArg_ParseTuple (args, "s", &regnum_str))
+  if (!PyArg_UnpackTuple (args, "read_register", 1, 1, &pyo_reg_id))
     return NULL;
-
   try
     {
       struct frame_info *frame;
@@ -255,14 +253,18 @@ frapy_read_register (PyObject *self, PyObject *args)
 
       FRAPY_REQUIRE_VALID (self, frame);
 
-      regnum = user_reg_map_name_to_regnum (get_frame_arch (frame),
-                                            regnum_str,
-                                            strlen (regnum_str));
-      if (regnum >= 0)
-        val = value_of_register (regnum, frame);
+      if (!gdbpy_parse_register_id (get_frame_arch (frame), pyo_reg_id,
+    &regnum))
+ {
+  PyErr_SetString (PyExc_ValueError, "Bad register");
+  return NULL;
+ }
+
+      gdb_assert (regnum >= 0);
+      val = value_of_register (regnum, frame);
 
       if (val == NULL)
-        PyErr_SetString (PyExc_ValueError, _("Unknown register."));
+        PyErr_SetString (PyExc_ValueError, _("Can't read register."));
     }
   catch (const gdb_exception &except)
     {
diff --git a/gdb/python/py-registers.c b/gdb/python/py-registers.c
index fffe3ecb1e6..e5bca7e6064 100644
--- a/gdb/python/py-registers.c
+++ b/gdb/python/py-registers.c
@@ -370,6 +370,54 @@ register_descriptor_iter_find (PyObject *self, PyObject *args, PyObject *kw)
   Py_RETURN_NONE;
 }
 
+/* See python-internal.h.  */
+
+bool
+gdbpy_parse_register_id (struct gdbarch *gdbarch, PyObject *pyo_reg_id,
+ int *reg_num)
+{
+  if (pyo_reg_id == NULL)
+    return false;
+
+  /* The register could be a string, its name.  */
+  if (gdbpy_is_string (pyo_reg_id))
+    {
+      gdb::unique_xmalloc_ptr<char> reg_name (gdbpy_obj_to_string (pyo_reg_id));
+
+      if (reg_name != NULL)
+ {
+  *reg_num = user_reg_map_name_to_regnum (gdbarch, reg_name.get (),
+  strlen (reg_name.get ()));
+  return *reg_num >= 0;
+ }
+    }
+  /* The register could be its internal GDB register number.  */
+  else if (PyInt_Check (pyo_reg_id))
+    {
+      long value;
+      if (gdb_py_int_as_long (pyo_reg_id, &value) && (int) value == value)
+        {
+  if (user_reg_map_regnum_to_name (gdbarch, value) != NULL)
+    {
+      *reg_num = (int) value;
+      return true;
+    }
+        }
+    }
+  /* The register could be a gdb.RegisterDescriptor object.  */
+  else if (PyObject_IsInstance (pyo_reg_id,
+   (PyObject *) &register_descriptor_object_type))
+    {
+      register_descriptor_object *reg
+ = (register_descriptor_object *) pyo_reg_id;
+      if (reg->gdbarch != gdbarch)
+ return false;
+      *reg_num = reg->regnum;
+      return true;
+    }
+  return false;
+}
+
 /* Initializes the new Python classes from this file in the gdb module.  */
 
 int
diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index 1cef491cedf..55d6a310c93 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -27,7 +27,6 @@
 #include "python-internal.h"
 #include "regcache.h"
 #include "valprint.h"
-#include "user-regs.h"
 
 #define TRACE_PY_UNWIND(level, args...) if (pyuw_debug >= level)  \
   { fprintf_unfiltered (gdb_stdlog, args); }
@@ -101,37 +100,6 @@ static unsigned int pyuw_debug = 0;
 
 static struct gdbarch_data *pyuw_gdbarch_data;
 
-/* Parses register id, which can be either a number or a name.
-   Returns 1 on success, 0 otherwise.  */
-
-static int
-pyuw_parse_register_id (struct gdbarch *gdbarch, PyObject *pyo_reg_id,
-                        int *reg_num)
-{
-  if (pyo_reg_id == NULL)
-    return 0;
-  if (gdbpy_is_string (pyo_reg_id))
-    {
-      gdb::unique_xmalloc_ptr<char> reg_name (gdbpy_obj_to_string (pyo_reg_id));
-
-      if (reg_name == NULL)
-        return 0;
-      *reg_num = user_reg_map_name_to_regnum (gdbarch, reg_name.get (),
-                                              strlen (reg_name.get ()));
-      return *reg_num >= 0;
-    }
-  else if (PyInt_Check (pyo_reg_id))
-    {
-      long value;
-      if (gdb_py_int_as_long (pyo_reg_id, &value) && (int) value == value)
-        {
-          *reg_num = (int) value;
-          return user_reg_map_regnum_to_name (gdbarch, *reg_num) != NULL;
-        }
-    }
-  return 0;
-}
-
 /* Convert gdb.Value instance to inferior's pointer.  Return 1 on success,
    0 on failure.  */
 
@@ -275,7 +243,7 @@ unwind_infopy_add_saved_register (PyObject *self, PyObject *args)
   if (!PyArg_UnpackTuple (args, "previous_frame_register", 2, 2,
                           &pyo_reg_id, &pyo_reg_value))
     return NULL;
-  if (!pyuw_parse_register_id (pending_frame->gdbarch, pyo_reg_id, &regnum))
+  if (!gdbpy_parse_register_id (pending_frame->gdbarch, pyo_reg_id, &regnum))
     {
       PyErr_SetString (PyExc_ValueError, "Bad register");
       return NULL;
@@ -376,7 +344,7 @@ pending_framepy_read_register (PyObject *self, PyObject *args)
     }
   if (!PyArg_UnpackTuple (args, "read_register", 1, 1, &pyo_reg_id))
     return NULL;
-  if (!pyuw_parse_register_id (pending_frame->gdbarch, pyo_reg_id, &regnum))
+  if (!gdbpy_parse_register_id (pending_frame->gdbarch, pyo_reg_id, &regnum))
     {
       PyErr_SetString (PyExc_ValueError, "Bad register");
       return NULL;
diff --git a/gdb/python/python-internal.h b/gdb/python/python-internal.h
index 1e6dcf3dbb0..6874543441b 100644
--- a/gdb/python/python-internal.h
+++ b/gdb/python/python-internal.h
@@ -776,4 +776,23 @@ struct Py_buffer_deleter
 /* A unique_ptr specialization for Py_buffer.  */
 typedef std::unique_ptr<Py_buffer, Py_buffer_deleter> Py_buffer_up;
 
+/* Parse a register number from PYO_REG_ID and place the register number
+   into *REG_NUM.  The register is a register for GDBARCH.
+
+   If a register is parsed successfully then *REG_NUM will have been
+   updated, and true is returned.  Otherwise the contents of *REG_NUM are
+   undefined, and false is returned.
+
+   The PYO_REG_ID object can be a string, the name of the register.  This
+   is the slowest approach as GDB has to map the name to a number for each
+   call.  Alternatively PYO_REG_ID can be an internal GDB register
+   number.  This is quick but should not be encouraged as this means
+   Python scripts are now dependent on GDB's internal register numbering.
+   Final PYO_REG_ID can be a gdb.RegisterDescriptor object, these objects
+   can be looked up by name once, and then cache the register number so
+   should be as quick as using a register number.  */
+
+extern bool gdbpy_parse_register_id (struct gdbarch *gdbarch,
+     PyObject *pyo_reg_id, int *reg_num);
+
 #endif /* PYTHON_PYTHON_INTERNAL_H */
diff --git a/gdb/testsuite/gdb.python/py-unwind.py b/gdb/testsuite/gdb.python/py-unwind.py
index d01da80f25b..59602d69028 100644
--- a/gdb/testsuite/gdb.python/py-unwind.py
+++ b/gdb/testsuite/gdb.python/py-unwind.py
@@ -33,12 +33,19 @@ class FrameId(object):
 class TestUnwinder(Unwinder):
     AMD64_RBP = 6
     AMD64_RSP = 7
-    AMD64_RIP = 16
+    AMD64_RIP = None
 
     def __init__(self):
         Unwinder.__init__(self, "test unwinder")
         self.char_ptr_t = gdb.lookup_type("unsigned char").pointer()
         self.char_ptr_ptr_t = self.char_ptr_t.pointer()
+        self._last_arch = None
+
+    # Update the register descriptor AMD64_RIP based on ARCH.
+    def _update_register_descriptors (self, arch):
+        if (self._last_arch != arch):
+            TestUnwinder.AMD64_RIP = arch.registers ().find ("rip")
+            self._last_arch = arch
 
     def _read_word(self, address):
         return address.cast(self.char_ptr_ptr_t).dereference()
@@ -77,6 +84,8 @@ class TestUnwinder(Unwinder):
         if (inf_arch != frame_arch):
             raise gdb.GdbError ("architecture mismatch")
 
+        self._update_register_descriptors (frame_arch)
+
         try:
             # NOTE: the registers in Unwinder API can be referenced
             # either by name or by number. The code below uses both
--
2.25.4

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] gdb: Add a find method for RegisterDescriptorIterator

Eli Zaretskii
In reply to this post by Andrew Burgess
> From: Andrew Burgess <[hidden email]>
> Date: Fri, 24 Jul 2020 10:57:51 +0100
>
> +It is also possible to lookup a register descriptor based on its name
> +using the following @code{gdb.RegisterDescriptorIterator} function:
> +
> +@defun RegisterDescriptorIterator.find ( @var{name} )
                                           ^          ^
Please remove these extra blanks, they are against our coding
conventions.

> +Returns a @code{gdb.RegisterDescriptor} for the register with name
> +@var{name}, or @code{None} if there is no register with that name.
> +@end defun

Is it necessary or useful to say what kind of data type is NAME?  Or
will this be obvious to any Python programmer?

Thanks.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] gdb/python: make more use of RegisterDescriptors

Eli Zaretskii
In reply to this post by Andrew Burgess
> From: Andrew Burgess <[hidden email]>
> Date: Fri, 24 Jul 2020 10:57:52 +0100
>
> +A @code{gdb.RegisterDescriptor} object, see @ref{Registers In Python}
> +for further information.

Please add a comma after the closing brace of @ref.  Newer versions of
Texinfo don't need that, but older ones will barf.

> +@item
> +A @value{GDBN} internal, platform specific number.  Using these
> +numbers is supported for historic reasons, but is not recommended as
> +future changes to @value{GDBN} could change the mapping between numbers
> +and the registers they represent, breaking any Python code.  The

I suggest "breaking any Python code that uses the platform-specific
numbers", to qualify the breakage.

The documentation part is okay with these nits fixed.  Thanks.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] gdb: Add a find method for RegisterDescriptorIterator

Tom Tromey-2
In reply to this post by Andrew Burgess
>>>>> "Andrew" == Andrew Burgess <[hidden email]> writes:

Andrew> Adds a new method 'find' to the gdb.RegisterDescriptorIterator class,
Andrew> this allows gdb.RegisterDescriptor objects to be looked up directly by
Andrew> register name rather than having to iterate over all registers.

Andrew> This will be of use for a later commit.

Andrew> I've documented the new function in the manual, but I don't think a
Andrew> NEWS entry is required here, as, since the last release, the whole
Andrew> register descriptor mechanism is new, and is already mentioned in the
Andrew> NEWS file.

Andrew> gdb/ChangeLog:

Andrew> * python/py-registers.c: Add 'user-regs.h' include.
Andrew> (register_descriptor_iter_find): New function.
Andrew> (register_descriptor_iterator_object_methods): New static global
Andrew> methods array.
Andrew> (register_descriptor_iterator_object_type): Add pointer to methods
Andrew> array.

Thanks for doing this.  This looks good.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] gdb/python: make more use of RegisterDescriptors

Tom Tromey-2
In reply to this post by Andrew Burgess
>>>>> "Andrew" == Andrew Burgess <[hidden email]> writes:

Andrew> This commit unifies all of the Python register lookup code (used by
Andrew> Frame.read_register, PendingFrame.read_register, and
Andrew> gdb.UnwindInfo.add_saved_register), and adds support for using a
Andrew> gdb.RegisterDescriptor for register lookup.

Thanks for doing this.  This approach seems good to me.

Andrew> +other two methods as @value{GDBN} must lookup the mapping between name

I'd suggest "look up" here.

Andrew> +/* See python-internal.h.  */
Andrew> +
Andrew> +bool
Andrew> +gdbpy_parse_register_id (struct gdbarch *gdbarch, PyObject *pyo_reg_id,
Andrew> + int *reg_num)
Andrew> +{
Andrew> +  if (pyo_reg_id == NULL)
Andrew> +    return false;

I think this function should consistently set the Python error when
returning false.

Andrew> +  if (gdbpy_is_string (pyo_reg_id))
Andrew> +    {
Andrew> +      gdb::unique_xmalloc_ptr<char> reg_name (gdbpy_obj_to_string (pyo_reg_id));

... gdbpy_obj_to_string will set the Python error when it returns NULL.

Andrew> +      if (gdb_py_int_as_long (pyo_reg_id, &value) && (int) value == value)

gdb_py_int_as_long can set the error as well.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] gdb: Add a find method for RegisterDescriptorIterator

Andrew Burgess
In reply to this post by Eli Zaretskii
* Eli Zaretskii <[hidden email]> [2020-07-24 13:32:17 +0300]:

> > From: Andrew Burgess <[hidden email]>
> > Date: Fri, 24 Jul 2020 10:57:51 +0100
> >
> > +It is also possible to lookup a register descriptor based on its name
> > +using the following @code{gdb.RegisterDescriptorIterator} function:
> > +
> > +@defun RegisterDescriptorIterator.find ( @var{name} )
>                                            ^          ^
> Please remove these extra blanks, they are against our coding
> conventions.
>
> > +Returns a @code{gdb.RegisterDescriptor} for the register with name
> > +@var{name}, or @code{None} if there is no register with that name.
> > +@end defun
>
> Is it necessary or useful to say what kind of data type is NAME?  Or
> will this be obvious to any Python programmer?

Thanks for the feedback.  The patch below includes updated
documentation.  How's this?

Thanks,
Andrew

--

commit efe6a55ba25e088c67cde777bc8d524281356bb2
Author: Andrew Burgess <[hidden email]>
Date:   Wed Jul 22 14:02:30 2020 +0100

    gdb: Add a find method for RegisterDescriptorIterator
   
    Adds a new method 'find' to the gdb.RegisterDescriptorIterator class,
    this allows gdb.RegisterDescriptor objects to be looked up directly by
    register name rather than having to iterate over all registers.
   
    This will be of use for a later commit.
   
    I've documented the new function in the manual, but I don't think a
    NEWS entry is required here, as, since the last release, the whole
    register descriptor mechanism is new, and is already mentioned in the
    NEWS file.
   
    gdb/ChangeLog:
   
            * python/py-registers.c: Add 'user-regs.h' include.
            (register_descriptor_iter_find): New function.
            (register_descriptor_iterator_object_methods): New static global
            methods array.
            (register_descriptor_iterator_object_type): Add pointer to methods
            array.
   
    gdb/testsuite/ChangeLog:
   
            * gdb.python/py-arch-reg-names.exp: Add additional tests.
   
    gdb/doc/ChangeLog:
   
            * python.texi (Registers In Python): Document new find function.

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 4fb994ca6d9..c9dc1ff3b71 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -5755,6 +5755,15 @@
 The name of this register.
 @end defvar
 
+It is also possible to lookup a register descriptor based on its name
+using the following @code{gdb.RegisterDescriptorIterator} function:
+
+@defun RegisterDescriptorIterator.find (@var{name})
+Takes @var{name} as an argument, which must be a string, and returns a
+@code{gdb.RegisterDescriptor} for the register with that name, or
+@code{None} if there is no register with that name.
+@end defun
+
 Python code can also request from a @code{gdb.Architecture}
 information about the set of register groups available on a given
 architecture
diff --git a/gdb/python/py-registers.c b/gdb/python/py-registers.c
index f64ca3c401b..fffe3ecb1e6 100644
--- a/gdb/python/py-registers.c
+++ b/gdb/python/py-registers.c
@@ -23,6 +23,7 @@
 #include "disasm.h"
 #include "reggroups.h"
 #include "python-internal.h"
+#include "user-regs.h"
 #include <unordered_map>
 
 /* Token to access per-gdbarch data related to register descriptors.  */
@@ -337,6 +338,38 @@ gdbpy_register_descriptor_iter_next (PyObject *self)
   while (true);
 }
 
+/* Implement:
+
+   gdb.RegisterDescriptorIterator.find (self, name) -> gdb.RegisterDescriptor
+
+   Look up a descriptor for register with NAME.  If no matching register is
+   found then return None.  */
+
+static PyObject *
+register_descriptor_iter_find (PyObject *self, PyObject *args, PyObject *kw)
+{
+  static const char *keywords[] = { "name", NULL };
+  const char *register_name = NULL;
+
+  register_descriptor_iterator_object *iter_obj
+    = (register_descriptor_iterator_object *) self;
+  struct gdbarch *gdbarch = iter_obj->gdbarch;
+
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kw, "s", keywords,
+ &register_name))
+    return NULL;
+
+  if (register_name != NULL && *register_name != '\0')
+    {
+      int regnum = user_reg_map_name_to_regnum (gdbarch, register_name,
+ strlen (register_name));
+      if (regnum >= 0)
+ return gdbpy_get_register_descriptor (gdbarch, regnum).release ();
+    }
+
+  Py_RETURN_NONE;
+}
+
 /* Initializes the new Python classes from this file in the gdb module.  */
 
 int
@@ -377,6 +410,15 @@ gdbpy_initialize_registers ()
    (PyObject *) &register_descriptor_iterator_object_type));
 }
 
+static PyMethodDef register_descriptor_iterator_object_methods [] = {
+  { "find", (PyCFunction) register_descriptor_iter_find,
+    METH_VARARGS | METH_KEYWORDS,
+    "registers (name) -> gdb.RegisterDescriptor.\n\
+Return a register descriptor for the register NAME, or None if no register\n\
+with that name exists in this iterator." },
+  {NULL}  /* Sentinel */
+};
+
 PyTypeObject register_descriptor_iterator_object_type = {
   PyVarObject_HEAD_INIT (NULL, 0)
   "gdb.RegisterDescriptorIterator",   /*tp_name*/
@@ -405,7 +447,7 @@ PyTypeObject register_descriptor_iterator_object_type = {
   0,  /*tp_weaklistoffset */
   gdbpy_register_descriptor_iter,  /*tp_iter */
   gdbpy_register_descriptor_iter_next,  /*tp_iternext */
-  0  /*tp_methods */
+  register_descriptor_iterator_object_methods /*tp_methods */
 };
 
 static gdb_PyGetSetDef gdbpy_register_descriptor_getset[] = {
diff --git a/gdb/testsuite/gdb.python/py-arch-reg-names.exp b/gdb/testsuite/gdb.python/py-arch-reg-names.exp
index 8dd34ef5fd2..891da1b6af5 100644
--- a/gdb/testsuite/gdb.python/py-arch-reg-names.exp
+++ b/gdb/testsuite/gdb.python/py-arch-reg-names.exp
@@ -104,3 +104,18 @@ gdb_py_test_silent_cmd \
  "    raise gdb.GdbError (\"miss-matched objects\")" \
  "\004" ] \
     "check names and objects match" 1
+
+# Ensure that the '.find' method on the iterator returns the same
+# Python object as we got from the iterator's list of descriptors.
+gdb_py_test_silent_cmd \
+    [multi_line \
+ "python" \
+ "def check_regs (arch, regs):" \
+ "   for r in (regs):" \
+ "     if (arch.registers ().find (r.name) != r):" \
+ "       raise gdb.GdbError (\"object miss-match\")" \
+ "end" ] \
+    "build check_obj function" 0
+gdb_py_test_silent_cmd \
+    "python check_regs (arch, arch.registers (\"general\"))" \
+    "ensure find gets expected descriptors" 1
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] gdb: Add a find method for RegisterDescriptorIterator

Eli Zaretskii
> Date: Sat, 25 Jul 2020 00:17:05 +0100
> From: Andrew Burgess <[hidden email]>
> Cc: [hidden email]
>
> Thanks for the feedback.  The patch below includes updated
> documentation.  How's this?

LGTM, thanks.