[PATCHv2,1/2] gdb/python: Reuse gdb.RegisterDescriptor objects where possible

Message ID c79440bcf9ea71e9887a7b3d04177f98aabc38b8.1594746770.git.andrew.burgess@embecosm.com
State New
Headers show
Series
  • Improvements to Python Register Descriptor API
Related show

Commit Message

Andrew Burgess July 14, 2020, 5:14 p.m.
Instead of having the gdb.RegisterDescriptorIterator creating new
gdb.RegisterDescriptor objects for each regnum, instead cache
gdb.RegisterDescriptor objects on the gdbarch object and reuse these.

This means that for every gdbarch/regnum pair there is a single unique
gdb.RegisterDescriptor, this feels like a neater implementation than
the existing one.

It is possible for a user to see (in Python code) that the descriptors
are now identical, but as the descriptors are read-only this should
make no real difference.

There should be no other user visible changes.

gdb/ChangeLog:

	* python/py-registers.c (gdbpy_register_object_data): New static
	global.
	(gdbpy_register_object_data_init): New function.
	(gdbpy_new_register_descriptor): Renamed to...
	(gdbpy_get_register_descriptor): ...this, and update to reuse
	existing register descriptors where possible.
	(gdbpy_register_descriptor_iter_next): Update.
	(gdbpy_initialize_registers): Register new gdbarch data.

gdb/testsuite/ChangeLog:

	* gdb.python/py-arch-reg-names.exp: Additional tests.
---
 gdb/ChangeLog                                 | 11 ++++
 gdb/python/py-registers.c                     | 61 +++++++++++++++----
 gdb/testsuite/ChangeLog                       |  4 ++
 .../gdb.python/py-arch-reg-names.exp          | 19 ++++++
 4 files changed, 82 insertions(+), 13 deletions(-)

-- 
2.25.4

Comments

Pedro Alves July 22, 2020, 1:10 p.m. | #1
Hi Andrew,

Some nits below.

On 7/14/20 6:14 PM, Andrew Burgess wrote:

>  

> +/* Token to access per-gdbarch data related to register descriptors.  */

> +static struct gdbarch_data *gdbpy_register_object_data = NULL;

> +

>  /* Structure for iterator over register descriptors.  */

>  typedef struct {

>    PyObject_HEAD

> @@ -81,6 +84,17 @@ typedef struct {

>  extern PyTypeObject reggroup_object_type

>      CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("reggroup_object");

>  

> +/* Associates a vector of gdb.RegisterDescriptor objects with GDBARCH as

> +   gdbarch_data via the gdbarch post init registration mechanism

> +   (gdbarch_data_register_post_init).  */

> +

> +static void *

> +gdbpy_register_object_data_init (struct gdbarch *gdbarch)

> +{

> +  std::vector<gdbpy_ref<>> *vec = new (std::vector<gdbpy_ref<>>);

> +  return (void *) vec;


This could just be:

  return new std::vector<gdbpy_ref<>>;


> +}

> +

>  /* Create a new gdb.RegisterGroup object wrapping REGGROUP.  */

>  

>  static PyObject *

> @@ -117,20 +131,38 @@ gdbpy_reggroup_name (PyObject *self, void *closure)

>    return gdbpy_reggroup_to_string (self);

>  }

>  

> -/* Create an return a new gdb.RegisterDescriptor object.  */

> -static PyObject *

> -gdbpy_new_register_descriptor (struct gdbarch *gdbarch,

> +/* Return a gdb.RegisterDescriptor object for REGNUM from GDBARCH.  For

> +   each REGNUM (in GDBARCH) only one descriptor is ever created, which is

> +   then cached on the GDBARCH.  */

> +

> +static gdbpy_ref<>

> +gdbpy_get_register_descriptor (struct gdbarch *gdbarch,

>  			       int regnum)

>  {

> -  /* Create a new object and fill in its details.  */

> -  register_descriptor_object *reg

> -    = PyObject_New (register_descriptor_object,

> -		    &register_descriptor_object_type);

> -  if (reg == NULL)

> -    return NULL;

> -  reg->regnum = regnum;

> -  reg->gdbarch = gdbarch;

> -  return (PyObject *) reg;

> +  auto vec = (std::vector<gdbpy_ref<>> *) gdbarch_data

> +    (gdbarch, gdbpy_register_object_data);


Wrap the rhs in parens so the second line is properly aligned,
per the GNU standard's "so that emacs aligns it automatically"
rule:

  auto vec = ((std::vector<gdbpy_ref<>> *) gdbarch_data
	      (gdbarch, gdbpy_register_object_data));

Or you could put the = in the next line, so that it aligns
without the parens:

  auto vec
    = (std::vector<gdbpy_ref<>> *) gdbarch_data (gdbarch,
						 gdbpy_register_object_data);

Note, I think it's more idiomatic to write the * for pointers:

  auto *vec = ....

But since vec can't be NULL, I'd write a reference instead:

  auto &vec
    = *(std::vector<gdbpy_ref<>> *) gdbarch_data (gdbarch,
						  gdbpy_register_object_data);

> +

> +  /* Ensure that we have enough entries in the vector.  */

> +  if (vec->size () <= regnum)

> +    vec->resize ((regnum + 1), nullptr);

> +

> +  /* If we don't already have a descriptor for REGNUM in GDBARCH then

> +     create one now.  */

> +  if (vec->at (regnum) == nullptr)


Is there a reason you're using at?  Was it to avoid

 (*vec)[regnum]

or was it really about out-of-range errors?

If such as exception were thrown, it would be a logic
bug in GDB.  And note that we don't catch it anywhere, so
it would bring down GDB.

There's a school of thought that claims that at should not
really exist, and I agree with it.  :-)

If you go the reference route, then you can write the natural:

  if (vec[regnum] = nullptr)

> +    {

> +      gdbpy_ref <register_descriptor_object> reg

> +	(PyObject_New (register_descriptor_object,

> +		       &register_descriptor_object_type));

> +      if (reg == NULL)

> +	return NULL;

> +      reg->regnum = regnum;

> +      reg->gdbarch = gdbarch;

> +      vec->at (regnum) = gdbpy_ref<> ((PyObject *) reg.release ());

> +    }

> +

> +  /* Grab the register descriptor from the vector, the reference count is

> +     automatically incremented thanks to gdbpy_ref.  */

> +  return vec->at (regnum);


Ditto in these two other spots.
Andrew Burgess July 22, 2020, 2:05 p.m. | #2
* Pedro Alves <pedro@palves.net> [2020-07-22 14:10:17 +0100]:

> Hi Andrew,

> 

> Some nits below.

> 

> On 7/14/20 6:14 PM, Andrew Burgess wrote:

> 

> >  

> > +/* Token to access per-gdbarch data related to register descriptors.  */

> > +static struct gdbarch_data *gdbpy_register_object_data = NULL;

> > +

> >  /* Structure for iterator over register descriptors.  */

> >  typedef struct {

> >    PyObject_HEAD

> > @@ -81,6 +84,17 @@ typedef struct {

> >  extern PyTypeObject reggroup_object_type

> >      CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("reggroup_object");

> >  

> > +/* Associates a vector of gdb.RegisterDescriptor objects with GDBARCH as

> > +   gdbarch_data via the gdbarch post init registration mechanism

> > +   (gdbarch_data_register_post_init).  */

> > +

> > +static void *

> > +gdbpy_register_object_data_init (struct gdbarch *gdbarch)

> > +{

> > +  std::vector<gdbpy_ref<>> *vec = new (std::vector<gdbpy_ref<>>);

> > +  return (void *) vec;

> 

> This could just be:

> 

>   return new std::vector<gdbpy_ref<>>;

> 

> 

> > +}

> > +

> >  /* Create a new gdb.RegisterGroup object wrapping REGGROUP.  */

> >  

> >  static PyObject *

> > @@ -117,20 +131,38 @@ gdbpy_reggroup_name (PyObject *self, void *closure)

> >    return gdbpy_reggroup_to_string (self);

> >  }

> >  

> > -/* Create an return a new gdb.RegisterDescriptor object.  */

> > -static PyObject *

> > -gdbpy_new_register_descriptor (struct gdbarch *gdbarch,

> > +/* Return a gdb.RegisterDescriptor object for REGNUM from GDBARCH.  For

> > +   each REGNUM (in GDBARCH) only one descriptor is ever created, which is

> > +   then cached on the GDBARCH.  */

> > +

> > +static gdbpy_ref<>

> > +gdbpy_get_register_descriptor (struct gdbarch *gdbarch,

> >  			       int regnum)

> >  {

> > -  /* Create a new object and fill in its details.  */

> > -  register_descriptor_object *reg

> > -    = PyObject_New (register_descriptor_object,

> > -		    &register_descriptor_object_type);

> > -  if (reg == NULL)

> > -    return NULL;

> > -  reg->regnum = regnum;

> > -  reg->gdbarch = gdbarch;

> > -  return (PyObject *) reg;

> > +  auto vec = (std::vector<gdbpy_ref<>> *) gdbarch_data

> > +    (gdbarch, gdbpy_register_object_data);

> 

> Wrap the rhs in parens so the second line is properly aligned,

> per the GNU standard's "so that emacs aligns it automatically"

> rule:

> 

>   auto vec = ((std::vector<gdbpy_ref<>> *) gdbarch_data

> 	      (gdbarch, gdbpy_register_object_data));

> 

> Or you could put the = in the next line, so that it aligns

> without the parens:

> 

>   auto vec

>     = (std::vector<gdbpy_ref<>> *) gdbarch_data (gdbarch,

> 						 gdbpy_register_object_data);

> 

> Note, I think it's more idiomatic to write the * for pointers:

> 

>   auto *vec = ....

> 

> But since vec can't be NULL, I'd write a reference instead:

> 

>   auto &vec

>     = *(std::vector<gdbpy_ref<>> *) gdbarch_data (gdbarch,

> 						  gdbpy_register_object_data);

> 

> > +

> > +  /* Ensure that we have enough entries in the vector.  */

> > +  if (vec->size () <= regnum)

> > +    vec->resize ((regnum + 1), nullptr);

> > +

> > +  /* If we don't already have a descriptor for REGNUM in GDBARCH then

> > +     create one now.  */

> > +  if (vec->at (regnum) == nullptr)

> 

> Is there a reason you're using at?  Was it to avoid

> 

>  (*vec)[regnum]

> 

> or was it really about out-of-range errors?

> 

> If such as exception were thrown, it would be a logic

> bug in GDB.  And note that we don't catch it anywhere, so

> it would bring down GDB.

> 

> There's a school of thought that claims that at should not

> really exist, and I agree with it.  :-)

> 

> If you go the reference route, then you can write the natural:

> 

>   if (vec[regnum] = nullptr)

> 

> > +    {

> > +      gdbpy_ref <register_descriptor_object> reg

> > +	(PyObject_New (register_descriptor_object,

> > +		       &register_descriptor_object_type));

> > +      if (reg == NULL)

> > +	return NULL;

> > +      reg->regnum = regnum;

> > +      reg->gdbarch = gdbarch;

> > +      vec->at (regnum) = gdbpy_ref<> ((PyObject *) reg.release ());

> > +    }

> > +

> > +  /* Grab the register descriptor from the vector, the reference count is

> > +     automatically incremented thanks to gdbpy_ref.  */

> > +  return vec->at (regnum);

> 

> Ditto in these two other spots.


Pedro,

Thanks for your feedback.  As this patch was already merged, the patch
below applies on top of current master to address these issues.

If I don't hear anything I'll apply this in a couple of days.

Thanks,
Andrew

---

commit ed3d57aca93bfe3e0eaa5888486b9fc84d06a0c6
Author: Andrew Burgess <andrew.burgess@embecosm.com>
Date:   Wed Jul 22 14:57:55 2020 +0100

    gdb/python: Use reference not pointer in py-registers.c
    
    Pedro's review comments arrived after I'd already committed this
    change:
    
      commit f7306dac19c502232f766c3881313857915f330d
      Date:   Tue Jul 7 15:00:30 2020 +0100
    
          gdb/python: Reuse gdb.RegisterDescriptor objects where possible
    
    See:
    
      https://sourceware.org/pipermail/gdb-patches/2020-July/170726.html
    
    There should be no user visible changes after this commit.
    
    gdb/ChangeLog:
    
            * python/py-registers.c (gdbpy_register_object_data_init): Remove
            redundant local variable.
            (gdbpy_get_register_descriptor): Extract descriptor vector as a
            reference, not pointer, update code accordingly.

diff --git a/gdb/python/py-registers.c b/gdb/python/py-registers.c
index 9396498cc34..f64ca3c401b 100644
--- a/gdb/python/py-registers.c
+++ b/gdb/python/py-registers.c
@@ -92,8 +92,7 @@ extern PyTypeObject reggroup_object_type
 static void *
 gdbpy_register_object_data_init (struct gdbarch *gdbarch)
 {
-  std::vector<gdbpy_ref<>> *vec = new (std::vector<gdbpy_ref<>>);
-  return (void *) vec;
+  return new std::vector<gdbpy_ref<>>;
 }
 
 /* Return a gdb.RegisterGroup object wrapping REGGROUP.  The register
@@ -158,16 +157,17 @@ static gdbpy_ref<>
 gdbpy_get_register_descriptor (struct gdbarch *gdbarch,
 			       int regnum)
 {
-  auto vec = (std::vector<gdbpy_ref<>> *) gdbarch_data
-    (gdbarch, gdbpy_register_object_data);
+  auto &vec
+    = *(std::vector<gdbpy_ref<>> *) gdbarch_data (gdbarch,
+						  gdbpy_register_object_data);
 
   /* Ensure that we have enough entries in the vector.  */
-  if (vec->size () <= regnum)
-    vec->resize ((regnum + 1), nullptr);
+  if (vec.size () <= regnum)
+    vec.resize ((regnum + 1), nullptr);
 
   /* If we don't already have a descriptor for REGNUM in GDBARCH then
      create one now.  */
-  if (vec->at (regnum) == nullptr)
+  if (vec[regnum] == nullptr)
     {
       gdbpy_ref <register_descriptor_object> reg
 	(PyObject_New (register_descriptor_object,
@@ -176,12 +176,12 @@ gdbpy_get_register_descriptor (struct gdbarch *gdbarch,
 	return NULL;
       reg->regnum = regnum;
       reg->gdbarch = gdbarch;
-      vec->at (regnum) = gdbpy_ref<> ((PyObject *) reg.release ());
+      vec[regnum] = gdbpy_ref<> ((PyObject *) reg.release ());
     }
 
   /* Grab the register descriptor from the vector, the reference count is
      automatically incremented thanks to gdbpy_ref.  */
-  return vec->at (regnum);
+  return vec[regnum];
 }
 
 /* Convert the register descriptor to a string.  */
Pedro Alves July 22, 2020, 2:32 p.m. | #3
On 7/22/20 3:05 PM, Andrew Burgess wrote:
> * Pedro Alves <pedro@palves.net> [2020-07-22 14:10:17 +0100]:

> 

>> Hi Andrew,

>>


Hi Andrew,

> Thanks for your feedback.  As this patch was already merged, the patch

> below applies on top of current master to address these issues.


Sorry, I didn't realize it was already in.

> 

> If I don't hear anything I'll apply this in a couple of days.


It looks great.  Please merge it.

Thanks!
Andrew Burgess July 22, 2020, 3:10 p.m. | #4
* Pedro Alves <pedro@palves.net> [2020-07-22 15:32:40 +0100]:

> On 7/22/20 3:05 PM, Andrew Burgess wrote:

> > * Pedro Alves <pedro@palves.net> [2020-07-22 14:10:17 +0100]:

> > 

> >> Hi Andrew,

> >>

> 

> Hi Andrew,

> 

> > Thanks for your feedback.  As this patch was already merged, the patch

> > below applies on top of current master to address these issues.

> 

> Sorry, I didn't realize it was already in.


Not a problem.  Always happy to get feedback on how to improve
patches.

> 

> > 

> > If I don't hear anything I'll apply this in a couple of days.

> 

> It looks great.  Please merge it.


Done.

Thanks,
Andrew

Patch

diff --git a/gdb/python/py-registers.c b/gdb/python/py-registers.c
index db0fe37eecb..8e22a919d87 100644
--- a/gdb/python/py-registers.c
+++ b/gdb/python/py-registers.c
@@ -24,6 +24,9 @@ 
 #include "reggroups.h"
 #include "python-internal.h"
 
+/* Token to access per-gdbarch data related to register descriptors.  */
+static struct gdbarch_data *gdbpy_register_object_data = NULL;
+
 /* Structure for iterator over register descriptors.  */
 typedef struct {
   PyObject_HEAD
@@ -81,6 +84,17 @@  typedef struct {
 extern PyTypeObject reggroup_object_type
     CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("reggroup_object");
 
+/* Associates a vector of gdb.RegisterDescriptor objects with GDBARCH as
+   gdbarch_data via the gdbarch post init registration mechanism
+   (gdbarch_data_register_post_init).  */
+
+static void *
+gdbpy_register_object_data_init (struct gdbarch *gdbarch)
+{
+  std::vector<gdbpy_ref<>> *vec = new (std::vector<gdbpy_ref<>>);
+  return (void *) vec;
+}
+
 /* Create a new gdb.RegisterGroup object wrapping REGGROUP.  */
 
 static PyObject *
@@ -117,20 +131,38 @@  gdbpy_reggroup_name (PyObject *self, void *closure)
   return gdbpy_reggroup_to_string (self);
 }
 
-/* Create an return a new gdb.RegisterDescriptor object.  */
-static PyObject *
-gdbpy_new_register_descriptor (struct gdbarch *gdbarch,
+/* Return a gdb.RegisterDescriptor object for REGNUM from GDBARCH.  For
+   each REGNUM (in GDBARCH) only one descriptor is ever created, which is
+   then cached on the GDBARCH.  */
+
+static gdbpy_ref<>
+gdbpy_get_register_descriptor (struct gdbarch *gdbarch,
 			       int regnum)
 {
-  /* Create a new object and fill in its details.  */
-  register_descriptor_object *reg
-    = PyObject_New (register_descriptor_object,
-		    &register_descriptor_object_type);
-  if (reg == NULL)
-    return NULL;
-  reg->regnum = regnum;
-  reg->gdbarch = gdbarch;
-  return (PyObject *) reg;
+  auto vec = (std::vector<gdbpy_ref<>> *) gdbarch_data
+    (gdbarch, gdbpy_register_object_data);
+
+  /* Ensure that we have enough entries in the vector.  */
+  if (vec->size () <= regnum)
+    vec->resize ((regnum + 1), nullptr);
+
+  /* If we don't already have a descriptor for REGNUM in GDBARCH then
+     create one now.  */
+  if (vec->at (regnum) == nullptr)
+    {
+      gdbpy_ref <register_descriptor_object> reg
+	(PyObject_New (register_descriptor_object,
+		       &register_descriptor_object_type));
+      if (reg == NULL)
+	return NULL;
+      reg->regnum = regnum;
+      reg->gdbarch = gdbarch;
+      vec->at (regnum) = gdbpy_ref<> ((PyObject *) reg.release ());
+    }
+
+  /* Grab the register descriptor from the vector, the reference count is
+     automatically incremented thanks to gdbpy_ref.  */
+  return vec->at (regnum);
 }
 
 /* Convert the register descriptor to a string.  */
@@ -281,7 +313,7 @@  gdbpy_register_descriptor_iter_next (PyObject *self)
       iter_obj->regnum++;
 
       if (name != nullptr && *name != '\0')
-	return gdbpy_new_register_descriptor (gdbarch, regnum);
+	return gdbpy_get_register_descriptor (gdbarch, regnum).release ();
     }
   while (true);
 }
@@ -291,6 +323,9 @@  gdbpy_register_descriptor_iter_next (PyObject *self)
 int
 gdbpy_initialize_registers ()
 {
+  gdbpy_register_object_data
+    = gdbarch_data_register_post_init (gdbpy_register_object_data_init);
+
   register_descriptor_object_type.tp_new = PyType_GenericNew;
   if (PyType_Ready (&register_descriptor_object_type) < 0)
     return -1;
diff --git a/gdb/testsuite/gdb.python/py-arch-reg-names.exp b/gdb/testsuite/gdb.python/py-arch-reg-names.exp
index 14bc0a822a4..8dd34ef5fd2 100644
--- a/gdb/testsuite/gdb.python/py-arch-reg-names.exp
+++ b/gdb/testsuite/gdb.python/py-arch-reg-names.exp
@@ -85,3 +85,22 @@  for { set i 0 } { $i < [llength $regs] } { incr i } {
     }
 }
 gdb_assert { $found_non_match == 0 } "all registers match"
+
+# Check that we get the same register descriptors from two different
+# iterators.
+gdb_py_test_silent_cmd \
+    "python iter1 = arch.registers ()" \
+    "get first all register iterator" 0
+gdb_py_test_silent_cmd \
+    "python iter2 = arch.registers ()" \
+    "get second all register iterator" 0
+gdb_py_test_silent_cmd \
+    [multi_line_input \
+	 "python" \
+	 "for r1, r2 in zip(iter1, iter2):" \
+	 "  if (r1.name != r2.name):"\
+	 "    raise gdb.GdbError (\"miss-matched names\")" \
+	 "  if (r1 != r2):" \
+	 "    raise gdb.GdbError (\"miss-matched objects\")" \
+	 "\004" ] \
+    "check names and objects match" 1