Re: [PATCH 2/2] gdb: add missing regcache_map_entry array null terminators in aarch64-linux-tdep.c

[Simon Marchi <simark@simark.ca>]

On 2023-11-30 18:44, John Baldwin wrote:
> On 11/30/23 1:20 PM, Simon Marchi wrote:
>> Fix two spots in aarch64-linux-tdep.c that build regcache_map_entry
>> arrays without a null terminator.  The null terminators are needed for
>> regcache::transfer_regset to work properly.
>>
>> Some shower thoughts: I'm wondering: if a caller uses
>> supply_regset/collect_regset with a regcache_map_entry array that
>> describes exactly X bytes, and passes a buffer of X bytes, should a null
>> terminator really be needed?  regcache::transfer_regset could be written
>> in a way that it exits as soon as the remaining buffer size reaches 0.
>> Right now, even when it has consumed exactly the X bytes of the buffer,
>> transfer_regset needs to read the following regcache_map_entry's count
>> (expected to be 0) to realize it's done.  If it exited its outer loop
>> when `offs == size`, it would remove the need for the null terminator in
>> this case.
> 
> regcache_map_entry_size depends on a nul terminator as it is computing a
> size, not taking a register block size as input.

Good point, thanks.  That settles that regcache_map_entry arrays need a
null terminator to be valid.

>> Shower thought #2: we could also bypass that by using array_view to pass
>> regcache_map_entry arrays, removing the need for null terminators
>> altogether, but that's a bigger change.
> 
> I do think this is the better long-term solution.
> 
>> Change-Id: I3224a314e1360b319438f32de8c81e70ab42e105
>> ---
>>   gdb/aarch64-linux-tdep.c | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
>> index cd99b33fed25..5be887a9c817 100644
>> --- a/gdb/aarch64-linux-tdep.c
>> +++ b/gdb/aarch64-linux-tdep.c
>> @@ -1497,7 +1497,9 @@ aarch64_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
>>         /* Create this on the fly in order to handle the ZA register size.  */
>>         const struct regcache_map_entry za_regmap[] =
>>       {
>> -      { 1, tdep->sme_za_regnum, (int) std::pow (sve_vl_from_vq (tdep->sme_svq), 2) }
>> +      { 1, tdep->sme_za_regnum,
>> +        (int) std::pow (sve_vl_from_vq (tdep->sme_svq), 2) },
>> +      { 0 }
>>       };
>>           const struct regset aarch64_linux_za_regset =
>> @@ -1518,7 +1520,8 @@ aarch64_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
>>       {
>>         const struct regcache_map_entry zt_regmap[] =
>>           {
>> -          { 1, tdep->sme2_zt0_regnum, AARCH64_SME2_ZT0_SIZE }
>> +          { 1, tdep->sme2_zt0_regnum, AARCH64_SME2_ZT0_SIZE },
>> +          { 0 }
>>           };
>>           /* We set the register set size to REGSET_VARIABLE_SIZE here because
> 
> Reviewed-By: John Baldwin <jhb@FreeBSD.org>

Thanks for the review.  I will wait for feedback from Luis, since this
concerns AArch64.

Simon