Re: [PATCH] Removal of uses of MAX_REGISTER_SIZE

Alan Hayward
2017-02-09T13:26:36Z

> On 8 Feb 2017, at 17:09, Yao Qi <qiyaoltc@gmail.com> wrote:
>
> Alan Hayward <Alan.Hayward@arm.com> writes:
>
>> @@ -1135,8 +1135,8 @@ register_changed_p (int regnum, struct regcache *prev_regs,
>> struct regcache *this_regs)
>> {
>> struct gdbarch *gdbarch = get_regcache_arch (this_regs);
>> - gdb_byte prev_buffer[MAX_REGISTER_SIZE];
>> - gdb_byte this_buffer[MAX_REGISTER_SIZE];
>> + std::vector<gdb_byte> prev_buffer (register_size (gdbarch, regnum));
>> + std::vector<gdb_byte> this_buffer (register_size (gdbarch, regnum));
>> enum register_status prev_status;
>> enum register_status this_status;
>>
>
> This function should be moved to regcache.c, because it is about
> comparing bytes of a certain register in both regcaches. Then, wen can
> compare raw registers from register_buffer, and pseudo registers from
> the values.

Trying to remove the buffer results in quite a lot of code, as you can’t assume
the register has the same state in both prev_regs and this_regs.
For pseudo cases, it will still result in buffer usage.
(Attempted code pasted below).

I feel this is adding a whole lot of complexity and mostly duplicates code from
regcache_cooked_read.

Unless you really feel this case is going to hit performance, then I think the
existing code is much simpler and easier to read.

(Happy to move the function to regcache and change result to a bool).

Maybe a long term solution would be an alternative additional version of
regcache_cooked_read:

enum register_status
regcache_cooked_read (struct regcache *regcache, int regnum, gdb_byte **buf,
bool *requires_deallocation)

The function returns you a ptr in **buf. If *requires_deallocation is true then you
need to free buf once you have finished with it.

Or maybe a version of regcache_cooked_read which returns a struct *value
(assuming it’s possible to set the buffer in a struct value to be an existing pointer).

This could then be used throughout the codebase, preventing lots of small memcpy.
However, that would then open a rabbit hole of changes to common functions, which is
all far out of the scope of these patches.

It’s possible that all that is not possible because we don’t want to expose the
internal regcache pointers in case the values move.

This was my work in progress:

bool
regcache_register_changed_p (int regnum, struct regcache *prev_regs,
struct regcache *this_regs)
{
struct gdbarch *this_gdbarch = get_regcache_arch (this_regs);
struct gdbarch *prev_gdbarch;
gdb_byte *prev_buffer = NULL;
gdb_byte *this_buffer = NULL;
enum register_status prev_status;
enum register_status this_status;
gdb_assert (regnum >= 0);
gdb_assert (regnum < this_regs->descr->nr_cooked_registers);

/* if there are no previous registers consider all registers as changed. */
if (!prev_regs)
return true;
gdb_assert (regnum < regcache->descr->nr_cooked_registers);

prev_gdbarch = get_regcache_arch (prev_regs);

/* If arches don't match then consider all registers as changed. */
if (prev_gdbarch != gdbarch)
return true;

if (regnum < regcache->descr->nr_raw_registers)
{
prev_status = prev_regs->register_status[regnum];
this_status = this_status->register_status[regnum];
prev_buffer = register_buffer (prev_regs, regnum);
this_buffer = register_buffer (this_regs, regnum);
}
else
{
if (prev_regs->readonly_p
&& prev_regs->register_status[regnum] != REG_UNKNOWN)
{
prev_status = prev_regs->register_status[regnum];
prev_buffer = register_buffer (prev_regs, regnum);
}
else if (gdbarch_pseudo_register_read_value_p (prev_regs->descr->gdbarch))
{
struct value *mark, *computed;

mark = value_mark ();

computed = gdbarch_pseudo_register_read_value (prev_gdbarch,
prev_regs, regnum);
if (value_entirely_available (computed))
{
prev_buffer = value_contents_raw (computed);
prev_status = REG_VALID;
}
else
prev_status = REG_UNAVAILABLE;
}
}
else
{
prev_buffer = xmalloc (register_size (prev_gdbarch, regnum));
gdbarch_pseudo_register_read (prev_gdbarch, prev_regcache, regnum, prev_buffer);
}

....duplicate above code for this_regcache....
Possibly put above code into a static function (but then you still have to deal
with allocation and frees spanning the functions)

}

if (this_status != prev_status)
ret = true;
else if (this_status == REG_VALID)
ret = memcmp (prev_buffer, this_buffer,
register_size (gdbarch, regnum)) != 0;
else
ret = false;

...code to free any buf or mark for created for either prev and this...

return ret;
}

>
>> @@ -1146,13 +1146,13 @@ register_changed_p (int regnum, struct regcache *prev_regs,
>> return 1;
>>
>> /* Get register contents and compare. */
>> - prev_status = regcache_cooked_read (prev_regs, regnum, prev_buffer);
>> - this_status = regcache_cooked_read (this_regs, regnum, this_buffer);
>> + prev_status = regcache_cooked_read (prev_regs, regnum, prev_buffer.data ());
>> + this_status = regcache_cooked_read (this_regs, regnum, this_buffer.data ());
>>
>> if (this_status != prev_status)
>> return 1;
>> else if (this_status == REG_VALID)
>> - return memcmp (prev_buffer, this_buffer,
>> + return memcmp (prev_buffer.data (), this_buffer.data (),
>> register_size (gdbarch, regnum)) != 0;
>> else
>> return 0;
>
> --
> Yao (齐尧)