diff mbox

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

Message ID BA53D49A-6B07-48DA-8C25-C5092C865B10@arm.com
State New
Headers show

Commit Message

Alan Hayward March 23, 2018, 2:54 p.m. UTC 
> 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:

Comments

Simon Marchi March 23, 2018, 3:35 p.m. UTC | #1 
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. UTC | #2 
> 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. UTC | #3 
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
diff mbox

Patch

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;