Remove MAX_REGISTER_SIZE from regcache.c

  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
  

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

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

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