Re: [PATCH 05/24] gdb: change regcache interface to use array_view

Simon Marchi
2023-11-13T16:47:30Z

On 11/13/23 09:00, Andrew Burgess wrote:
>>> diff --git a/gdbsupport/common-regcache.h b/gdbsupport/common-regcache.h
>>> index 6d98ca8c92ed..75d209f7cbc0 100644
>>> --- a/gdbsupport/common-regcache.h
>>> +++ b/gdbsupport/common-regcache.h
>>> @@ -78,11 +78,44 @@ struct reg_buffer_common
>>> buffer. */
>>> virtual register_status get_register_status (int regnum) const = 0;
>>>
>>> - /* Supply register REGNUM, whose contents are stored in BUF, to REGCACHE. */
>>> - virtual void raw_supply (int regnum, const void *buf) = 0;
>>> + /* Supply register REGNUM, whose contents are stored in SRC, to this register
>>> + buffer. */
>>> + virtual void raw_supply (int regnum, gdb::array_view<const gdb_byte> src,
>>> + float) = 0;
>>
>> The addition of the 'float' argument is pretty unexpected here. It's
>> not mentioned in the commit message or the function comment, nor is it
>> actually used in any of the function implementations as far as I can
>> tell. I'm assuming this is resolving some C++ overload problem.
>>
>> Sorry if I've missed something ... I've only looked at the diff so far.
>> I thought I'd reach out before I start playing with the code trying to
>> figure out what's going on.
>
> As an initial experiment, I tried removing the float parameter after
> applying every patch up to and including this one, and GDB still rebuilt
> fine.
>
> Then I applied every patch in the series, and removed the float
> parameter, and again GDB built fine.
>
> Maybe I'm missing something though...
>
> Thanks,
> Andrew

Haha, good catch, I added them during development as a temporary thing,
to make sure I inspected all call sites, but of course I totally forgot
to remove them. I added them to differentiate the calls to the raw
pointer and array_view overloads of the functions, I don't recall my
logic exactly. I remember I was worried about cases like these:

gdb_byte buf[SOME_ARCH_MAX_REG_SIZE];
cooked_read (regnum, buf);

We want this to pick up the raw pointer overload, because the size of
register regnum is possibly smaller than SOME_ARCH_MAX_REG_SIZE. When I
remove the dummy float parameters, clangd tells me that these
cooked_read calls still pick up the raw pointer overload, so it seems
ok. But I'll see in testing.

For now I did the changes locally, I wull send a v2 once I
fix the regressions identified by Luis.

Thanks,

Simon