diff mbox

Remove MAX_REGISTER_SIZE from regcache.c

Message ID 0DADF920-69B9-4F96-A153-6965E56B5DA8@arm.com
State New
Headers show

Commit Message

Alan Hayward April 5, 2017, 2:53 p.m. UTC
> 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.

Comments

Yao Qi April 5, 2017, 4 p.m. UTC | #1
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);
    }
Alan Hayward April 5, 2017, 6:10 p.m. UTC | #2
> 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.
diff mbox

Patch

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