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

[Andrew Burgess <andrew.burgess@embecosm.com>]

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