[PATCH] Remove MAX_REGISTER_SIZE from regcache.c

[PATCH] Remove MAX_REGISTER_SIZE from regcache.c

[Alan Hayward]

Also adds functionality to calculate maximum register size.

Cannot remove buffer from regcache_restore becuase we need to keep the
register value if the read fails.

Leaves two asserts check with MAX_REGISTER_SIZE which must be kept
until MAX_REGISTER_SIZE is fully removed from all files.

Tested using make check with board files unix and native-gdbserver.

Ok to commit?

Alan.

2017-02-24  Alan Hayward  <alan.hayward@arm.com>

	* regcache.c (struct regcache_descr): Add max_register_size
	(init_regcache_descr): Calculate maximum register size.
	(max_register_size): New function.
	(regcache_save): Avoid buffer use.
	(regcache_restore): Use vec instead of array.
	(regcache_dump): Avoid buffer use.
	* regcache.h (struct regcache_descr): Add max_register_size
	(max_register_size): New function.


index 59233308f926ebd52db9958cba168daacc77c1ee..a4a47c8f738ee5df69074375a3f169558891cf0e 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -206,6 +206,8 @@ extern struct type *register_type (struct gdbarch *gdbarch, int regnum);

 extern int register_size (struct gdbarch *gdbarch, int regnum);

+/* Return the size of the largest register.  */
+extern long max_register_size (struct gdbarch *gdbarch);

 /* Save/restore a register cache.  The set of registers saved /
    restored into the DST regcache determined by the save_reggroup /
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 31aa1baf7ef69c27c00e45e3c8d4eb3c41dc4203..0da184cb7c6ea3a171139a40676658322e62ad2e 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -73,6 +73,9 @@ struct regcache_descr

   /* Cached table containing the type of each register.  */
   struct type **register_type;
+
+  /* Size of the largest register.  */
+  long max_register_size;
 };

 static void *
@@ -126,6 +129,8 @@ init_regcache_descr (struct gdbarch *gdbarch)
 	descr->register_offset[i] = offset;
 	offset += descr->sizeof_register[i];
 	gdb_assert (MAX_REGISTER_SIZE >= descr->sizeof_register[i]);
+	descr->max_register_size = std::max (descr->max_register_size,
+					     descr->sizeof_register[i]);
       }
     /* Set the real size of the raw register cache buffer.  */
     descr->sizeof_raw_registers = offset;
@@ -136,6 +141,8 @@ init_regcache_descr (struct gdbarch *gdbarch)
 	descr->register_offset[i] = offset;
 	offset += descr->sizeof_register[i];
 	gdb_assert (MAX_REGISTER_SIZE >= descr->sizeof_register[i]);
+	descr->max_register_size = std::max (descr->max_register_size,
+					     descr->sizeof_register[i]);
       }
     /* Set the real size of the readonly register cache buffer.  */
     descr->sizeof_cooked_registers = offset;
@@ -187,6 +194,13 @@ regcache_register_size (const struct regcache *regcache, int n)
   return register_size (get_regcache_arch (regcache), n);
 }

+long
+max_register_size (struct gdbarch *gdbarch)
+{
+  struct regcache_descr *descr = regcache_descr (gdbarch);
+  return descr->max_register_size;
+}
+
 /* The register cache for storing raw register values.  */

 struct regcache
@@ -327,7 +341,6 @@ regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read,
 	       void *src)
 {
   struct gdbarch *gdbarch = dst->descr->gdbarch;
-  gdb_byte buf[MAX_REGISTER_SIZE];
   int regnum;

   /* The DST should be `read-only', if it wasn't then the save would
@@ -346,17 +359,13 @@ regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read,
     {
       if (gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup))
 	{
-	  enum register_status status = cooked_read (src, regnum, buf);
+	  gdb_byte *dst_buf = register_buffer (dst, regnum);
+	  enum register_status status = cooked_read (src, regnum, dst_buf);

-	  if (status == REG_VALID)
-	    memcpy (register_buffer (dst, regnum), buf,
-		    register_size (gdbarch, regnum));
-	  else
+	  if (status != REG_VALID)
 	    {
 	      gdb_assert (status != REG_UNKNOWN);
-
-	      memset (register_buffer (dst, regnum), 0,
-		      register_size (gdbarch, regnum));
+	      memset (dst_buf, 0, register_size (gdbarch, regnum));
 	    }
 	  dst->register_status[regnum] = status;
 	}
@@ -369,7 +378,7 @@ regcache_restore (struct regcache *dst,
 		  void *cooked_read_context)
 {
   struct gdbarch *gdbarch = dst->descr->gdbarch;
-  gdb_byte buf[MAX_REGISTER_SIZE];
+  std::vector<gdb_byte> buf (max_register_size (gdbarch));
   int regnum;

   /* The dst had better not be read-only.  If it is, the `restore'
@@ -385,9 +394,9 @@ regcache_restore (struct regcache *dst,
 	{
 	  enum register_status status;

-	  status = cooked_read (cooked_read_context, regnum, buf);
+	  status = cooked_read (cooked_read_context, regnum, buf.data ());
 	  if (status == REG_VALID)
-	    regcache_cooked_write (dst, regnum, buf);
+	    regcache_cooked_write (dst, regnum, buf.data ());
 	}
     }
 }
@@ -1324,7 +1333,6 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
   int footnote_register_offset = 0;
   int footnote_register_type_name_null = 0;
   long register_offset = 0;
-  gdb_byte buf[MAX_REGISTER_SIZE];

 #if 0
   fprintf_unfiltered (file, "nr_raw_registers %d\n",
@@ -1451,8 +1459,8 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
 	    fprintf_unfiltered (file, "<unavailable>");
 	  else
 	    {
-	      regcache_raw_read (regcache, regnum, buf);
-	      print_hex_chars (file, buf,
+	      regcache_raw_update (regcache, regnum);
+	      print_hex_chars (file, register_buffer (regcache, regnum),
 			       regcache->descr->sizeof_register[regnum],
 			       gdbarch_byte_order (gdbarch));
 	    }
@@ -1465,17 +1473,19 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
 	    fprintf_unfiltered (file, "Cooked value");
 	  else
 	    {
-	      enum register_status status;
+	      struct value *value = regcache_cooked_read_value (regcache,
+								regnum);

-	      status = regcache_cooked_read (regcache, regnum, buf);
-	      if (status == REG_UNKNOWN)
-		fprintf_unfiltered (file, "<invalid>");
-	      else if (status == REG_UNAVAILABLE)
+	      if (value_optimized_out (value)
+		  || !value_entirely_available (value))
 		fprintf_unfiltered (file, "<unavailable>");
 	      else
-		print_hex_chars (file, buf,
+		print_hex_chars (file, value_contents_all (value),
 				 regcache->descr->sizeof_register[regnum],
 				 gdbarch_byte_order (gdbarch));
+
+	      release_value (value);
+	      value_free (value);
 	    }
 	}

Re: [PATCH] Remove MAX_REGISTER_SIZE from regcache.c

[Alan Hayward]

Ping.

> On 24 Feb 2017, at 10:19, Alan Hayward <Alan.Hayward@arm.com> wrote:
> 
> Also adds functionality to calculate maximum register size.
> 
> Cannot remove buffer from regcache_restore becuase we need to keep the
> register value if the read fails.
> 
> Leaves two asserts check with MAX_REGISTER_SIZE which must be kept
> until MAX_REGISTER_SIZE is fully removed from all files.
> 
> Tested using make check with board files unix and native-gdbserver.
> 
> Ok to commit?
> 
> Alan.
> 
> 2017-02-24  Alan Hayward  <alan.hayward@arm.com>
> 
> 	* regcache.c (struct regcache_descr): Add max_register_size
> 	(init_regcache_descr): Calculate maximum register size.
> 	(max_register_size): New function.
> 	(regcache_save): Avoid buffer use.
> 	(regcache_restore): Use vec instead of array.
> 	(regcache_dump): Avoid buffer use.
> 	* regcache.h (struct regcache_descr): Add max_register_size
> 	(max_register_size): New function.
> 
> 
> index 59233308f926ebd52db9958cba168daacc77c1ee..a4a47c8f738ee5df69074375a3f169558891cf0e 100644
> --- a/gdb/regcache.h
> +++ b/gdb/regcache.h
> @@ -206,6 +206,8 @@ extern struct type *register_type (struct gdbarch *gdbarch, int regnum);
> 
> extern int register_size (struct gdbarch *gdbarch, int regnum);
> 
> +/* Return the size of the largest register.  */
> +extern long max_register_size (struct gdbarch *gdbarch);
> 
> /* Save/restore a register cache.  The set of registers saved /
>    restored into the DST regcache determined by the save_reggroup /
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 31aa1baf7ef69c27c00e45e3c8d4eb3c41dc4203..0da184cb7c6ea3a171139a40676658322e62ad2e 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -73,6 +73,9 @@ struct regcache_descr
> 
>   /* Cached table containing the type of each register.  */
>   struct type **register_type;
> +
> +  /* Size of the largest register.  */
> +  long max_register_size;
> };
> 
> static void *
> @@ -126,6 +129,8 @@ init_regcache_descr (struct gdbarch *gdbarch)
> 	descr->register_offset[i] = offset;
> 	offset += descr->sizeof_register[i];
> 	gdb_assert (MAX_REGISTER_SIZE >= descr->sizeof_register[i]);
> +	descr->max_register_size = std::max (descr->max_register_size,
> +					     descr->sizeof_register[i]);
>       }
>     /* Set the real size of the raw register cache buffer.  */
>     descr->sizeof_raw_registers = offset;
> @@ -136,6 +141,8 @@ init_regcache_descr (struct gdbarch *gdbarch)
> 	descr->register_offset[i] = offset;
> 	offset += descr->sizeof_register[i];
> 	gdb_assert (MAX_REGISTER_SIZE >= descr->sizeof_register[i]);
> +	descr->max_register_size = std::max (descr->max_register_size,
> +					     descr->sizeof_register[i]);
>       }
>     /* Set the real size of the readonly register cache buffer.  */
>     descr->sizeof_cooked_registers = offset;
> @@ -187,6 +194,13 @@ regcache_register_size (const struct regcache *regcache, int n)
>   return register_size (get_regcache_arch (regcache), n);
> }
> 
> +long
> +max_register_size (struct gdbarch *gdbarch)
> +{
> +  struct regcache_descr *descr = regcache_descr (gdbarch);
> +  return descr->max_register_size;
> +}
> +
> /* The register cache for storing raw register values.  */
> 
> struct regcache
> @@ -327,7 +341,6 @@ regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read,
> 	       void *src)
> {
>   struct gdbarch *gdbarch = dst->descr->gdbarch;
> -  gdb_byte buf[MAX_REGISTER_SIZE];
>   int regnum;
> 
>   /* The DST should be `read-only', if it wasn't then the save would
> @@ -346,17 +359,13 @@ regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read,
>     {
>       if (gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup))
> 	{
> -	  enum register_status status = cooked_read (src, regnum, buf);
> +	  gdb_byte *dst_buf = register_buffer (dst, regnum);
> +	  enum register_status status = cooked_read (src, regnum, dst_buf);
> 
> -	  if (status == REG_VALID)
> -	    memcpy (register_buffer (dst, regnum), buf,
> -		    register_size (gdbarch, regnum));
> -	  else
> +	  if (status != REG_VALID)
> 	    {
> 	      gdb_assert (status != REG_UNKNOWN);
> -
> -	      memset (register_buffer (dst, regnum), 0,
> -		      register_size (gdbarch, regnum));
> +	      memset (dst_buf, 0, register_size (gdbarch, regnum));
> 	    }
> 	  dst->register_status[regnum] = status;
> 	}
> @@ -369,7 +378,7 @@ regcache_restore (struct regcache *dst,
> 		  void *cooked_read_context)
> {
>   struct gdbarch *gdbarch = dst->descr->gdbarch;
> -  gdb_byte buf[MAX_REGISTER_SIZE];
> +  std::vector<gdb_byte> buf (max_register_size (gdbarch));
>   int regnum;
> 
>   /* The dst had better not be read-only.  If it is, the `restore'
> @@ -385,9 +394,9 @@ regcache_restore (struct regcache *dst,
> 	{
> 	  enum register_status status;
> 
> -	  status = cooked_read (cooked_read_context, regnum, buf);
> +	  status = cooked_read (cooked_read_context, regnum, buf.data ());
> 	  if (status == REG_VALID)
> -	    regcache_cooked_write (dst, regnum, buf);
> +	    regcache_cooked_write (dst, regnum, buf.data ());
> 	}
>     }
> }
> @@ -1324,7 +1333,6 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
>   int footnote_register_offset = 0;
>   int footnote_register_type_name_null = 0;
>   long register_offset = 0;
> -  gdb_byte buf[MAX_REGISTER_SIZE];
> 
> #if 0
>   fprintf_unfiltered (file, "nr_raw_registers %d\n",
> @@ -1451,8 +1459,8 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
> 	    fprintf_unfiltered (file, "<unavailable>");
> 	  else
> 	    {
> -	      regcache_raw_read (regcache, regnum, buf);
> -	      print_hex_chars (file, buf,
> +	      regcache_raw_update (regcache, regnum);
> +	      print_hex_chars (file, register_buffer (regcache, regnum),
> 			       regcache->descr->sizeof_register[regnum],
> 			       gdbarch_byte_order (gdbarch));
> 	    }
> @@ -1465,17 +1473,19 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
> 	    fprintf_unfiltered (file, "Cooked value");
> 	  else
> 	    {
> -	      enum register_status status;
> +	      struct value *value = regcache_cooked_read_value (regcache,
> +								regnum);
> 
> -	      status = regcache_cooked_read (regcache, regnum, buf);
> -	      if (status == REG_UNKNOWN)
> -		fprintf_unfiltered (file, "<invalid>");
> -	      else if (status == REG_UNAVAILABLE)
> +	      if (value_optimized_out (value)
> +		  || !value_entirely_available (value))
> 		fprintf_unfiltered (file, "<unavailable>");
> 	      else
> -		print_hex_chars (file, buf,
> +		print_hex_chars (file, value_contents_all (value),
> 				 regcache->descr->sizeof_register[regnum],
> 				 gdbarch_byte_order (gdbarch));
> +
> +	      release_value (value);
> +	      value_free (value);
> 	    }
> 	}
> 

Re: [PATCH] Remove MAX_REGISTER_SIZE from regcache.c

[Yao Qi]

Alan Hayward <Alan.Hayward@arm.com> writes:

> @@ -126,6 +129,8 @@ init_regcache_descr (struct gdbarch *gdbarch)
>  	descr->register_offset[i] = offset;
>  	offset += descr->sizeof_register[i];
>  	gdb_assert (MAX_REGISTER_SIZE >= descr->sizeof_register[i]);

Do we still need to keep MAX_REGISTER_SIZE? or you plan to remove it later.

> +	descr->max_register_size = std::max (descr->max_register_size,
> +					     descr->sizeof_register[i]);
>        }
>      /* Set the real size of the raw register cache buffer.  */
>      descr->sizeof_raw_registers = offset;

> @@ -1465,17 +1473,19 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
>  	    fprintf_unfiltered (file, "Cooked value");
>  	  else
>  	    {
> -	      enum register_status status;
> +	      struct value *value = regcache_cooked_read_value (regcache,
> +								regnum);
>
> -	      status = regcache_cooked_read (regcache, regnum, buf);
> -	      if (status == REG_UNKNOWN)
> -		fprintf_unfiltered (file, "<invalid>");

"<invalid>" is lost after your patch.

> -	      else if (status == REG_UNAVAILABLE)
> +	      if (value_optimized_out (value)
> +		  || !value_entirely_available (value))
>  		fprintf_unfiltered (file, "<unavailable>");
>  	      else
> -		print_hex_chars (file, buf,
> +		print_hex_chars (file, value_contents_all (value),
>  				 regcache->descr->sizeof_register[regnum],
>  				 gdbarch_byte_order (gdbarch));
> +
> +	      release_value (value);
> +	      value_free (value);

-- 
Yao (齐尧)

Re: [PATCH] Remove MAX_REGISTER_SIZE from regcache.c

[Alan Hayward]

> On 24 Mar 2017, at 08:49, Yao Qi <qiyaoltc@gmail.com> wrote:
> 
> Alan Hayward <Alan.Hayward@arm.com> writes:
> 
>> @@ -126,6 +129,8 @@ init_regcache_descr (struct gdbarch *gdbarch)
>> 	descr->register_offset[i] = offset;
>> 	offset += descr->sizeof_register[i];
>> 	gdb_assert (MAX_REGISTER_SIZE >= descr->sizeof_register[i]);
> 
> Do we still need to keep MAX_REGISTER_SIZE? or you plan to remove it later.
> 

I planned to keep it here until I remove MAX_REGISTER_SIZE.
Whilst the define is still in use, we should really keep this check here.


>> +	descr->max_register_size = std::max (descr->max_register_size,
>> +					     descr->sizeof_register[i]);
>>       }
>>     /* Set the real size of the raw register cache buffer.  */
>>     descr->sizeof_raw_registers = offset;
> 
>> @@ -1465,17 +1473,19 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
>> 	    fprintf_unfiltered (file, "Cooked value");
>> 	  else
>> 	    {
>> -	      enum register_status status;
>> +	      struct value *value = regcache_cooked_read_value (regcache,
>> +								regnum);
>> 
>> -	      status = regcache_cooked_read (regcache, regnum, buf);
>> -	      if (status == REG_UNKNOWN)
>> -		fprintf_unfiltered (file, "<invalid>");
> 
> "<invalid>" is lost after your patch.

Yes. There seems to be no way of getting an UNKNOWN status when reading using values.

Looking into the code, only msp430 and nds32 seem to explicitly set status to UNKNOWN.
Everything else looks like it will error instead of setting UNKNOWN

I could add "if (regcache_register_status (regcache, regnum) == REG_UNKNOWN)"
before calling regcache_cooked_read_value. But I think that’s not what we want, as we
want to always try reading from the target.


> 
>> -	      else if (status == REG_UNAVAILABLE)
>> +	      if (value_optimized_out (value)
>> +		  || !value_entirely_available (value))
>> 		fprintf_unfiltered (file, "<unavailable>");
>> 	      else
>> -		print_hex_chars (file, buf,
>> +		print_hex_chars (file, value_contents_all (value),
>> 				 regcache->descr->sizeof_register[regnum],
>> 				 gdbarch_byte_order (gdbarch));
>> +
>> +	      release_value (value);
>> +	      value_free (value);
> 
> -- 
> Yao (齐尧)


Alan.

Re: [PATCH] Remove MAX_REGISTER_SIZE from regcache.c

[Alan Hayward]

> On 24 Mar 2017, at 10:27, Alan Hayward <Alan.Hayward@arm.com> wrote:
> 
>> 
>> On 24 Mar 2017, at 08:49, Yao Qi <qiyaoltc@gmail.com> wrote:
>> 
>> Alan Hayward <Alan.Hayward@arm.com> writes:
>> 
>>> @@ -126,6 +129,8 @@ init_regcache_descr (struct gdbarch *gdbarch)
>>> 	descr->register_offset[i] = offset;
>>> 	offset += descr->sizeof_register[i];
>>> 	gdb_assert (MAX_REGISTER_SIZE >= descr->sizeof_register[i]);
>> 
>> Do we still need to keep MAX_REGISTER_SIZE? or you plan to remove it later.
>> 
> 
> I planned to keep it here until I remove MAX_REGISTER_SIZE.
> Whilst the define is still in use, we should really keep this check here.
> 
> 
>>> +	descr->max_register_size = std::max (descr->max_register_size,
>>> +					     descr->sizeof_register[i]);
>>>      }
>>>    /* Set the real size of the raw register cache buffer.  */
>>>    descr->sizeof_raw_registers = offset;
>> 
>>> @@ -1465,17 +1473,19 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
>>> 	    fprintf_unfiltered (file, "Cooked value");
>>> 	  else
>>> 	    {
>>> -	      enum register_status status;
>>> +	      struct value *value = regcache_cooked_read_value (regcache,
>>> +								regnum);
>>> 
>>> -	      status = regcache_cooked_read (regcache, regnum, buf);
>>> -	      if (status == REG_UNKNOWN)
>>> -		fprintf_unfiltered (file, "<invalid>");
>> 
>> "<invalid>" is lost after your patch.
> 
> Yes. There seems to be no way of getting an UNKNOWN status when reading using values.
> 
> Looking into the code, only msp430 and nds32 seem to explicitly set status to UNKNOWN.
> Everything else looks like it will error instead of setting UNKNOWN
> 
> I could add "if (regcache_register_status (regcache, regnum) == REG_UNKNOWN)"
> before calling regcache_cooked_read_value. But I think that’s not what we want, as we
> want to always try reading from the target.
> 

Looking at this a little further, it looks that in reality mps430 will
never return REG_UNKNOWN. Also, nds32 has two checks at the top that should
never fire (and cause a REG_UNKNOWN), and a single return where every other
architecture has an error.

In the patch below, I've updated both those files to make them more like
other architectures. Nothing else has changed.

Tested on a --enable-targets=all build using make check with board files
unix and native-gdbserver.

I do not have a mps430 or nds32 machine to test on.

Ok to commit?

Alan.

2017-04-05  Alan Hayward  <alan.hayward@arm.com>

	* gdb/regcache.c (struct regcache_descr): Add max_register_size
	(init_regcache_descr): Calculate maximum register size.
	(max_register_size): New function.
	(regcache_save): Avoid buffer use.
	(regcache_restore): Use vec instead of array.
	(regcache_dump): Avoid buffer use.
	* gdb/regcache.h (struct regcache_descr): Add max_register_size
	(max_register_size): New function.
	* gdb/msp430-tdep.c (msp430_pseudo_register_read): Never return
	REG_UNKNOWN.
	* gdb/nds32-tdep.c (nds32_pseudo_register_read): Likewise.



diff --git a/gdb/msp430-tdep.c b/gdb/msp430-tdep.c
index 75329dfcc5ed94fff19639db4db21dd0874d0e96..d9eebf0cc2647a079db2f822145d0fb74ea301e4 100644
--- a/gdb/msp430-tdep.c
+++ b/gdb/msp430-tdep.c
@@ -221,10 +221,9 @@ msp430_pseudo_register_read (struct gdbarch *gdbarch,
 			     struct regcache *regcache,
 			     int regnum, gdb_byte *buffer)
 {
-  enum register_status status = REG_UNKNOWN;
-
   if (MSP430_NUM_REGS <= regnum && regnum < MSP430_NUM_TOTAL_REGS)
     {
+      enum register_status status;
       ULONGEST val;
       enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
       int regsize = register_size (gdbarch, regnum);
@@ -234,11 +233,10 @@ msp430_pseudo_register_read (struct gdbarch *gdbarch,
       if (status == REG_VALID)
 	store_unsigned_integer (buffer, regsize, byte_order, val);

+      return status;
     }
   else
     gdb_assert_not_reached ("invalid pseudo register number");
-
-  return status;
 }

 /* Implement the "pseudo_register_write" gdbarch method.  */
diff --git a/gdb/nds32-tdep.c b/gdb/nds32-tdep.c
index 05c48aa27d84bc0286712f0143a9447a79ae066b..e9955d60a8132d5bc3af234dff0c86c8e59d4855 100644
--- a/gdb/nds32-tdep.c
+++ b/gdb/nds32-tdep.c
@@ -445,11 +445,11 @@ nds32_pseudo_register_read (struct gdbarch *gdbarch,
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
   gdb_byte reg_buf[8];
   int offset, fdr_regnum;
-  enum register_status status = REG_UNKNOWN;
+  enum register_status status;

   /* Sanity check.  */
-  if (tdep->fpu_freg == -1 || tdep->use_pseudo_fsrs == 0)
-    return status;
+  gdb_assert (tdep->fpu_freg != -1);
+  gdb_assert (tdep->use_pseudo_fsrs > 0);

   regnum -= gdbarch_num_regs (gdbarch);

@@ -466,9 +466,11 @@ nds32_pseudo_register_read (struct gdbarch *gdbarch,
       status = regcache_raw_read (regcache, fdr_regnum, reg_buf);
       if (status == REG_VALID)
 	memcpy (buf, reg_buf + offset, 4);
+
+      return status;
     }

-  return status;
+  gdb_assert_not_reached ("invalid pseudo register number");
 }

 /* Implement the "pseudo_register_write" gdbarch method.  */
diff --git a/gdb/regcache.h b/gdb/regcache.h
index 1dd4127818ba1f6fa5b9e5f2d326843833d42945..f201f0c084f40ccf276a7e1ba19050cbc11208ad 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -215,6 +215,8 @@ extern struct type *register_type (struct gdbarch *gdbarch, int regnum);

 extern int register_size (struct gdbarch *gdbarch, int regnum);

+/* Return the size of the largest register.  */
+extern long max_register_size (struct gdbarch *gdbarch);

 /* Save/restore a register cache.  The set of registers saved /
    restored into the DST regcache determined by the save_reggroup /
diff --git a/gdb/regcache.c b/gdb/regcache.c
index 8bbd3a655c007d649b91ec0e3d2374553990923f..cc621cf248c44d6159b68e01eb130fa5bf176fb3 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -73,6 +73,9 @@ struct regcache_descr

   /* Cached table containing the type of each register.  */
   struct type **register_type;
+
+  /* Size of the largest register.  */
+  long max_register_size;
 };

 static void *
@@ -126,6 +129,8 @@ init_regcache_descr (struct gdbarch *gdbarch)
 	descr->register_offset[i] = offset;
 	offset += descr->sizeof_register[i];
 	gdb_assert (MAX_REGISTER_SIZE >= descr->sizeof_register[i]);
+	descr->max_register_size = std::max (descr->max_register_size,
+					     descr->sizeof_register[i]);
       }
     /* Set the real size of the raw register cache buffer.  */
     descr->sizeof_raw_registers = offset;
@@ -136,6 +141,8 @@ init_regcache_descr (struct gdbarch *gdbarch)
 	descr->register_offset[i] = offset;
 	offset += descr->sizeof_register[i];
 	gdb_assert (MAX_REGISTER_SIZE >= descr->sizeof_register[i]);
+	descr->max_register_size = std::max (descr->max_register_size,
+					     descr->sizeof_register[i]);
       }
     /* Set the real size of the readonly register cache buffer.  */
     descr->sizeof_cooked_registers = offset;
@@ -187,6 +194,13 @@ regcache_register_size (const struct regcache *regcache, int n)
   return register_size (get_regcache_arch (regcache), n);
 }

+long
+max_register_size (struct gdbarch *gdbarch)
+{
+  struct regcache_descr *descr = regcache_descr (gdbarch);
+  return descr->max_register_size;
+}
+
 /* The register cache for storing raw register values.  */

 struct regcache
@@ -337,7 +351,6 @@ regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read,
 	       void *src)
 {
   struct gdbarch *gdbarch = dst->descr->gdbarch;
-  gdb_byte buf[MAX_REGISTER_SIZE];
   int regnum;

   /* The DST should be `read-only', if it wasn't then the save would
@@ -356,17 +369,13 @@ regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read,
     {
       if (gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup))
 	{
-	  enum register_status status = cooked_read (src, regnum, buf);
+	  gdb_byte *dst_buf = register_buffer (dst, regnum);
+	  enum register_status status = cooked_read (src, regnum, dst_buf);

-	  if (status == REG_VALID)
-	    memcpy (register_buffer (dst, regnum), buf,
-		    register_size (gdbarch, regnum));
-	  else
+	  if (status != REG_VALID)
 	    {
 	      gdb_assert (status != REG_UNKNOWN);
-
-	      memset (register_buffer (dst, regnum), 0,
-		      register_size (gdbarch, regnum));
+	      memset (dst_buf, 0, register_size (gdbarch, regnum));
 	    }
 	  dst->register_status[regnum] = status;
 	}
@@ -379,7 +388,7 @@ regcache_restore (struct regcache *dst,
 		  void *cooked_read_context)
 {
   struct gdbarch *gdbarch = dst->descr->gdbarch;
-  gdb_byte buf[MAX_REGISTER_SIZE];
+  std::vector<gdb_byte> buf (max_register_size (gdbarch));
   int regnum;

   /* The dst had better not be read-only.  If it is, the `restore'
@@ -395,9 +404,9 @@ regcache_restore (struct regcache *dst,
 	{
 	  enum register_status status;

-	  status = cooked_read (cooked_read_context, regnum, buf);
+	  status = cooked_read (cooked_read_context, regnum, buf.data ());
 	  if (status == REG_VALID)
-	    regcache_cooked_write (dst, regnum, buf);
+	    regcache_cooked_write (dst, regnum, buf.data ());
 	}
     }
 }
@@ -1339,7 +1348,6 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
   int footnote_register_offset = 0;
   int footnote_register_type_name_null = 0;
   long register_offset = 0;
-  gdb_byte buf[MAX_REGISTER_SIZE];

 #if 0
   fprintf_unfiltered (file, "nr_raw_registers %d\n",
@@ -1466,8 +1474,8 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
 	    fprintf_unfiltered (file, "<unavailable>");
 	  else
 	    {
-	      regcache_raw_read (regcache, regnum, buf);
-	      print_hex_chars (file, buf,
+	      regcache_raw_update (regcache, regnum);
+	      print_hex_chars (file, register_buffer (regcache, regnum),
 			       regcache->descr->sizeof_register[regnum],
 			       gdbarch_byte_order (gdbarch));
 	    }
@@ -1480,17 +1488,19 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
 	    fprintf_unfiltered (file, "Cooked value");
 	  else
 	    {
-	      enum register_status status;
+	      struct value *value = regcache_cooked_read_value (regcache,
+								regnum);

-	      status = regcache_cooked_read (regcache, regnum, buf);
-	      if (status == REG_UNKNOWN)
-		fprintf_unfiltered (file, "<invalid>");
-	      else if (status == REG_UNAVAILABLE)
+	      if (value_optimized_out (value)
+		  || !value_entirely_available (value))
 		fprintf_unfiltered (file, "<unavailable>");
 	      else
-		print_hex_chars (file, buf,
+		print_hex_chars (file, value_contents_all (value),
 				 regcache->descr->sizeof_register[regnum],
 				 gdbarch_byte_order (gdbarch));
+
+	      release_value (value);
+	      value_free (value);
 	    }
 	}

Re: [PATCH] Remove MAX_REGISTER_SIZE from regcache.c

[Yao Qi]

Alan Hayward <Alan.Hayward@arm.com> writes:

> diff --git a/gdb/msp430-tdep.c b/gdb/msp430-tdep.c
> index 75329dfcc5ed94fff19639db4db21dd0874d0e96..d9eebf0cc2647a079db2f822145d0fb74ea301e4 100644
> --- a/gdb/msp430-tdep.c
> +++ b/gdb/msp430-tdep.c
> @@ -221,10 +221,9 @@ msp430_pseudo_register_read (struct gdbarch *gdbarch,
>  			     struct regcache *regcache,
>  			     int regnum, gdb_byte *buffer)
>  {
> -  enum register_status status = REG_UNKNOWN;
> -
>    if (MSP430_NUM_REGS <= regnum && regnum < MSP430_NUM_TOTAL_REGS)
>      {
> +      enum register_status status;
>        ULONGEST val;
>        enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>        int regsize = register_size (gdbarch, regnum);
> @@ -234,11 +233,10 @@ msp430_pseudo_register_read (struct gdbarch *gdbarch,
>        if (status == REG_VALID)
>  	store_unsigned_integer (buffer, regsize, byte_order, val);
>
> +      return status;
>      }
>    else
>      gdb_assert_not_reached ("invalid pseudo register number");
> -
> -  return status;
>  }

This looks reasonable to me, but could you put it into a separate patch
so that people interested in msp430 may take a look?

>
>  /* Implement the "pseudo_register_write" gdbarch method.  */
> diff --git a/gdb/nds32-tdep.c b/gdb/nds32-tdep.c
> index 05c48aa27d84bc0286712f0143a9447a79ae066b..e9955d60a8132d5bc3af234dff0c86c8e59d4855 100644
> --- a/gdb/nds32-tdep.c
> +++ b/gdb/nds32-tdep.c
> @@ -445,11 +445,11 @@ nds32_pseudo_register_read (struct gdbarch *gdbarch,
>    struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>    gdb_byte reg_buf[8];
>    int offset, fdr_regnum;
> -  enum register_status status = REG_UNKNOWN;
> +  enum register_status status;
>
>    /* Sanity check.  */
> -  if (tdep->fpu_freg == -1 || tdep->use_pseudo_fsrs == 0)
> -    return status;
> +  gdb_assert (tdep->fpu_freg != -1);
> +  gdb_assert (tdep->use_pseudo_fsrs > 0);
>

The nds32 change can go to a separate patch as well, so that nds32
people can take a look too.  Secondly, it is not easy to see your change
is right or not, unless I read the code in nds32_gdbarch_init,

  if (fpu_freg == -1)
    num_regs = NDS32_NUM_REGS;
  else if (use_pseudo_fsrs == 1)
    {
      set_gdbarch_pseudo_register_read (gdbarch, nds32_pseudo_register_read);
      set_gdbarch_pseudo_register_write (gdbarch, nds32_pseudo_register_write);

so please add some comments in the assert like

  /* This function is only registered when fpu_regs != -1 and
     use_pseudo_fsrs == 1 in nds32_gdbarch_init.  */
  gdb_assert (tdep->fpu_freg != -1);
  gdb_assert (tdep->use_pseudo_fsrs == 1);

>    regnum -= gdbarch_num_regs (gdbarch);
>
> @@ -466,9 +466,11 @@ nds32_pseudo_register_read (struct gdbarch *gdbarch,
>        status = regcache_raw_read (regcache, fdr_regnum, reg_buf);
>        if (status == REG_VALID)
>  	memcpy (buf, reg_buf + offset, 4);
> +
> +      return status;
>      }
>
> -  return status;
> +  gdb_assert_not_reached ("invalid pseudo register number");
>  }

also, it would be nice to do the same change in
nds32_pseudo_register_write too.

> @@ -379,7 +388,7 @@ regcache_restore (struct regcache *dst,
>  		  void *cooked_read_context)
>  {
>    struct gdbarch *gdbarch = dst->descr->gdbarch;
> -  gdb_byte buf[MAX_REGISTER_SIZE];
> +  std::vector<gdb_byte> buf (max_register_size (gdbarch));
>    int regnum;
>
>    /* The dst had better not be read-only.  If it is, the `restore'
> @@ -395,9 +404,9 @@ regcache_restore (struct regcache *dst,
>  	{
>  	  enum register_status status;

Can we move "buf" here? and initialize it with the register_size,

           std::vector<gdb_byte> buf (register_size (gdbarch, regnum));

then, we don't need max_register_size ().

> @@ -1480,17 +1488,19 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
>  	    fprintf_unfiltered (file, "Cooked value");
>  	  else
>  	    {
> -	      enum register_status status;
> +	      struct value *value = regcache_cooked_read_value (regcache,
> +								regnum);
>
> -	      status = regcache_cooked_read (regcache, regnum, buf);
> -	      if (status == REG_UNKNOWN)
> -		fprintf_unfiltered (file, "<invalid>");
> -	      else if (status == REG_UNAVAILABLE)
> +	      if (value_optimized_out (value)
> +		  || !value_entirely_available (value))
>  		fprintf_unfiltered (file, "<unavailable>");

It is still not right to me.  With your changes to msp430 and nds32, we
won't get REG_UNKNOWN for pseudo registers, but we may still get
REG_UNKNOWN from raw registers (from regcache->register_status[]).  How
is this?

  gdb_byte *buf = NULL;
  enum register_status status;
  struct value * value = NULL;

  if (regnum < regcache->descr->nr_raw_registers)
    {
      regcache_raw_update (regcache, regnum);

      status = regcache->register_status[regnum];
      buf = register_buffer (regcache, regnum);
    }
  else
   {
      value = regcache_cooked_read_value (regcache, regnum);

      if (value_entirely_available (value))
        {
          status = REG_VALID;
          buf = value_contents_all (value);
        }
      else
        status = REG_REG_UNAVAILABLE;
   }

   if (status == REG_UNKNOWN)
      fprintf_unfiltered (file, "<invalid>");
   else if (status == REG_UNAVAILABLE)
      fprintf_unfiltered (file, "<unavailable>");
   else
       print_hex_chars (file, buf,
 			 regcache->descr->sizeof_register[regnum],
 			 gdbarch_byte_order (gdbarch));

   if (value != NULL)
    {
      release_value (value);
      value_free (value);
    }

-- 
Yao (齐尧)

Re: [PATCH] Remove MAX_REGISTER_SIZE from regcache.c

[Alan Hayward]

> On 5 Apr 2017, at 17:00, Yao Qi <qiyaoltc@gmail.com> wrote:

<snip Msp430 and nds32 changes>
Moved these out to a different patch

>> @@ -379,7 +388,7 @@ regcache_restore (struct regcache *dst,
>> 		  void *cooked_read_context)
>> {
>>   struct gdbarch *gdbarch = dst->descr->gdbarch;
>> -  gdb_byte buf[MAX_REGISTER_SIZE];
>> +  std::vector<gdb_byte> buf (max_register_size (gdbarch));
>>   int regnum;
>> 
>>   /* The dst had better not be read-only.  If it is, the `restore'
>> @@ -395,9 +404,9 @@ regcache_restore (struct regcache *dst,
>> 	{
>> 	  enum register_status status;
> 
> Can we move "buf" here? and initialize it with the register_size,
> 
>           std::vector<gdb_byte> buf (register_size (gdbarch, regnum));
> 
> then, we don't need max_register_size ().
> 

Problem with this is that we are then creating a brand new buffer for each
iteration of the loop, which is a little heavyweight.
We could create an empty buf outside the loop and re-size it each iteration,
but that's still going to cost.

We'll still need to keep max_register_size () if we want to add checks
that the FOO_MAX_REGISTER size defines are big enough.
(see the BFIN_MAX_REGISTER_SIZE email thread)



>> @@ -1480,17 +1488,19 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
>> 	    fprintf_unfiltered (file, "Cooked value");
>> 	  else
>> 	    {
>> -	      enum register_status status;
>> +	      struct value *value = regcache_cooked_read_value (regcache,
>> +								regnum);
>> 
>> -	      status = regcache_cooked_read (regcache, regnum, buf);
>> -	      if (status == REG_UNKNOWN)
>> -		fprintf_unfiltered (file, "<invalid>");
>> -	      else if (status == REG_UNAVAILABLE)
>> +	      if (value_optimized_out (value)
>> +		  || !value_entirely_available (value))
>> 		fprintf_unfiltered (file, "<unavailable>");
> 
> It is still not right to me.  With your changes to msp430 and nds32, we
> won't get REG_UNKNOWN for pseudo registers, but we may still get
> REG_UNKNOWN from raw registers (from regcache->register_status[]).  How
> is this?
> 
>  gdb_byte *buf = NULL;
>  enum register_status status;
>  struct value * value = NULL;
> 
>  if (regnum < regcache->descr->nr_raw_registers)
>    {
>      regcache_raw_update (regcache, regnum);
> 
>      status = regcache->register_status[regnum];
>      buf = register_buffer (regcache, regnum);
>    }
>  else
>   {
>      value = regcache_cooked_read_value (regcache, regnum);
> 
>      if (value_entirely_available (value))
>        {
>          status = REG_VALID;
>          buf = value_contents_all (value);
>        }
>      else
>        status = REG_REG_UNAVAILABLE;
>   }
> 
>   if (status == REG_UNKNOWN)
>      fprintf_unfiltered (file, "<invalid>");
>   else if (status == REG_UNAVAILABLE)
>      fprintf_unfiltered (file, "<unavailable>");
>   else
>       print_hex_chars (file, buf,
> 			 regcache->descr->sizeof_register[regnum],
> 			 gdbarch_byte_order (gdbarch));
> 
>   if (value != NULL)
>    {
>      release_value (value);
>      value_free (value);
>    }
> 

Yes, I’ll add those changes.

Re: [PATCH] Remove MAX_REGISTER_SIZE from regcache.c

[Yao Qi]

Alan Hayward <Alan.Hayward@arm.com> writes:

>>> @@ -395,9 +404,9 @@ regcache_restore (struct regcache *dst,
>>> 	{
>>> 	  enum register_status status;
>> 
>> Can we move "buf" here? and initialize it with the register_size,
>> 
>>           std::vector<gdb_byte> buf (register_size (gdbarch, regnum));
>> 
>> then, we don't need max_register_size ().
>> 
>
> Problem with this is that we are then creating a brand new buffer for each
> iteration of the loop, which is a little heavyweight.
> We could create an empty buf outside the loop and re-size it each iteration,
> but that's still going to cost.
>

How is this patch below?  I class-fied regcache last month in my local
tree, and MAX_REGISTER_SIZE is disappeared from regcache.c.  I suggested
using std::vector in a loop so that it is easy to rebase my patches.

-- 
Yao (齐尧)
From 25d562b5f858314ad9d41a7ffa825d8d24e3828e Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Fri, 7 Apr 2017 22:50:27 +0100
Subject: [PATCH] Simplify regcache_restore

This patches removes the 2nd argument of regcache_restore, because it
is only called by regcache_cpy.  In regcache_cpy, if regcache_restore
is called, dst is not readonly, but src is readonly.  So this patch
adds an assert that src is readonly in regcache_restore.
regcache_cook_read read everything from a readonly regcache cache
(src)'s register_buffer, and register status is from src->register_status.

gdb:

2017-04-07  Yao Qi  <yao.qi@linaro.org>

	* regcache.c (regcache_restore): Remove argument 2.  Replace
	argument 3 with regcache.  Get register status from
	src->register_status and get register contents from
	register_buffer (src, regnum).
	(regcache_cpy): Update.

diff --git a/gdb/regcache.c b/gdb/regcache.c
index 37bc2f0..41c23a5 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -374,17 +374,15 @@ regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read,
 }
 
 static void
-regcache_restore (struct regcache *dst,
-		  regcache_cooked_read_ftype *cooked_read,
-		  void *cooked_read_context)
+regcache_restore (struct regcache *dst, struct regcache *src)
 {
   struct gdbarch *gdbarch = dst->descr->gdbarch;
-  gdb_byte buf[MAX_REGISTER_SIZE];
   int regnum;
 
   /* The dst had better not be read-only.  If it is, the `restore'
      doesn't make much sense.  */
   gdb_assert (!dst->readonly_p);
+  gdb_assert (src->readonly_p);
   /* Copy over any registers, being careful to only restore those that
      were both saved and need to be restored.  The full [0 .. gdbarch_num_regs
      + gdbarch_num_pseudo_regs) range is checked since some architectures need
@@ -393,11 +391,8 @@ regcache_restore (struct regcache *dst,
     {
       if (gdbarch_register_reggroup_p (gdbarch, regnum, restore_reggroup))
 	{
-	  enum register_status status;
-
-	  status = cooked_read (cooked_read_context, regnum, buf);
-	  if (status == REG_VALID)
-	    regcache_cooked_write (dst, regnum, buf);
+	  if (src->register_status[regnum] == REG_VALID)
+	    regcache_cooked_write (dst, regnum, register_buffer (src, regnum));
 	}
     }
 }
@@ -424,7 +419,7 @@ regcache_cpy (struct regcache *dst, struct regcache *src)
   if (!src->readonly_p)
     regcache_save (dst, do_cooked_read, src);
   else if (!dst->readonly_p)
-    regcache_restore (dst, do_cooked_read, src);
+    regcache_restore (dst, src);
   else
     regcache_cpy_no_passthrough (dst, src);
 }

Re: [PATCH] Remove MAX_REGISTER_SIZE from regcache.c

[Alan Hayward]

> On 10 Apr 2017, at 09:59, Yao Qi <qiyaoltc@gmail.com> wrote:
> 
> Alan Hayward <Alan.Hayward@arm.com> writes:
> 
>>>> @@ -395,9 +404,9 @@ regcache_restore (struct regcache *dst,
>>>> 	{
>>>> 	  enum register_status status;
>>> 
>>> Can we move "buf" here? and initialize it with the register_size,
>>> 
>>>          std::vector<gdb_byte> buf (register_size (gdbarch, regnum));
>>> 
>>> then, we don't need max_register_size ().
>>> 
>> 
>> Problem with this is that we are then creating a brand new buffer for each
>> iteration of the loop, which is a little heavyweight.
>> We could create an empty buf outside the loop and re-size it each iteration,
>> but that's still going to cost.
>> 
> 
> How is this patch below?  I class-fied regcache last month in my local
> tree, and MAX_REGISTER_SIZE is disappeared from regcache.c.  I suggested
> using std::vector in a loop so that it is easy to rebase my patches.
> 

I’m happy with this. It also simplifies the code logic a little, which is good.

This would then reduce my patch to recache_save and regcache_dump changes.

Alan.


> -- 
> Yao (齐尧)
> From 25d562b5f858314ad9d41a7ffa825d8d24e3828e Mon Sep 17 00:00:00 2001
> From: Yao Qi <yao.qi@linaro.org>
> Date: Fri, 7 Apr 2017 22:50:27 +0100
> Subject: [PATCH] Simplify regcache_restore
> 
> This patches removes the 2nd argument of regcache_restore, because it
> is only called by regcache_cpy.  In regcache_cpy, if regcache_restore
> is called, dst is not readonly, but src is readonly.  So this patch
> adds an assert that src is readonly in regcache_restore.
> regcache_cook_read read everything from a readonly regcache cache
> (src)'s register_buffer, and register status is from src->register_status.
> 
> gdb:
> 
> 2017-04-07  Yao Qi  <yao.qi@linaro.org>
> 
> 	* regcache.c (regcache_restore): Remove argument 2.  Replace
> 	argument 3 with regcache.  Get register status from
> 	src->register_status and get register contents from
> 	register_buffer (src, regnum).
> 	(regcache_cpy): Update.
> 
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 37bc2f0..41c23a5 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -374,17 +374,15 @@ regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read,
> }
> 
> static void
> -regcache_restore (struct regcache *dst,
> -		  regcache_cooked_read_ftype *cooked_read,
> -		  void *cooked_read_context)
> +regcache_restore (struct regcache *dst, struct regcache *src)
> {
>   struct gdbarch *gdbarch = dst->descr->gdbarch;
> -  gdb_byte buf[MAX_REGISTER_SIZE];
>   int regnum;
> 
>   /* The dst had better not be read-only.  If it is, the `restore'
>      doesn't make much sense.  */
>   gdb_assert (!dst->readonly_p);
> +  gdb_assert (src->readonly_p);
>   /* Copy over any registers, being careful to only restore those that
>      were both saved and need to be restored.  The full [0 .. gdbarch_num_regs
>      + gdbarch_num_pseudo_regs) range is checked since some architectures need
> @@ -393,11 +391,8 @@ regcache_restore (struct regcache *dst,
>     {
>       if (gdbarch_register_reggroup_p (gdbarch, regnum, restore_reggroup))
> 	{
> -	  enum register_status status;
> -
> -	  status = cooked_read (cooked_read_context, regnum, buf);
> -	  if (status == REG_VALID)
> -	    regcache_cooked_write (dst, regnum, buf);
> +	  if (src->register_status[regnum] == REG_VALID)
> +	    regcache_cooked_write (dst, regnum, register_buffer (src, regnum));
> 	}
>     }
> }
> @@ -424,7 +419,7 @@ regcache_cpy (struct regcache *dst, struct regcache *src)
>   if (!src->readonly_p)
>     regcache_save (dst, do_cooked_read, src);
>   else if (!dst->readonly_p)
> -    regcache_restore (dst, do_cooked_read, src);
> +    regcache_restore (dst, src);
>   else
>     regcache_cpy_no_passthrough (dst, src);
> }

Re: [PATCH] Remove MAX_REGISTER_SIZE from regcache.c

[Yao Qi]

On Mon, Apr 10, 2017 at 9:59 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
>
> This patches removes the 2nd argument of regcache_restore, because it
> is only called by regcache_cpy.  In regcache_cpy, if regcache_restore
> is called, dst is not readonly, but src is readonly.  So this patch
> adds an assert that src is readonly in regcache_restore.
> regcache_cook_read read everything from a readonly regcache cache
> (src)'s register_buffer, and register status is from src->register_status.
>
> gdb:
>
> 2017-04-07  Yao Qi  <yao.qi@linaro.org>
>
>         * regcache.c (regcache_restore): Remove argument 2.  Replace
>         argument 3 with regcache.  Get register status from
>         src->register_status and get register contents from
>         register_buffer (src, regnum).
>         (regcache_cpy): Update.
>

Tested it again on x86_64-linux.  Pushed it in.

-- 
Yao (齐尧)

Re: [PATCH] Remove MAX_REGISTER_SIZE from regcache.c

[Alan Hayward]

> On 21 Apr 2017, at 15:01, Yao Qi <qiyaoltc@gmail.com> wrote:
> 
> On Mon, Apr 10, 2017 at 9:59 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
>> 
>> This patches removes the 2nd argument of regcache_restore, because it
>> is only called by regcache_cpy.  In regcache_cpy, if regcache_restore
>> is called, dst is not readonly, but src is readonly.  So this patch
>> adds an assert that src is readonly in regcache_restore.
>> regcache_cook_read read everything from a readonly regcache cache
>> (src)'s register_buffer, and register status is from src->register_status.
>> 
>> gdb:
>> 
>> 2017-04-07  Yao Qi  <yao.qi@linaro.org>
>> 
>>        * regcache.c (regcache_restore): Remove argument 2.  Replace
>>        argument 3 with regcache.  Get register status from
>>        src->register_status and get register contents from
>>        register_buffer (src, regnum).
>>        (regcache_cpy): Update.
>> 
> 
> Tested it again on x86_64-linux.  Pushed it in.
> 
> -- 
> Yao (齐尧)

Ok, this leaves the following in regcache.c.

Tested on a --enable-targets=all using make check with board files
unix and native-gdbserver.

Ok to commit?

Alan.

2017-04-26  Alan Hayward  <alan.hayward@arm.com>

	* regcache.c (regcache_save): Avoid buffer use.
	(regcache_dump): Avoid buffer use.



diff --git a/gdb/regcache.c b/gdb/regcache.c
index f9bf992197b8e7bdc3cfc6fc3bafbcf928c49df9..6c1c8b9ee373d8998f771d756542f59e2adde8ff 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -337,7 +337,6 @@ regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read,
 	       void *src)
 {
   struct gdbarch *gdbarch = dst->descr->gdbarch;
-  gdb_byte buf[MAX_REGISTER_SIZE];
   int regnum;

   /* The DST should be `read-only', if it wasn't then the save would
@@ -356,17 +355,13 @@ regcache_save (struct regcache *dst, regcache_cooked_read_ftype *cooked_read,
     {
       if (gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup))
 	{
-	  enum register_status status = cooked_read (src, regnum, buf);
+	  gdb_byte *dst_buf = register_buffer (dst, regnum);
+	  enum register_status status = cooked_read (src, regnum, dst_buf);

-	  if (status == REG_VALID)
-	    memcpy (register_buffer (dst, regnum), buf,
-		    register_size (gdbarch, regnum));
-	  else
+	  if (status != REG_VALID)
 	    {
 	      gdb_assert (status != REG_UNKNOWN);
-
-	      memset (register_buffer (dst, regnum), 0,
-		      register_size (gdbarch, regnum));
+	      memset (dst_buf, 0, register_size (gdbarch, regnum));
 	    }
 	  dst->register_status[regnum] = status;
 	}
@@ -1334,7 +1329,6 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
   int footnote_register_offset = 0;
   int footnote_register_type_name_null = 0;
   long register_offset = 0;
-  gdb_byte buf[MAX_REGISTER_SIZE];

 #if 0
   fprintf_unfiltered (file, "nr_raw_registers %d\n",
@@ -1461,8 +1455,8 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
 	    fprintf_unfiltered (file, "<unavailable>");
 	  else
 	    {
-	      regcache_raw_read (regcache, regnum, buf);
-	      print_hex_chars (file, buf,
+	      regcache_raw_update (regcache, regnum);
+	      print_hex_chars (file, register_buffer (regcache, regnum),
 			       regcache->descr->sizeof_register[regnum],
 			       gdbarch_byte_order (gdbarch));
 	    }
@@ -1475,9 +1469,30 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
 	    fprintf_unfiltered (file, "Cooked value");
 	  else
 	    {
+	      const gdb_byte *buf = NULL;
 	      enum register_status status;
+	      struct value *value = NULL;
+
+	      if (regnum < regcache->descr->nr_raw_registers)
+		{
+		  regcache_raw_update (regcache, regnum);
+		  status = regcache_register_status (regcache, regnum);
+		  buf = register_buffer (regcache, regnum);
+		}
+	      else
+		{
+		  value = regcache_cooked_read_value (regcache, regnum);
+
+		  if (!value_optimized_out (value)
+		      && value_entirely_available (value))
+		    {
+		      status = REG_VALID;
+		      buf = value_contents_all (value);
+		    }
+		  else
+		    status = REG_UNAVAILABLE;
+		}

-	      status = regcache_cooked_read (regcache, regnum, buf);
 	      if (status == REG_UNKNOWN)
 		fprintf_unfiltered (file, "<invalid>");
 	      else if (status == REG_UNAVAILABLE)
@@ -1486,6 +1501,12 @@ regcache_dump (struct regcache *regcache, struct ui_file *file,
 		print_hex_chars (file, buf,
 				 regcache->descr->sizeof_register[regnum],
 				 gdbarch_byte_order (gdbarch));
+
+	      if (value != NULL)
+		{
+		  release_value (value);
+		  value_free (value);
+		}
 	    }
 	}

Re: [PATCH] Remove MAX_REGISTER_SIZE from regcache.c

[Yao Qi]

Alan Hayward <Alan.Hayward@arm.com> writes:

> Ok to commit?
>
> Alan.
>
> 2017-04-26  Alan Hayward  <alan.hayward@arm.com>
>
> 	* regcache.c (regcache_save): Avoid buffer use.
> 	(regcache_dump): Avoid buffer use.

Hi Alan,
Patch is good to me, but it has conflict with my patches

  [PATCH 0/6] Class-fy regcache in GDB
  https://sourceware.org/ml/gdb-patches/2017-04/msg00684.html

Can you postpone pushing this patch until my patches are committed?  You
then need to rebase your patch.  I've rebased my patch sets twice before
I posted them.  Considering the size of your patch, it takes less
effort to rebase yours.

-- 
Yao (齐尧)

Re: [PATCH] Remove MAX_REGISTER_SIZE from regcache.c

[Alan Hayward]

> On 27 Apr 2017, at 10:43, Yao Qi <qiyaoltc@gmail.com> wrote:
> 
> Alan Hayward <Alan.Hayward@arm.com> writes:
> 
>> Ok to commit?
>> 
>> Alan.
>> 
>> 2017-04-26  Alan Hayward  <alan.hayward@arm.com>
>> 
>> 	* regcache.c (regcache_save): Avoid buffer use.
>> 	(regcache_dump): Avoid buffer use.
> 
> Hi Alan,
> Patch is good to me, but it has conflict with my patches
> 
>  [PATCH 0/6] Class-fy regcache in GDB
>  https://sourceware.org/ml/gdb-patches/2017-04/msg00684.html
> 
> Can you postpone pushing this patch until my patches are committed?  You
> then need to rebase your patch.  I've rebased my patch sets twice before
> I posted them.  Considering the size of your patch, it takes less
> effort to rebase yours.
> 

That’s fine. My rebase should be easy enough.
If you remember, could you let me know when you have pushed your patch.


Alan.

Re: [PATCH] Remove MAX_REGISTER_SIZE from regcache.c

[Yao Qi]

Alan Hayward <Alan.Hayward@arm.com> writes:

> That’s fine. My rebase should be easy enough.
> If you remember, could you let me know when you have pushed your patch.

Hi Alan,
My patch set is pushed in.  You can rebase your patch.

-- 
Yao (齐尧)

Re: [PATCH] Remove MAX_REGISTER_SIZE from regcache.c

[Alan Hayward]

> On 3 May 2017, at 09:21, Yao Qi <qiyaoltc@gmail.com> wrote:
> 
> Alan Hayward <Alan.Hayward@arm.com> writes:
> 
>> That’s fine. My rebase should be easy enough.
>> If you remember, could you let me know when you have pushed your patch.
> 
> Hi Alan,
> My patch set is pushed in.  You can rebase your patch.
> 
> -- 
> Yao (齐尧)

Patch updated to head.

Tested on a --enable-targets=all using make check with board files
unix and native-gdbserver.

Ok to commit?

Alan.

2017-05-03  Alan Hayward  <alan.hayward@arm.com>

	* regcache.c (regcache_save): Avoid buffer use.
	(regcache_dump): Likewise.


diff --git a/gdb/regcache.c b/gdb/regcache.c
index 255246b2606036f178c7db3125ecd465dbc1af05..5d319153ad1184b754dbb0bc740c3bbb59dfa2e8 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -327,7 +327,6 @@ regcache::save (regcache_cooked_read_ftype *cooked_read,
 		void *src)
 {
   struct gdbarch *gdbarch = m_descr->gdbarch;
-  gdb_byte buf[MAX_REGISTER_SIZE];
   int regnum;

   /* The DST should be `read-only', if it wasn't then the save would
@@ -345,18 +344,14 @@ regcache::save (regcache_cooked_read_ftype *cooked_read,
     {
       if (gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup))
 	{
-	  enum register_status status = cooked_read (src, regnum, buf);
+	  gdb_byte *dst_buf = register_buffer (regnum);
+	  enum register_status status = cooked_read (src, regnum, dst_buf);

-	  if (status == REG_VALID)
-	    memcpy (register_buffer (regnum), buf,
-		    register_size (gdbarch, regnum));
-	  else
-	    {
-	      gdb_assert (status != REG_UNKNOWN);
+	  gdb_assert (status != REG_UNKNOWN);
+
+	  if (status != REG_VALID)
+	    memset (dst_buf, 0, register_size (gdbarch, regnum));

-	      memset (register_buffer (regnum), 0,
-		      register_size (gdbarch, regnum));
-	    }
 	  m_register_status[regnum] = status;
 	}
     }
@@ -1439,7 +1434,6 @@ regcache::dump (ui_file *file, enum regcache_dump_what what_to_dump)
   int footnote_register_offset = 0;
   int footnote_register_type_name_null = 0;
   long register_offset = 0;
-  gdb_byte buf[MAX_REGISTER_SIZE];

 #if 0
   fprintf_unfiltered (file, "nr_raw_registers %d\n",
@@ -1565,8 +1559,8 @@ regcache::dump (ui_file *file, enum regcache_dump_what what_to_dump)
 	    fprintf_unfiltered (file, "<unavailable>");
 	  else
 	    {
-	      raw_read (regnum, buf);
-	      print_hex_chars (file, buf,
+	      raw_update (regnum);
+	      print_hex_chars (file, register_buffer (regnum),
 			       m_descr->sizeof_register[regnum],
 			       gdbarch_byte_order (gdbarch));
 	    }
@@ -1579,9 +1573,30 @@ regcache::dump (ui_file *file, enum regcache_dump_what what_to_dump)
 	    fprintf_unfiltered (file, "Cooked value");
 	  else
 	    {
+	      const gdb_byte *buf = NULL;
 	      enum register_status status;
+	      struct value *value = NULL;
+
+	      if (regnum < m_descr->nr_raw_registers)
+		{
+		  raw_update (regnum);
+		  status = get_register_status (regnum);
+		  buf = register_buffer (regnum);
+		}
+	      else
+		{
+		  value = cooked_read_value (regnum);
+
+		  if (!value_optimized_out (value)
+		      && value_entirely_available (value))
+		    {
+		      status = REG_VALID;
+		      buf = value_contents_all (value);
+		    }
+		  else
+		    status = REG_UNAVAILABLE;
+		}

-	      status = cooked_read (regnum, buf);
 	      if (status == REG_UNKNOWN)
 		fprintf_unfiltered (file, "<invalid>");
 	      else if (status == REG_UNAVAILABLE)
@@ -1590,6 +1605,12 @@ regcache::dump (ui_file *file, enum regcache_dump_what what_to_dump)
 		print_hex_chars (file, buf,
 				 m_descr->sizeof_register[regnum],
 				 gdbarch_byte_order (gdbarch));
+
+	      if (value != NULL)
+		{
+		  release_value (value);
+		  value_free (value);
+		}
 	    }
 	}

Re: [PATCH] Remove MAX_REGISTER_SIZE from regcache.c

[Alan Hayward]

Ping.
Looks like this one got missed.
(Has been recently re-tested too).


Alan.

> On 3 May 2017, at 11:42, Alan Hayward <Alan.Hayward@arm.com> wrote:
> 
> 
>> On 3 May 2017, at 09:21, Yao Qi <qiyaoltc@gmail.com> wrote:
>> 
>> Alan Hayward <Alan.Hayward@arm.com> writes:
>> 
>>> That’s fine. My rebase should be easy enough.
>>> If you remember, could you let me know when you have pushed your patch.
>> 
>> Hi Alan,
>> My patch set is pushed in.  You can rebase your patch.
>> 
>> -- 
>> Yao (齐尧)
> 
> Patch updated to head.
> 
> Tested on a --enable-targets=all using make check with board files
> unix and native-gdbserver.
> 
> Ok to commit?
> 
> Alan.
> 
> 2017-05-03  Alan Hayward  <alan.hayward@arm.com>
> 
> 	* regcache.c (regcache_save): Avoid buffer use.
> 	(regcache_dump): Likewise.
> 
> 
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 255246b2606036f178c7db3125ecd465dbc1af05..5d319153ad1184b754dbb0bc740c3bbb59dfa2e8 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -327,7 +327,6 @@ regcache::save (regcache_cooked_read_ftype *cooked_read,
> 		void *src)
> {
>   struct gdbarch *gdbarch = m_descr->gdbarch;
> -  gdb_byte buf[MAX_REGISTER_SIZE];
>   int regnum;
> 
>   /* The DST should be `read-only', if it wasn't then the save would
> @@ -345,18 +344,14 @@ regcache::save (regcache_cooked_read_ftype *cooked_read,
>     {
>       if (gdbarch_register_reggroup_p (gdbarch, regnum, save_reggroup))
> 	{
> -	  enum register_status status = cooked_read (src, regnum, buf);
> +	  gdb_byte *dst_buf = register_buffer (regnum);
> +	  enum register_status status = cooked_read (src, regnum, dst_buf);
> 
> -	  if (status == REG_VALID)
> -	    memcpy (register_buffer (regnum), buf,
> -		    register_size (gdbarch, regnum));
> -	  else
> -	    {
> -	      gdb_assert (status != REG_UNKNOWN);
> +	  gdb_assert (status != REG_UNKNOWN);
> +
> +	  if (status != REG_VALID)
> +	    memset (dst_buf, 0, register_size (gdbarch, regnum));
> 
> -	      memset (register_buffer (regnum), 0,
> -		      register_size (gdbarch, regnum));
> -	    }
> 	  m_register_status[regnum] = status;
> 	}
>     }
> @@ -1439,7 +1434,6 @@ regcache::dump (ui_file *file, enum regcache_dump_what what_to_dump)
>   int footnote_register_offset = 0;
>   int footnote_register_type_name_null = 0;
>   long register_offset = 0;
> -  gdb_byte buf[MAX_REGISTER_SIZE];
> 
> #if 0
>   fprintf_unfiltered (file, "nr_raw_registers %d\n",
> @@ -1565,8 +1559,8 @@ regcache::dump (ui_file *file, enum regcache_dump_what what_to_dump)
> 	    fprintf_unfiltered (file, "<unavailable>");
> 	  else
> 	    {
> -	      raw_read (regnum, buf);
> -	      print_hex_chars (file, buf,
> +	      raw_update (regnum);
> +	      print_hex_chars (file, register_buffer (regnum),
> 			       m_descr->sizeof_register[regnum],
> 			       gdbarch_byte_order (gdbarch));
> 	    }
> @@ -1579,9 +1573,30 @@ regcache::dump (ui_file *file, enum regcache_dump_what what_to_dump)
> 	    fprintf_unfiltered (file, "Cooked value");
> 	  else
> 	    {
> +	      const gdb_byte *buf = NULL;
> 	      enum register_status status;
> +	      struct value *value = NULL;
> +
> +	      if (regnum < m_descr->nr_raw_registers)
> +		{
> +		  raw_update (regnum);
> +		  status = get_register_status (regnum);
> +		  buf = register_buffer (regnum);
> +		}
> +	      else
> +		{
> +		  value = cooked_read_value (regnum);
> +
> +		  if (!value_optimized_out (value)
> +		      && value_entirely_available (value))
> +		    {
> +		      status = REG_VALID;
> +		      buf = value_contents_all (value);
> +		    }
> +		  else
> +		    status = REG_UNAVAILABLE;
> +		}
> 
> -	      status = cooked_read (regnum, buf);
> 	      if (status == REG_UNKNOWN)
> 		fprintf_unfiltered (file, "<invalid>");
> 	      else if (status == REG_UNAVAILABLE)
> @@ -1590,6 +1605,12 @@ regcache::dump (ui_file *file, enum regcache_dump_what what_to_dump)
> 		print_hex_chars (file, buf,
> 				 m_descr->sizeof_register[regnum],
> 				 gdbarch_byte_order (gdbarch));
> +
> +	      if (value != NULL)
> +		{
> +		  release_value (value);
> +		  value_free (value);
> +		}
> 	    }
> 	}
> 
> 

Re: [PATCH] Remove MAX_REGISTER_SIZE from regcache.c

[Yao Qi]

Alan Hayward <Alan.Hayward@arm.com> writes:

> 2017-05-03  Alan Hayward  <alan.hayward@arm.com>
>
> 	* regcache.c (regcache_save): Avoid buffer use.

s/regcache_save/regcache::save/

> 	(regcache_dump): Likewise.

s/regcche_dump/regcache::dump/

Otherwise, patch is good to me.

-- 
Yao (齐尧)