Remove MAX_REGISTER_SIZE from regcache.c

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

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

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