[PATCH 0/2] Fix gdb.arch/aarch64-sme-core-*.exp failures

[PATCH 0/2] Fix gdb.arch/aarch64-sme-core-*.exp failures

[Simon Marchi]

While testing my pseudo-register series on QEMU AArch64 (to test SVE and
SME support), I noticed some pre-existing failures in the
gdb.arch/aarch64-sme-core-*.exp.  This small series fixes them.

Simon Marchi (2):
  gdb: return when exceeding buffer size in regcache::transfer_regset
  gdb: add missing regcache_map_entry array null terminators in
    aarch64-linux-tdep.c

 gdb/aarch64-linux-tdep.c | 7 +++++--
 gdb/regcache.c           | 2 +-
 2 files changed, 6 insertions(+), 3 deletions(-)


base-commit: d5835df2eebf8e9cd3ae35b683c831c2a16a5269
-- 
2.43.0

[PATCH 1/2] gdb: return when exceeding buffer size in regcache::transfer_regset

[Simon Marchi]

regcache::transfer_regset iterates over an array of regcache_map_entry,
transferring the registers (between regcache and buffer) described by
those entries.  It stops either when it reaches the end of the
regcache_map_entry array (marked by a null entry) or (it seems like the
intent is) when it reaches the end of the buffer (in which case not all
described registers are transferred).

I said "seems like the intent is", because there appears to be a small
bug.  transfer_regset is made of two loops:

    foreach regcache_map_entry:
      foreach register described by the regcache_map_entry:
        if the register doesn't fit in the remainder of the buffer:
	  break

        transfer register

When stopping because we have reached the end of the buffer, the break
only breaks out of the inner loop.

This problem causes some failures when I run tests such as
gdb.arch/aarch64-sme-core-3.exp (on AArch64 Linux, in qemu).  This is
partly due to aarch64_linux_iterate_over_regset_sections failing to add
a null terminator in its regcache_map_entry array, but I think there is
still a problem in transfer_regset.

The sequence to the crash is:

 - The `regcache_map_entry za_regmap` object built in
   aarch64_linux_iterate_over_regset_sections does not have a null
   terminator.
 - When the target does not have a ZA register,
   aarch64_linux_collect_za_regset calls `regcache->collect_regset` with
   a size of 0 (it's actually pointless, but still it should work).
 - transfer_regset gets called with a buffer size of 0.
 - transfer_regset detects that the register to transfer wouldn't fit in
   0 bytes, so it breaks out of the inner loop.
 - The outer loop tries to go read the next regcache_map_entry, but
   there isn't one, and we start reading garbage.

Obviously, this would get fixed by making
aarch64_linux_iterate_over_regset_sections use a null terminator (which
is what the following patch does).  But I think that when detecting that
there is not enough buffer left for the current register,
transfer_regset should return, not only break out of the inner loop.

This is a kind of contrived scenario, but imagine we have these two
regcache_map_entry objects:

  - 2 registers of 8 bytes
  - 2 registers of 4 bytes

For some reason, the caller passes a buffer of 12 bytes.
transfer_regset will detect that the second 8 byte register does not
fit, and break out of the inner loop.  However, it will then go try the
next regcache_map_entry.  It will see that it can fit one 4 byte
register in the remaining buffer space, and transfer it from/to there.
This is very likely not an expected behavior, we wouldn't expect to
read/write this sequence of registers from/to the buffer.

In this example, whether passing a 12 bytes buffer makes sense or
whether it is a size computation bug in the caller, we don't know, but I
think that exiting as soon as a register doesn't fit is the sane thing
to do.

Change-Id: Ia349627d2e5d281822ade92a8e7a4dea4f839e07
---
 gdb/regcache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/regcache.c b/gdb/regcache.c
index 9dc354ec2b3a..e46a0b58f505 100644
--- a/gdb/regcache.c
+++ b/gdb/regcache.c
@@ -1208,7 +1208,7 @@ regcache::transfer_regset (const struct regset *regset, int regbase,
 	for (; count--; regno++, offs += slot_size)
 	  {
 	    if (offs + slot_size > size)
-	      break;
+	      return;
 
 	    transfer_regset_register (out_regcache, regno, in_buf, out_buf,
 				      slot_size, offs);
-- 
2.43.0

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

[Simon Marchi]

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
-- 
2.43.0

Re: [PATCH 1/2] gdb: return when exceeding buffer size in regcache::transfer_regset

[John Baldwin]

On 11/30/23 1:20 PM, Simon Marchi wrote:
> regcache::transfer_regset iterates over an array of regcache_map_entry,
> transferring the registers (between regcache and buffer) described by
> those entries.  It stops either when it reaches the end of the
> regcache_map_entry array (marked by a null entry) or (it seems like the
> intent is) when it reaches the end of the buffer (in which case not all
> described registers are transferred).
> 
> I said "seems like the intent is", because there appears to be a small
> bug.  transfer_regset is made of two loops:
> 
>      foreach regcache_map_entry:
>        foreach register described by the regcache_map_entry:
>          if the register doesn't fit in the remainder of the buffer:
> 	  break
> 
>          transfer register
> 
> When stopping because we have reached the end of the buffer, the break
> only breaks out of the inner loop.
> 
> This problem causes some failures when I run tests such as
> gdb.arch/aarch64-sme-core-3.exp (on AArch64 Linux, in qemu).  This is
> partly due to aarch64_linux_iterate_over_regset_sections failing to add
> a null terminator in its regcache_map_entry array, but I think there is
> still a problem in transfer_regset.
> 
> The sequence to the crash is:
> 
>   - The `regcache_map_entry za_regmap` object built in
>     aarch64_linux_iterate_over_regset_sections does not have a null
>     terminator.
>   - When the target does not have a ZA register,
>     aarch64_linux_collect_za_regset calls `regcache->collect_regset` with
>     a size of 0 (it's actually pointless, but still it should work).
>   - transfer_regset gets called with a buffer size of 0.
>   - transfer_regset detects that the register to transfer wouldn't fit in
>     0 bytes, so it breaks out of the inner loop.
>   - The outer loop tries to go read the next regcache_map_entry, but
>     there isn't one, and we start reading garbage.
> 
> Obviously, this would get fixed by making
> aarch64_linux_iterate_over_regset_sections use a null terminator (which
> is what the following patch does).  But I think that when detecting that
> there is not enough buffer left for the current register,
> transfer_regset should return, not only break out of the inner loop.
> 
> This is a kind of contrived scenario, but imagine we have these two
> regcache_map_entry objects:
> 
>    - 2 registers of 8 bytes
>    - 2 registers of 4 bytes
> 
> For some reason, the caller passes a buffer of 12 bytes.
> transfer_regset will detect that the second 8 byte register does not
> fit, and break out of the inner loop.  However, it will then go try the
> next regcache_map_entry.  It will see that it can fit one 4 byte
> register in the remaining buffer space, and transfer it from/to there.
> This is very likely not an expected behavior, we wouldn't expect to
> read/write this sequence of registers from/to the buffer.
> 
> In this example, whether passing a 12 bytes buffer makes sense or
> whether it is a size computation bug in the caller, we don't know, but I
> think that exiting as soon as a register doesn't fit is the sane thing
> to do.
> 
> Change-Id: Ia349627d2e5d281822ade92a8e7a4dea4f839e07
> ---
>   gdb/regcache.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 9dc354ec2b3a..e46a0b58f505 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -1208,7 +1208,7 @@ regcache::transfer_regset (const struct regset *regset, int regbase,
>   	for (; count--; regno++, offs += slot_size)
>   	  {
>   	    if (offs + slot_size > size)
> -	      break;
> +	      return;
>   
>   	    transfer_regset_register (out_regcache, regno, in_buf, out_buf,
>   				      slot_size, offs);

This makes sense to me.  In particular, note that a few lines below we do a
return for the same condition when an individual register doesn't fit.

Reviewed-By: John Baldwin <jhb@FreeBSD.org>

-- 
John Baldwin

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

[John Baldwin]

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.

> 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>

-- 
John Baldwin

Re: [PATCH 1/2] gdb: return when exceeding buffer size in regcache::transfer_regset

[Simon Marchi]

On 2023-11-30 18:42, John Baldwin wrote:
> On 11/30/23 1:20 PM, Simon Marchi wrote:
>> regcache::transfer_regset iterates over an array of regcache_map_entry,
>> transferring the registers (between regcache and buffer) described by
>> those entries.  It stops either when it reaches the end of the
>> regcache_map_entry array (marked by a null entry) or (it seems like the
>> intent is) when it reaches the end of the buffer (in which case not all
>> described registers are transferred).
>>
>> I said "seems like the intent is", because there appears to be a small
>> bug.  transfer_regset is made of two loops:
>>
>>      foreach regcache_map_entry:
>>        foreach register described by the regcache_map_entry:
>>          if the register doesn't fit in the remainder of the buffer:
>>       break
>>
>>          transfer register
>>
>> When stopping because we have reached the end of the buffer, the break
>> only breaks out of the inner loop.
>>
>> This problem causes some failures when I run tests such as
>> gdb.arch/aarch64-sme-core-3.exp (on AArch64 Linux, in qemu).  This is
>> partly due to aarch64_linux_iterate_over_regset_sections failing to add
>> a null terminator in its regcache_map_entry array, but I think there is
>> still a problem in transfer_regset.
>>
>> The sequence to the crash is:
>>
>>   - The `regcache_map_entry za_regmap` object built in
>>     aarch64_linux_iterate_over_regset_sections does not have a null
>>     terminator.
>>   - When the target does not have a ZA register,
>>     aarch64_linux_collect_za_regset calls `regcache->collect_regset` with
>>     a size of 0 (it's actually pointless, but still it should work).
>>   - transfer_regset gets called with a buffer size of 0.
>>   - transfer_regset detects that the register to transfer wouldn't fit in
>>     0 bytes, so it breaks out of the inner loop.
>>   - The outer loop tries to go read the next regcache_map_entry, but
>>     there isn't one, and we start reading garbage.
>>
>> Obviously, this would get fixed by making
>> aarch64_linux_iterate_over_regset_sections use a null terminator (which
>> is what the following patch does).  But I think that when detecting that
>> there is not enough buffer left for the current register,
>> transfer_regset should return, not only break out of the inner loop.
>>
>> This is a kind of contrived scenario, but imagine we have these two
>> regcache_map_entry objects:
>>
>>    - 2 registers of 8 bytes
>>    - 2 registers of 4 bytes
>>
>> For some reason, the caller passes a buffer of 12 bytes.
>> transfer_regset will detect that the second 8 byte register does not
>> fit, and break out of the inner loop.  However, it will then go try the
>> next regcache_map_entry.  It will see that it can fit one 4 byte
>> register in the remaining buffer space, and transfer it from/to there.
>> This is very likely not an expected behavior, we wouldn't expect to
>> read/write this sequence of registers from/to the buffer.
>>
>> In this example, whether passing a 12 bytes buffer makes sense or
>> whether it is a size computation bug in the caller, we don't know, but I
>> think that exiting as soon as a register doesn't fit is the sane thing
>> to do.
>>
>> Change-Id: Ia349627d2e5d281822ade92a8e7a4dea4f839e07
>> ---
>>   gdb/regcache.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/gdb/regcache.c b/gdb/regcache.c
>> index 9dc354ec2b3a..e46a0b58f505 100644
>> --- a/gdb/regcache.c
>> +++ b/gdb/regcache.c
>> @@ -1208,7 +1208,7 @@ regcache::transfer_regset (const struct regset *regset, int regbase,
>>       for (; count--; regno++, offs += slot_size)
>>         {
>>           if (offs + slot_size > size)
>> -          break;
>> +          return;
>>             transfer_regset_register (out_regcache, regno, in_buf, out_buf,
>>                         slot_size, offs);
> 
> This makes sense to me.  In particular, note that a few lines below we do a
> return for the same condition when an individual register doesn't fit.

Yeah, although it could be argued that this case is different: we're
targetting a single register, we found the regcache_map_entry that
describes that register, so regardless of the outcome, we know we don't
want to keep searching the regmap array.

> Reviewed-By: John Baldwin <jhb@FreeBSD.org>

Thanks, will add the trailer.

Simon

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

[Simon Marchi]

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

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

[Luis Machado]

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>

Re: [PATCH 1/2] gdb: return when exceeding buffer size in regcache::transfer_regset

[Luis Machado]

On 11/30/23 21:20, Simon Marchi wrote:
> regcache::transfer_regset iterates over an array of regcache_map_entry,
> transferring the registers (between regcache and buffer) described by
> those entries.  It stops either when it reaches the end of the
> regcache_map_entry array (marked by a null entry) or (it seems like the
> intent is) when it reaches the end of the buffer (in which case not all
> described registers are transferred).
> 
> I said "seems like the intent is", because there appears to be a small
> bug.  transfer_regset is made of two loops:
> 
>     foreach regcache_map_entry:
>       foreach register described by the regcache_map_entry:
>         if the register doesn't fit in the remainder of the buffer:
> 	  break
> 
>         transfer register
> 
> When stopping because we have reached the end of the buffer, the break
> only breaks out of the inner loop.
> 
> This problem causes some failures when I run tests such as
> gdb.arch/aarch64-sme-core-3.exp (on AArch64 Linux, in qemu).  This is
> partly due to aarch64_linux_iterate_over_regset_sections failing to add
> a null terminator in its regcache_map_entry array, but I think there is
> still a problem in transfer_regset.
> 
> The sequence to the crash is:
> 
>  - The `regcache_map_entry za_regmap` object built in
>    aarch64_linux_iterate_over_regset_sections does not have a null
>    terminator.
>  - When the target does not have a ZA register,
>    aarch64_linux_collect_za_regset calls `regcache->collect_regset` with
>    a size of 0 (it's actually pointless, but still it should work).
>  - transfer_regset gets called with a buffer size of 0.
>  - transfer_regset detects that the register to transfer wouldn't fit in
>    0 bytes, so it breaks out of the inner loop.
>  - The outer loop tries to go read the next regcache_map_entry, but
>    there isn't one, and we start reading garbage.
> 
> Obviously, this would get fixed by making
> aarch64_linux_iterate_over_regset_sections use a null terminator (which
> is what the following patch does).  But I think that when detecting that
> there is not enough buffer left for the current register,
> transfer_regset should return, not only break out of the inner loop.
> 
> This is a kind of contrived scenario, but imagine we have these two
> regcache_map_entry objects:
> 
>   - 2 registers of 8 bytes
>   - 2 registers of 4 bytes
> 
> For some reason, the caller passes a buffer of 12 bytes.
> transfer_regset will detect that the second 8 byte register does not
> fit, and break out of the inner loop.  However, it will then go try the
> next regcache_map_entry.  It will see that it can fit one 4 byte
> register in the remaining buffer space, and transfer it from/to there.
> This is very likely not an expected behavior, we wouldn't expect to
> read/write this sequence of registers from/to the buffer.
> 
> In this example, whether passing a 12 bytes buffer makes sense or
> whether it is a size computation bug in the caller, we don't know, but I
> think that exiting as soon as a register doesn't fit is the sane thing
> to do.
> 
> Change-Id: Ia349627d2e5d281822ade92a8e7a4dea4f839e07
> ---
>  gdb/regcache.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/regcache.c b/gdb/regcache.c
> index 9dc354ec2b3a..e46a0b58f505 100644
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -1208,7 +1208,7 @@ regcache::transfer_regset (const struct regset *regset, int regbase,
>  	for (; count--; regno++, offs += slot_size)
>  	  {
>  	    if (offs + slot_size > size)
> -	      break;
> +	      return;
>  
>  	    transfer_regset_register (out_regcache, regno, in_buf, out_buf,
>  				      slot_size, offs);


LGTM.

Reviewed-By: Luis Machado <luis.machado@arm.com>

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

[Simon Marchi]

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