Re: [PATCH 06/24] gdb: fix bugs in {get,put}_frame_register_bytes

[Simon Marchi <simon.marchi@efficios.com>]

On 11/13/23 10:00, Andrew Burgess wrote:
> Simon Marchi <simon.marchi@polymtl.ca> writes:
> 
>> From: Simon Marchi <simon.marchi@efficios.com>
>>
>> I found this only by inspection: the myaddr pointer in
>> {get,put}_frame_register_bytes is reset to `buffer.data ()` at each
>> iteration.  This means that we will always use the bytes at the
>> beginning of `buffer` to read or write to the registers, instead of
>> progressing in `buffer`.
>>
>> Fix this by re-writing the functions to chip away the beginning of the
>> buffer array_view as we progress in reading or writing the data.
>>
>> These bugs was introduced almost 3 years ago [1], and yet nobody
>> complained.  I'm wondering which architecture relies on that register
>> "overflow" behavior (reading or writing multiple consecutive registers
>> with one {get,put}_frame_register_bytes calls), and in which situation.
>> I find these functions a bit dangerous, if a caller mis-calculates
>> things, it could end up silently reading or writing to the next
>> register, even if it's not the intent.
>>
>> If I could change it, I would prefer to have functions specifically made
>> for that ({get,put}_frame_register_bytes_consecutive or something like
>> that) and make {get,put}_frame_register_bytes only able to write within
>> a single register (which I presume represents most of the use cases of
>> the current {get,put}_frame_register_bytes).  If a caller mis-calculates
>> things and an overflow occurs while calling
>> {get,put}_frame_register_bytes, it would hit an assert.  The problem is
>> knowing which callers rely on the overflow behavior and which don't.
> 
> I agree that this overflow behaviour sucks.
> 
> I have a memory of being told (years ago now) that this was a result of
> some older compilers not emitting correct DWARF for compound value
> locations, instead the compiler would just emit a single register
> location, and assume that the debugger would read from consecutive DWARF
> registers.  Note, this code assumes that the DWARF register numbering is
> the same as GDB's register numbering, which is not always the case.
> 
> Personally, I'd love for GDB to be more aggressive about removing some
> of this legacy behaviour.  What I'd like to do is move things like this
> behind a switch, say 'set maintenance deprecated-features on|off', which
> would be off by default, but when it is turned on we'd print a message
> saying:
> 
>   The feature you turned this on for is deprecated, and will be removed
>   from future versions of GDB.  To avoid this feature removed, please
>   file a bug report here <url> describing the deprecated feature that
>   you are using.
> 
> Then if nobody complains after a couple of years, we can start deleting
> things.
> 
> Anyway... just putting my thoughts down.  I think this patch is fine.

Agreed, it would be nice.

> Approved-By: Andrew Burgess <aburgess@redhat.com>

Thanks,

Simon