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

[Simon Marchi <simark@simark.ca>]

On 12/1/23 04:12, Luis Machado wrote:
> Hi Simon,
> 
> On 11/30/23 21:20, 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.
>>
>> 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.
>>
>> 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
> 
> Yeah, that was a silly mistake. Glad it was spotted.
> 
> I failed to spot the missing terminators for these two regcache_map_entry's,
> as I see them in other regcache_map_entry's.
> 
> This is OK, thanks!
> 
> Approved-By: Luis Machado <luis.machado@arm.com>

Thanks, pushed both patches.

Simon