[v4,02/10] Make gdbserver reg_defs a vector of objects

Message ID 20180322084429.26250-3-alan.hayward@arm.com
State New
Headers show
Series
  • Remove gdbserver dependency on xml files
Related show

Commit Message

Alan Hayward March 22, 2018, 8:44 a.m.
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(-)

-- 
2.14.3 (Apple Git-98)

Comments

Simon Marchi March 22, 2018, 9:33 p.m. | #1
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
Alan Hayward March 23, 2018, 2:54 p.m. | #2
> On 22 Mar 2018, at 21:33, Simon Marchi <simon.marchi@ericsson.com> wrote:

> 

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

> 


Ok, I’ve pushed this bit as requested:

diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
index 1bb15900dd..d6511fda65 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 pointer 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

@@ -244,24 +251,13 @@ 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, reg->name) == 0)
+      if (strcmp (name, find_register_by_number (tdesc, i)->name) == 0)
        return i;
     }
   internal_error (__FILE__, __LINE__, "Unknown register %s requested",
                  name);
 }

-#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)
 {
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);


>> }

>> 

>> -#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())); }

> 


Didn’t realise that std:vector worked like this. Agreed with the change.

>> 	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);


I need to remember to think in C++ and not C. This way is much nicer/simpler.

> 

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


Yes, in the old code, if you give it a smaller regnum, vector wouldn’t have been resized,
and then the assert would have fired.
I’ll move the assert to the top of the function to ensure it doesn’t resize smaller.

New version updated with all of the above.
Checked this on X86 with make check on target board gdbserver.
(Patch 3/10, and possibly others, will need updating too, but should be obvious).

Thanks for the review.

Alan.


gdb/
	* regformats/regdef.h (reg): Add constructors.

gdb/gdbserver/
	* regcache.c (find_register_by_number): Return a ref.
	(find_regno): Use references.
	(register_size): Likewise.
	(register_data): Likewise.
	* tdesc.c (target_desc::~target_desc): Remove free calls.
	(target_desc::operator==): Use std::vector compare.
	(init_target_desc): Use reference.
	(tdesc_create_reg): Use reg constructors.
	* tdesc.h (struct target_desc): Replace pointer with object.


diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
index d6511fda650ca52688cd4d7db1acd94e822a3c0d..cbdf766df2c2b95f605198c617ffead9f9ac3775 100644
--- a/gdb/gdbserver/regcache.c
+++ b/gdb/gdbserver/regcache.c
@@ -196,9 +196,9 @@ regcache_cpy (struct regcache *dst, struct regcache *src)
   dst->registers_valid = src->registers_valid;
 }

-/* Return a pointer to the description of register N.  */
+/* Return a reference to the description of register N.  */

-static const struct reg *
+static const struct reg &
 find_register_by_number (const struct target_desc *tdesc, int n)
 {
   return tdesc->reg_defs[n];
@@ -251,7 +251,7 @@ find_regno (const struct target_desc *tdesc, const char *name)
 {
   for (int i = 0; i < tdesc->reg_defs.size (); ++i)
     {
-      if (strcmp (name, find_register_by_number (tdesc, i)->name) == 0)
+      if (strcmp (name, find_register_by_number (tdesc, i).name) == 0)
 	return i;
     }
   internal_error (__FILE__, __LINE__, "Unknown register %s requested",
@@ -288,7 +288,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.  */
@@ -303,7 +303,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/tdesc.h b/gdb/gdbserver/tdesc.h
index 4513ea74232a456cc86eb9a655904012ff117373..a62544341cd23a9a8ec6833e1eae73616a315d2d 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;
diff --git a/gdb/gdbserver/tdesc.c b/gdb/gdbserver/tdesc.c
index e50a848e2f9f280a84ab139cfce4d1f17bd05884..877dbdaecac43f42c031dffafe2bd9f01b577038 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);

@@ -40,18 +37,9 @@ target_desc::~target_desc ()

 bool target_desc::operator== (const target_desc &other) const
 {
-  if (reg_defs.size () != other.reg_defs.size ())
+  if (reg_defs != other.reg_defs)
     return false;

-  for (int i = 0; i < reg_defs.size (); ++i)
-    {
-      struct reg *reg = reg_defs[i];
-      struct reg *reg2 = other.reg_defs[i];
-
-      if (reg != reg2 && *reg != *reg2)
-	return false;
-    }
-
   /* Compare expedite_regs.  */
   int i = 0;
   for (; expedite_regs[i] != NULL; i++)
@@ -72,10 +60,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;
@@ -241,23 +229,12 @@ tdesc_create_reg (struct tdesc_feature *feature, const char *name,
 {
   struct target_desc *tdesc = (struct target_desc *) feature;

-  while (tdesc->reg_defs.size () < regnum)
-    {
-      struct reg *reg = XCNEW (struct reg);
-
-      reg->name = "";
-      reg->size = 0;
-      tdesc->reg_defs.push_back (reg);
-    }
-
-  gdb_assert (regnum == 0
-	      || regnum == tdesc->reg_defs.size ());
+  gdb_assert (regnum == 0 || regnum >= tdesc->reg_defs.size ());

-  struct reg *reg = XCNEW (struct reg);
+  if (regnum != 0)
+    tdesc->reg_defs.resize (regnum);

-  reg->name = name;
-  reg->size = bitsize;
-  tdesc->reg_defs.push_back (reg);
+  tdesc->reg_defs.emplace_back (name, 0, bitsize);
 }

 /* See common/tdesc.h.  */
diff --git a/gdb/regformats/regdef.h b/gdb/regformats/regdef.h
index 262d03c0785f48e83b784b6177c52e2d253a6067..1f7861fd98dd9085c3fe9d7ee4b8396999060265 100644
--- a/gdb/regformats/regdef.h
+++ b/gdb/regformats/regdef.h
@@ -21,6 +21,18 @@

 struct reg
 {
+  reg ()
+    : name (""),
+      offset (0),
+      size (0)
+  {}
+
+  reg (const char *_name, int _offset, int _size)
+    : name (_name),
+      offset (_offset),
+      size (_size)
+  {}
+
   /* The name of this register - NULL for pad entries.  */
   const char *name;
Simon Marchi March 23, 2018, 3:35 p.m. | #3
On 2018-03-23 10:54, Alan Hayward wrote:
>> 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.

>> 

> 

> Ok, I’ve pushed this bit as requested:


Thanks!

> New version updated with all of the above.

> Checked this on X86 with make check on target board gdbserver.

> (Patch 3/10, and possibly others, will need updating too, but should

> be obvious).

> 

> Thanks for the review.

> 

> Alan.


I had some problems applying your patch, the tabs were replaced with 
spaces.  You can either fix the settings of your email client, or you 
can also use git-send-email when sending individual patch updates, like 
this:

   git send-email HEAD^ --subject-prefix="PATCH v4.1 02/10" --to <to> 
--in-reply-to <message-id>

where message-id can be found in the headers of the message you want to 
reply to (header Message-ID).  This allows the message to be correctly 
threaded.  It means the patch will be in a separate email than your 
response to the reviewer's comments, but I think that's fine.

Anyway, this particular one wasn't too difficult to fix up by hand.  It 
LGTM with one nit fixed:

> diff --git a/gdb/regformats/regdef.h b/gdb/regformats/regdef.h

> index

> 262d03c0785f48e83b784b6177c52e2d253a6067..1f7861fd98dd9085c3fe9d7ee4b8396999060265

> 100644

> --- a/gdb/regformats/regdef.h

> +++ b/gdb/regformats/regdef.h

> @@ -21,6 +21,18 @@

> 

>  struct reg

>  {

> +  reg ()

> +    : name (""),

> +      offset (0),

> +      size (0)

> +  {}

> +

> +  reg (const char *_name, int _offset, int _size)


The offset value is always 0 initially, so you can remove it and 
initialize it to 0.

I think that this patch can also be pushed on its own, it's a good 
improvement regardless of the rest of the series.

Thanks,

Simon
Alan Hayward March 23, 2018, 4:52 p.m. | #4
> On 23 Mar 2018, at 15:35, Simon Marchi <simon.marchi@polymtl.ca> wrote:

> 

> On 2018-03-23 10:54, Alan Hayward wrote:

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

>> Ok, I’ve pushed this bit as requested:

> 

> Thanks!

> 

>> New version updated with all of the above.

>> Checked this on X86 with make check on target board gdbserver.

>> (Patch 3/10, and possibly others, will need updating too, but should

>> be obvious).

>> Thanks for the review.

>> Alan.

> 

> I had some problems applying your patch, the tabs were replaced with spaces.  You can either fix the settings of your email client, or you can also use git-send-email when sending individual patch updates, like this:

> 

>  git send-email HEAD^ --subject-prefix="PATCH v4.1 02/10" --to <to> --in-reply-to <message-id>

> 

> where message-id can be found in the headers of the message you want to reply to (header Message-ID).  This allows the message to be correctly threaded.  It means the patch will be in a separate email than your response to the reviewer's comments, but I think that's fine.

> 

> Anyway, this particular one wasn't too difficult to fix up by hand.  It LGTM with one nit fixed:

> 


Thanks for the tip

>> diff --git a/gdb/regformats/regdef.h b/gdb/regformats/regdef.h

>> index

>> 262d03c0785f48e83b784b6177c52e2d253a6067..1f7861fd98dd9085c3fe9d7ee4b8396999060265

>> 100644

>> --- a/gdb/regformats/regdef.h

>> +++ b/gdb/regformats/regdef.h

>> @@ -21,6 +21,18 @@

>> struct reg

>> {

>> +  reg ()

>> +    : name (""),

>> +      offset (0),

>> +      size (0)

>> +  {}

>> +

>> +  reg (const char *_name, int _offset, int _size)

> 

> The offset value is always 0 initially, so you can remove it and initialize it to 0.

> 


Reason I added offset here was that in "Commonise tdesc_reg” this code will get merged into init_target_desc().
At that point I’ll be creating a reg and setting and offset at the same time.
This was just so that I didn’t need to touch regdef.h again.

I can still remove offset from this patch if you want - given that I’m not using it yet?


> I think that this patch can also be pushed on its own, it's a good improvement regardless of the rest of the series.

> 

> Thanks,

> 

> Simon
Simon Marchi March 23, 2018, 5:04 p.m. | #5
On 2018-03-23 12:52, Alan Hayward wrote:
>> The offset value is always 0 initially, so you can remove it and 

>> initialize it to 0.

>> 

> 

> Reason I added offset here was that in "Commonise tdesc_reg” this code

> will get merged into init_target_desc().

> At that point I’ll be creating a reg and setting and offset at the same 

> time.

> This was just so that I didn’t need to touch regdef.h again.

> 

> I can still remove offset from this patch if you want - given that I’m

> not using it yet?


This it not terribly important, but I think it's better to only add it 
once you really need it.  The proposed patches could change, and the 
parameter could end up unused (not saying that's what will happen here, 
it's just an example).

Simon
Alan Hayward March 26, 2018, 10:50 a.m. | #6
> On 23 Mar 2018, at 17:04, Simon Marchi <simon.marchi@polymtl.ca> wrote:

> 

> On 2018-03-23 12:52, Alan Hayward wrote:

>>> The offset value is always 0 initially, so you can remove it and initialize it to 0.

>> Reason I added offset here was that in "Commonise tdesc_reg” this code

>> will get merged into init_target_desc().

>> At that point I’ll be creating a reg and setting and offset at the same time.

>> This was just so that I didn’t need to touch regdef.h again.

>> I can still remove offset from this patch if you want - given that I’m

>> not using it yet?

> 

> This it not terribly important, but I think it's better to only add it once you really need it.  The proposed patches could change, and the parameter could end up unused (not saying that's what will happen here, it's just an example).

> 

> Simon


Ok, pushed as suggested with offset initialised to 0.

Diff for reference:

diff --git a/gdb/gdbserver/regcache.c b/gdb/gdbserver/regcache.c
index d6511fda650ca52688cd4d7db1acd94e822a3c0d..cbdf766df2c2b95f605198c617ffead9f9ac3775 100644
--- a/gdb/gdbserver/regcache.c
+++ b/gdb/gdbserver/regcache.c
@@ -196,9 +196,9 @@ regcache_cpy (struct regcache *dst, struct regcache *src)
   dst->registers_valid = src->registers_valid;
 }

-/* Return a pointer to the description of register N.  */
+/* Return a reference to the description of register N.  */

-static const struct reg *
+static const struct reg &
 find_register_by_number (const struct target_desc *tdesc, int n)
 {
   return tdesc->reg_defs[n];
@@ -251,7 +251,7 @@ find_regno (const struct target_desc *tdesc, const char *name)
 {
   for (int i = 0; i < tdesc->reg_defs.size (); ++i)
     {
-      if (strcmp (name, find_register_by_number (tdesc, i)->name) == 0)
+      if (strcmp (name, find_register_by_number (tdesc, i).name) == 0)
 	return i;
     }
   internal_error (__FILE__, __LINE__, "Unknown register %s requested",
@@ -288,7 +288,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.  */
@@ -303,7 +303,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/tdesc.h b/gdb/gdbserver/tdesc.h
index 4513ea74232a456cc86eb9a655904012ff117373..a62544341cd23a9a8ec6833e1eae73616a315d2d 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;
diff --git a/gdb/gdbserver/tdesc.c b/gdb/gdbserver/tdesc.c
index e50a848e2f9f280a84ab139cfce4d1f17bd05884..cec7a66f9748f7295462c76fdae3d3e9029ee421 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);

@@ -40,18 +37,9 @@ target_desc::~target_desc ()

 bool target_desc::operator== (const target_desc &other) const
 {
-  if (reg_defs.size () != other.reg_defs.size ())
+  if (reg_defs != other.reg_defs)
     return false;

-  for (int i = 0; i < reg_defs.size (); ++i)
-    {
-      struct reg *reg = reg_defs[i];
-      struct reg *reg2 = other.reg_defs[i];
-
-      if (reg != reg2 && *reg != *reg2)
-	return false;
-    }
-
   /* Compare expedite_regs.  */
   int i = 0;
   for (; expedite_regs[i] != NULL; i++)
@@ -72,10 +60,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;
@@ -241,23 +229,12 @@ tdesc_create_reg (struct tdesc_feature *feature, const char *name,
 {
   struct target_desc *tdesc = (struct target_desc *) feature;

-  while (tdesc->reg_defs.size () < regnum)
-    {
-      struct reg *reg = XCNEW (struct reg);
-
-      reg->name = "";
-      reg->size = 0;
-      tdesc->reg_defs.push_back (reg);
-    }
-
-  gdb_assert (regnum == 0
-	      || regnum == tdesc->reg_defs.size ());
+  gdb_assert (regnum == 0 || regnum >= tdesc->reg_defs.size ());

-  struct reg *reg = XCNEW (struct reg);
+  if (regnum != 0)
+    tdesc->reg_defs.resize (regnum);

-  reg->name = name;
-  reg->size = bitsize;
-  tdesc->reg_defs.push_back (reg);
+  tdesc->reg_defs.emplace_back (name, bitsize);
 }

 /* See common/tdesc.h.  */
diff --git a/gdb/regformats/regdef.h b/gdb/regformats/regdef.h
index 262d03c0785f48e83b784b6177c52e2d253a6067..4775e863e9acc516c0d0ab90baf428604eb967c4 100644
--- a/gdb/regformats/regdef.h
+++ b/gdb/regformats/regdef.h
@@ -21,6 +21,18 @@

 struct reg
 {
+  reg ()
+    : name (""),
+      offset (0),
+      size (0)
+  {}
+
+  reg (const char *_name, int _size)
+    : name (_name),
+      offset (0),
+      size (_size)
+  {}
+
   /* The name of this register - NULL for pad entries.  */
   const char *name;

Patch

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);
 }
 
-#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)
 	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);
 }
 
 /* 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;