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
Ping.
> On 17 Jan 2017, at 14:22, Alan Hayward <Alan.Hayward@arm.com> wrote:
>
> 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?
>>
>