[2/2] gdb: add missing regcache_map_entry array null terminators in aarch64-linux-tdep.c
Checks
| Context |
Check |
Description |
| linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_build--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-arm |
success
|
Testing passed
|
| linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 |
success
|
Testing passed
|
Commit Message
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(-)
Comments
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>
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
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>
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
@@ -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