This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 3/3] Calculate max register size
- From: Alan Hayward <Alan dot Hayward at arm dot com>
- To: Luis Machado <lgustavo at codesourcery dot com>
- Cc: "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>, nd <nd at arm dot com>
- Date: Tue, 10 Jan 2017 13:50:02 +0000
- Subject: Re: [PATCH 3/3] Calculate max register size
- Authentication-results: sourceware.org; auth=none
- Authentication-results: spf=none (sender IP is ) smtp.mailfrom=Alan dot Hayward at arm dot com;
- Nodisclaimer: True
- References: <000B14E8-6B46-4C03-B70F-CC5E50BCFBCF@arm.com> <ded07baa-175e-c400-a55b-67f38b189afb@codesourcery.com>
- Spamdiagnosticmetadata: NSPM
- Spamdiagnosticoutput: 1:99
> 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?