Re: [PATCH v4 02/10] Make gdbserver reg_defs a vector of objects

[Simon Marchi <simon.marchi@ericsson.com>]

On 2018-03-22 04:44 AM, alan.hayward@arm.com wrote:
> From: Alan Hayward <alan.hayward@arm.com>
> 
> Following a suggestion from Philipp, this patch changes reg_defs in
> gdbserver tdesc so that it is a vector of objects instead of a vector
> of pointers. Updated uses of reg_defs to use references.
> 
> This change reduces the number of small hunks allocated when building
> reg_defs.
> 
> find_register_by_number() is only used by regcache, therefore I made
> it a static function and moved it earlier in the file so that find_regno()
> could use it. The eventual plan is to replace this with accessor functions
> in a common tdesc class.
> 
> Alan.
> 
> 2018-03-21  Alan Hayward  <alan.hayward@arm.com>
> 
> gdb/gdbserver/
> 	* regcache.c (find_register_by_number): Make static and return a ref.
> 	(find_regno): Use find_register_by_number
> 	(register_size): Use references.
> 	(register_data): Likewise.
> 	* regcache.h (struct reg): Remove declaration.
> 	* tdesc.c (target_desc::~target_desc): Remove free calls.
> 	(target_desc::operator==): Use references.
> 	(init_target_desc): Likewise.
> 	(tdesc_create_reg): Use references and resize in a single call.
> 	* tdesc.h (struct target_desc): Replace pointer with object.
> ---
>  gdb/gdbserver/regcache.c | 27 +++++++++++----------------
>  gdb/gdbserver/regcache.h |  4 ----
>  gdb/gdbserver/tdesc.c    | 30 ++++++++++++++----------------
>  gdb/gdbserver/tdesc.h    |  2 +-
>  4 files changed, 26 insertions(+), 37 deletions(-)
> 
> diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
> index 1bb15900dd..df5b9e9dd2 100644
> --- a/gdb/gdbserver/regcache.c
> +++ b/gdb/gdbserver/regcache.c
> @@ -196,6 +196,13 @@ regcache_cpy (struct regcache *dst, struct regcache *src)
>    dst->registers_valid = src->registers_valid;
>  }
>  
> +/* Return a reference to the description of register ``n''.  */
> +
> +static const struct reg &
> +find_register_by_number (const struct target_desc *tdesc, int n)
> +{
> +  return tdesc->reg_defs[n];
> +}
>  
>  #ifndef IN_PROCESS_AGENT
>  
> @@ -243,25 +250,13 @@ int
>  find_regno (const struct target_desc *tdesc, const char *name)
>  {
>    for (int i = 0; i < tdesc->reg_defs.size (); ++i)
> -    {
> -      struct reg *reg = tdesc->reg_defs[i];
> +    if (strcmp (name, find_register_by_number (tdesc, i).name) == 0)
> +      return i;
>  
> -      if (strcmp (name, reg->name) == 0)
> -	return i;
> -    }
>    internal_error (__FILE__, __LINE__, "Unknown register %s requested",
>  		  name);

Please keep the curly braces for the for, as shown here:

https://www.gnu.org/prep/standards/html_node/Syntactic-Conventions.html#index-multiple-variables-in-a-line

Can you put the moving of this function/making it static/removing the declaration
in its own patch?  It's pre-approved, with that fixed.

>  }
>  
> -#endif
> -
> -struct reg *
> -find_register_by_number (const struct target_desc *tdesc, int n)
> -{
> -  return tdesc->reg_defs[n];
> -}
> -
> -#ifndef IN_PROCESS_AGENT
>  static void
>  free_register_cache_thread (struct thread_info *thread)
>  {
> @@ -292,7 +287,7 @@ register_cache_size (const struct target_desc *tdesc)
>  int
>  register_size (const struct target_desc *tdesc, int n)
>  {
> -  return find_register_by_number (tdesc, n)->size / 8;
> +  return find_register_by_number (tdesc, n).size / 8;
>  }
>  
>  /* See common/common-regcache.h.  */
> @@ -307,7 +302,7 @@ static unsigned char *
>  register_data (struct regcache *regcache, int n, int fetch)
>  {
>    return (regcache->registers
> -	  + find_register_by_number (regcache->tdesc, n)->offset / 8);
> +	  + find_register_by_number (regcache->tdesc, n).offset / 8);
>  }
>  
>  /* Supply register N, whose contents are stored in BUF, to REGCACHE.
> diff --git a/gdb/gdbserver/regcache.h b/gdb/gdbserver/regcache.h
> index 3a75ce3fe1..6ff13084b0 100644
> --- a/gdb/gdbserver/regcache.h
> +++ b/gdb/gdbserver/regcache.h
> @@ -94,10 +94,6 @@ void registers_from_string (struct regcache *regcache, char *buf);
>  
>  void regcache_write_pc (struct regcache *regcache, CORE_ADDR pc);
>  
> -/* Return a pointer to the description of register ``n''.  */
> -
> -struct reg *find_register_by_number (const struct target_desc *tdesc, int n);
> -
>  int register_cache_size (const struct target_desc *tdesc);
>  
>  int register_size (const struct target_desc *tdesc, int n);
> diff --git a/gdb/gdbserver/tdesc.c b/gdb/gdbserver/tdesc.c
> index e50a848e2f..8e68a27c7c 100644
> --- a/gdb/gdbserver/tdesc.c
> +++ b/gdb/gdbserver/tdesc.c
> @@ -25,9 +25,6 @@ target_desc::~target_desc ()
>  {
>    int i;
>  
> -  for (reg *reg : reg_defs)
> -    xfree (reg);
> -
>    xfree ((char *) arch);
>    xfree ((char *) osabi);
>  
> @@ -45,10 +42,10 @@ bool target_desc::operator== (const target_desc &other) const
>  
>    for (int i = 0; i < reg_defs.size (); ++i)
>      {
> -      struct reg *reg = reg_defs[i];
> -      struct reg *reg2 = other.reg_defs[i];
> +      struct reg reg = reg_defs[i];
> +      struct reg reg2 = other.reg_defs[i];
>  
> -      if (reg != reg2 && *reg != *reg2)
> +      if (&reg != &reg2 && reg != reg2)

I don't think the first part, "&reg != &reg", makes sense.  It's comparing
the addresses of the local variables.  Also, that comparison only made
sense as a shortcut when the vector held pointers to dynamically allocated
memory.  So I think it should just be removed.

Actually, now that the vector elements are the objects directly, I think that
this whole for loop could be replaced with:

if (reg_defs != other.reg_defs)
  return false;

The if that compares the vector sizes above that can also be removed, since
it's also done by std::vector::operator==:

  template<typename _Tp, typename _Alloc>
    inline bool
    operator==(const vector<_Tp, _Alloc>& __x, const vector<_Tp, _Alloc>& __y)
    { return (__x.size() == __y.size()
	      && std::equal(__x.begin(), __x.end(), __y.begin())); }

>  	return false;
>      }
>  
> @@ -72,10 +69,10 @@ init_target_desc (struct target_desc *tdesc)
>  {
>    int offset = 0;
>  
> -  for (reg *reg : tdesc->reg_defs)
> +  for (reg &reg : tdesc->reg_defs)
>      {
> -      reg->offset = offset;
> -      offset += reg->size;
> +      reg.offset = offset;
> +      offset += reg.size;
>      }
>  
>    tdesc->registers_size = offset / 8;
> @@ -240,24 +237,25 @@ tdesc_create_reg (struct tdesc_feature *feature, const char *name,
>  		  int bitsize, const char *type)
>  {
>    struct target_desc *tdesc = (struct target_desc *) feature;
> +  int current_size = tdesc->reg_defs.size ();
>  
> -  while (tdesc->reg_defs.size () < regnum)
> -    {
> -      struct reg *reg = XCNEW (struct reg);
> +  tdesc->reg_defs.resize (regnum != 0 ? regnum + 1 : current_size + 1);
>  
> +  while (current_size < regnum)
> +    {
> +      struct reg *reg = &tdesc->reg_defs[current_size];
>        reg->name = "";
>        reg->size = 0;
> -      tdesc->reg_defs.push_back (reg);
> +      current_size++;
>      }
>  
>    gdb_assert (regnum == 0
> -	      || regnum == tdesc->reg_defs.size ());
> +	      || regnum + 1 == tdesc->reg_defs.size ());
>  
> -  struct reg *reg = XCNEW (struct reg);
> +  struct reg *reg = &tdesc->reg_defs.back ();
>  
>    reg->name = name;
>    reg->size = bitsize;
> -  tdesc->reg_defs.push_back (reg);
>  }

To simplify this further, I would suggest adding a default constructor to reg
that initializes everything to nullptr/0, and adding a constructor that takes
parameters.  The .resize() will only be necessary if regnum != 0 and will
automatically initialize all the unused register slots properly, so the for
loop will be unnecessary:

  if (regnum != 0)
    tdesc->reg_defs.resize (regnum);

The register can then be added with

  tdesc->reg_defs.emplace_back (name, bitsize);

One behavior that might change compared to the previous code is when trying to
add a register with a regnum smaller than the current size of the vector.  I
think that the old code would have asserted, but the new code would just
accept it and resize the vector to a smaller size.  Could you verify?  In that
case, maybe we should move the gdb_assert to the beginning of the function:

  gdb_assert (regnum == 0 || regnum >= tdesc->reg_defs.size ())

something like that.  It can also be moved under the "if (regnum != 0)" shown
above.

>  
>  /* See common/tdesc.h.  */
> diff --git a/gdb/gdbserver/tdesc.h b/gdb/gdbserver/tdesc.h
> index 4513ea7423..a62544341c 100644
> --- a/gdb/gdbserver/tdesc.h
> +++ b/gdb/gdbserver/tdesc.h
> @@ -34,7 +34,7 @@ struct target_desc : tdesc_feature
>  {
>    /* A vector of elements of register definitions that
>       describe the inferior's register set.  */
> -  std::vector<struct reg *> reg_defs;
> +  std::vector<struct reg> reg_defs;
>  
>    /* The register cache size, in bytes.  */
>    int registers_size;
> 

Thanks,

Simon