diff mbox

[3/3] Calculate max register size

Message ID 000B14E8-6B46-4C03-B70F-CC5E50BCFBCF@arm.com
State New
Headers show

Commit Message

Alan Hayward Jan. 9, 2017, 10:58 a.m. UTC
Aarch64 SVE requires a max register size of 256. The current max size in gdb
is 64. This is part of a series demonstrating the replacement of
MAX_REGISTER_SIZE.

In cases where a buffer is created to be used multiple times to hold different
registers, then the maximum register size is required. Add a max register value
to the regcache which is calculated on initialization.

This patch is restricted to remote.c and regcache.c.
Follow on patches will expand to other files.

Tested on x86.
Ok to commit?

Thanks,
Alan.

2017-01-09  Alan Hayward  <alan.hayward@arm.com>

	* regcache.c (struct regcache_descr): Add max_register_size
	(max_register_size): New.
	(init_regcache_descr): Find max register size.
	(regcache_save): Use max_register_size.
	(regcache_restore): Likewise.
	(regcache_dump): Likewise.
	* regcache.h (max_register_size): New.
	* remote.c (remote_prepare_to_store): Allocate buffer.

Comments

Luis Machado Jan. 9, 2017, 8:14 p.m. UTC | #1
On 01/09/2017 04:58 AM, Alan Hayward wrote:
> Aarch64 SVE requires a max register size of 256. The current max size in gdb
> is 64. This is part of a series demonstrating the replacement of
> MAX_REGISTER_SIZE.
>
> In cases where a buffer is created to be used multiple times to hold different
> registers, then the maximum register size is required. Add a max register value
> to the regcache which is calculated on initialization.
>
> This patch is restricted to remote.c and regcache.c.
> Follow on patches will expand to other files.
>
> Tested on x86.
> Ok to commit?
>
> Thanks,
> Alan.
>
> 2017-01-09  Alan Hayward  <alan.hayward@arm.com>
>
> 	* regcache.c (struct regcache_descr): Add max_register_size
> 	(max_register_size): New.
> 	(init_regcache_descr): Find max register size.
> 	(regcache_save): Use max_register_size.
> 	(regcache_restore): Likewise.
> 	(regcache_dump): Likewise.
> 	* regcache.h (max_register_size): New.
> 	* remote.c (remote_prepare_to_store): Allocate buffer.
>
>
> diff --git a/gdb/regcache.h b/gdb/regcache.h
> index e5a7cf553279b8cc0d546ec1b8274cbf97e246d5..4db9517a9dd464d9c43be2af0573b767b86bfb56 100644
> --- a/gdb/regcache.h
> +++ b/gdb/regcache.h
> @@ -202,6 +202,9 @@ 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.  */
> +

Spurious newline.

> +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 9d28aa2c2114e0f1c52758bb2fbe9669a329c13e..46d017c7b2abcb18c9cdda005749071328735dbd 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;

Is this ever negative? Why not make it unsigned?

>  };
>
>  static void *
> @@ -125,7 +128,9 @@ init_regcache_descr (struct gdbarch *gdbarch)
>  	descr->sizeof_register[i] = TYPE_LENGTH (descr->register_type[i]);
>  	descr->register_offset[i] = offset;
>  	offset += descr->sizeof_register[i];
> -	gdb_assert (MAX_REGISTER_SIZE >= 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;
> @@ -135,7 +140,9 @@ init_regcache_descr (struct gdbarch *gdbarch)
>  	descr->sizeof_register[i] = TYPE_LENGTH (descr->register_type[i]);
>  	descr->register_offset[i] = offset;
>  	offset += descr->sizeof_register[i];
> -	gdb_assert (MAX_REGISTER_SIZE >= 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

Same as above, is this ever negative?

> +max_register_size (struct gdbarch *gdbarch)
> +{
> +  struct regcache_descr *descr = regcache_descr (gdbarch);

Is descr ever NULL?

> +  return descr->max_register_size;
> +}
> +
>  /* The register cache for storing raw register values.  */
>
>  struct regcache
> @@ -327,7 +341,7 @@ 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];
> +  gdb_byte *buf = (gdb_byte *) alloca (max_register_size (gdbarch));
>    int regnum;
>
>    /* The DST should be `read-only', if it wasn't then the save would
> @@ -369,7 +383,7 @@ regcache_restore (struct regcache *dst,
>  		  void *cooked_read_context)
>  {
>    struct gdbarch *gdbarch = dst->descr->gdbarch;
> -  gdb_byte buf[MAX_REGISTER_SIZE];
> +  gdb_byte *buf = (gdb_byte *) alloca (max_register_size (gdbarch));
>    int regnum;
>
>    /* The dst had better not be read-only.  If it is, the `restore'
> @@ -1279,7 +1293,7 @@ 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];
> +  gdb_byte *buf = (gdb_byte *) alloca (max_register_size (gdbarch));
>
>  #if 0
>    fprintf_unfiltered (file, "nr_raw_registers %d\n",
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 9247d43b094925ff397eb36b450eaba521adfc99..86856e6a6aba1967faaa8ef547f8a48fcc63c383 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -7752,9 +7752,10 @@ remote_fetch_registers (struct target_ops *ops,
>  static void
>  remote_prepare_to_store (struct target_ops *self, struct regcache *regcache)
>  {
> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);
>    struct remote_arch_state *rsa = get_remote_arch_state ();
>    int i;
> -  gdb_byte buf[MAX_REGISTER_SIZE];
> +  gdb_byte *buf = (gdb_byte *) alloca (max_register_size (gdbarch));
>
>    /* Make sure the entire registers array is valid.  */
>    switch (packet_support (PACKET_P))
>
>
>

My comment is the same as 2/3. Should we use a different data structure 
that can grow/shrink as one wishes?
Alan Hayward Jan. 10, 2017, 1:50 p.m. UTC | #2
> On 9 Jan 2017, at 20:14, Luis Machado <lgustavo@codesourcery.com> wrote:

> 

> On 01/09/2017 04:58 AM, Alan Hayward wrote:

>> Aarch64 SVE requires a max register size of 256. The current max size in gdb

>> is 64. This is part of a series demonstrating the replacement of

>> MAX_REGISTER_SIZE.

>> 

>> In cases where a buffer is created to be used multiple times to hold different

>> registers, then the maximum register size is required. Add a max register value

>> to the regcache which is calculated on initialization.

>> 

>> This patch is restricted to remote.c and regcache.c.

>> Follow on patches will expand to other files.

>> 

>> Tested on x86.

>> Ok to commit?

>> 

>> Thanks,

>> Alan.

>> 

>> 2017-01-09  Alan Hayward <alan.hayward@arm.com>

>> 

>> 	* regcache.c (struct regcache_descr): Add max_register_size

>> 	(max_register_size): New.

>> 	(init_regcache_descr): Find max register size.

>> 	(regcache_save): Use max_register_size.

>> 	(regcache_restore): Likewise.

>> 	(regcache_dump): Likewise.

>> 	* regcache.h (max_register_size): New.

>> 	* remote.c (remote_prepare_to_store): Allocate buffer.

>> 

>> 

>> diff --git a/gdb/regcache.h b/gdb/regcache.h

>> index e5a7cf553279b8cc0d546ec1b8274cbf97e246d5..4db9517a9dd464d9c43be2af0573b767b86bfb56 100644

>> --- a/gdb/regcache.h

>> +++ b/gdb/regcache.h

>> @@ -202,6 +202,9 @@ 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.  */

>> +

> 

> Spurious newline.


Ok.

> 

>> +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 9d28aa2c2114e0f1c52758bb2fbe9669a329c13e..46d017c7b2abcb18c9cdda005749071328735dbd 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;

> 

> Is this ever negative? Why not make it unsigned?


Should never be negative. Will change.

> 

>> };

>> 

>> static void *

>> @@ -125,7 +128,9 @@ init_regcache_descr (struct gdbarch *gdbarch)

>> 	descr->sizeof_register[i] = TYPE_LENGTH (descr->register_type[i]);

>> 	descr->register_offset[i] = offset;

>> 	offset += descr->sizeof_register[i];

>> -	gdb_assert (MAX_REGISTER_SIZE >= 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;

>> @@ -135,7 +140,9 @@ init_regcache_descr (struct gdbarch *gdbarch)

>> 	descr->sizeof_register[i] = TYPE_LENGTH (descr->register_type[i]);

>> 	descr->register_offset[i] = offset;

>> 	offset += descr->sizeof_register[i];

>> -	gdb_assert (MAX_REGISTER_SIZE >= 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

> 

> Same as above, is this ever negative?


Ok.

> 

>> +max_register_size (struct gdbarch *gdbarch)

>> +{

>> +  struct regcache_descr *descr = regcache_descr (gdbarch);

> 

> Is descr ever NULL?


I don’t think so. Gdbarch.c ensures everything is initialised.
None of the other functions in this file ever check for null.

> 

>> +  return descr->max_register_size;

>> +}

>> +

>> /* The register cache for storing raw register values.  */

>> 

>> struct regcache

>> @@ -327,7 +341,7 @@ 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];

>> +  gdb_byte *buf = (gdb_byte *) alloca (max_register_size (gdbarch));

>>   int regnum;

>> 

>>   /* The DST should be `read-only', if it wasn't then the save would

>> @@ -369,7 +383,7 @@ regcache_restore (struct regcache *dst,

>> 		  void *cooked_read_context)

>> {

>>   struct gdbarch *gdbarch = dst->descr->gdbarch;

>> -  gdb_byte buf[MAX_REGISTER_SIZE];

>> +  gdb_byte *buf = (gdb_byte *) alloca (max_register_size (gdbarch));

>>   int regnum;

>> 

>>   /* The dst had better not be read-only. If it is, the `restore'

>> @@ -1279,7 +1293,7 @@ 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];

>> +  gdb_byte *buf = (gdb_byte *) alloca (max_register_size (gdbarch));

>> 

>> #if 0

>>   fprintf_unfiltered (file, "nr_raw_registers %d\n",

>> diff --git a/gdb/remote.c b/gdb/remote.c

>> index 9247d43b094925ff397eb36b450eaba521adfc99..86856e6a6aba1967faaa8ef547f8a48fcc63c383 100644

>> --- a/gdb/remote.c

>> +++ b/gdb/remote.c

>> @@ -7752,9 +7752,10 @@ remote_fetch_registers (struct target_ops *ops,

>> static void

>> remote_prepare_to_store (struct target_ops *self, struct regcache *regcache)

>> {

>> +  struct gdbarch *gdbarch = get_regcache_arch (regcache);

>>   struct remote_arch_state *rsa = get_remote_arch_state ();

>>   int i;

>> -  gdb_byte buf[MAX_REGISTER_SIZE];

>> +  gdb_byte *buf = (gdb_byte *) alloca (max_register_size (gdbarch));

>> 

>>   /* Make sure the entire registers array is valid.  */

>>   switch (packet_support (PACKET_P))

>> 

>> 

>> 

> 

> My comment is the same as 2/3. Should we use a different data structure that can grow/shrink as one wishes?
diff mbox

Patch

diff --git a/gdb/regcache.h b/gdb/regcache.h
index e5a7cf553279b8cc0d546ec1b8274cbf97e246d5..4db9517a9dd464d9c43be2af0573b767b86bfb56 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -202,6 +202,9 @@  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 9d28aa2c2114e0f1c52758bb2fbe9669a329c13e..46d017c7b2abcb18c9cdda005749071328735dbd 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 *
@@ -125,7 +128,9 @@  init_regcache_descr (struct gdbarch *gdbarch)
 	descr->sizeof_register[i] = TYPE_LENGTH (descr->register_type[i]);
 	descr->register_offset[i] = offset;
 	offset += descr->sizeof_register[i];
-	gdb_assert (MAX_REGISTER_SIZE >= 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;
@@ -135,7 +140,9 @@  init_regcache_descr (struct gdbarch *gdbarch)
 	descr->sizeof_register[i] = TYPE_LENGTH (descr->register_type[i]);
 	descr->register_offset[i] = offset;
 	offset += descr->sizeof_register[i];
-	gdb_assert (MAX_REGISTER_SIZE >= 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,7 @@  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];
+  gdb_byte *buf = (gdb_byte *) alloca (max_register_size (gdbarch));
   int regnum;

   /* The DST should be `read-only', if it wasn't then the save would
@@ -369,7 +383,7 @@  regcache_restore (struct regcache *dst,
 		  void *cooked_read_context)
 {
   struct gdbarch *gdbarch = dst->descr->gdbarch;
-  gdb_byte buf[MAX_REGISTER_SIZE];
+  gdb_byte *buf = (gdb_byte *) alloca (max_register_size (gdbarch));
   int regnum;

   /* The dst had better not be read-only.  If it is, the `restore'
@@ -1279,7 +1293,7 @@  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];
+  gdb_byte *buf = (gdb_byte *) alloca (max_register_size (gdbarch));

 #if 0
   fprintf_unfiltered (file, "nr_raw_registers %d\n",
diff --git a/gdb/remote.c b/gdb/remote.c
index 9247d43b094925ff397eb36b450eaba521adfc99..86856e6a6aba1967faaa8ef547f8a48fcc63c383 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7752,9 +7752,10 @@  remote_fetch_registers (struct target_ops *ops,
 static void
 remote_prepare_to_store (struct target_ops *self, struct regcache *regcache)
 {
+  struct gdbarch *gdbarch = get_regcache_arch (regcache);
   struct remote_arch_state *rsa = get_remote_arch_state ();
   int i;
-  gdb_byte buf[MAX_REGISTER_SIZE];
+  gdb_byte *buf = (gdb_byte *) alloca (max_register_size (gdbarch));

   /* Make sure the entire registers array is valid.  */
   switch (packet_support (PACKET_P))