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

[Pedro Alves]

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.