Re: [PATCH] Remove MAX_REGISTER_SIZE from regcache.c

[Yao Qi <qiyaoltc at gmail dot com>]

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 (齐尧)