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

[Luis Machado <luis.machado@arm.com>]

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>