This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 3/3] Calculate max register size


I attempted to change max_register_size to unsigned long.
However, this caused failures due to a type mismatch when calling std::max.
This required updating sizeof_register in regcache_descr to unsigned, and
for consistency, updated the rest of the regcache_descr to use unsigned.
This then caused a chain of small changes, which leaked into quite a few
other files (including gdbserver).
I think regcache_descr should be updated to use unsigned types as part of
a separate patch, and not in this patch.

I did not add a check for null descr in max_register_size() -
gdbarch.c ensures everything is initialised. Also, None of the other
functions in this file ever check descr for null.

In the end, the only change against the previous version is to remove a
spurious newline

Ok to commit?
Also, are people happy with the comments in patch 2/3 ?


2017-01-17  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..5bc99f5c1ef87318edf4e934ec60c7f1225e7561 100644
--- a/gdb/regcache.h
+++ b/gdb/regcache.h
@@ -202,6 +202,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 9d28aa2c2114e0f1c52758bb2fbe9669a329c13e..1cdc0604ff26110df4de1ab469d2fe623f918b3d 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,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))

Alan.


> On 10 Jan 2017, at 13:50, Alan Hayward <Alan.Hayward@arm.com> wrote:
> 
> 
>> 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?
> 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]