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;

