* [PATCH 0/2] Fix gdb.arch/aarch64-sme-core-*.exp failures @ 2023-11-30 21:20 Simon Marchi 2023-11-30 21:20 ` [PATCH 1/2] gdb: return when exceeding buffer size in regcache::transfer_regset Simon Marchi 2023-11-30 21:20 ` [PATCH 2/2] gdb: add missing regcache_map_entry array null terminators in aarch64-linux-tdep.c Simon Marchi 0 siblings, 2 replies; 10+ messages in thread From: Simon Marchi @ 2023-11-30 21:20 UTC (permalink / raw) To: gdb-patches; +Cc: Luis Machado, John Baldwin, 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] gdb: return when exceeding buffer size in regcache::transfer_regset 2023-11-30 21:20 [PATCH 0/2] Fix gdb.arch/aarch64-sme-core-*.exp failures Simon Marchi @ 2023-11-30 21:20 ` Simon Marchi 2023-11-30 23:42 ` John Baldwin 2023-12-01 9:13 ` Luis Machado 2023-11-30 21:20 ` [PATCH 2/2] gdb: add missing regcache_map_entry array null terminators in aarch64-linux-tdep.c Simon Marchi 1 sibling, 2 replies; 10+ messages in thread From: Simon Marchi @ 2023-11-30 21:20 UTC (permalink / raw) To: gdb-patches; +Cc: Luis Machado, John Baldwin, 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] gdb: return when exceeding buffer size in regcache::transfer_regset 2023-11-30 21:20 ` [PATCH 1/2] gdb: return when exceeding buffer size in regcache::transfer_regset Simon Marchi @ 2023-11-30 23:42 ` John Baldwin 2023-12-01 0:02 ` Simon Marchi 2023-12-01 9:13 ` Luis Machado 1 sibling, 1 reply; 10+ messages in thread From: John Baldwin @ 2023-11-30 23:42 UTC (permalink / raw) To: Simon Marchi, gdb-patches; +Cc: Luis Machado 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] gdb: return when exceeding buffer size in regcache::transfer_regset 2023-11-30 23:42 ` John Baldwin @ 2023-12-01 0:02 ` Simon Marchi 0 siblings, 0 replies; 10+ messages in thread From: Simon Marchi @ 2023-12-01 0:02 UTC (permalink / raw) To: John Baldwin, Simon Marchi, gdb-patches; +Cc: Luis Machado 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] gdb: return when exceeding buffer size in regcache::transfer_regset 2023-11-30 21:20 ` [PATCH 1/2] gdb: return when exceeding buffer size in regcache::transfer_regset Simon Marchi 2023-11-30 23:42 ` John Baldwin @ 2023-12-01 9:13 ` Luis Machado 1 sibling, 0 replies; 10+ messages in thread From: Luis Machado @ 2023-12-01 9:13 UTC (permalink / raw) To: Simon Marchi, gdb-patches; +Cc: John Baldwin 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] gdb: add missing regcache_map_entry array null terminators in aarch64-linux-tdep.c 2023-11-30 21:20 [PATCH 0/2] Fix gdb.arch/aarch64-sme-core-*.exp failures Simon Marchi 2023-11-30 21:20 ` [PATCH 1/2] gdb: return when exceeding buffer size in regcache::transfer_regset Simon Marchi @ 2023-11-30 21:20 ` Simon Marchi 2023-11-30 23:44 ` John Baldwin 2023-12-01 9:12 ` Luis Machado 1 sibling, 2 replies; 10+ messages in thread From: Simon Marchi @ 2023-11-30 21:20 UTC (permalink / raw) To: gdb-patches; +Cc: Luis Machado, John Baldwin, 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] gdb: add missing regcache_map_entry array null terminators in aarch64-linux-tdep.c 2023-11-30 21:20 ` [PATCH 2/2] gdb: add missing regcache_map_entry array null terminators in aarch64-linux-tdep.c Simon Marchi @ 2023-11-30 23:44 ` John Baldwin 2023-12-01 0:03 ` Simon Marchi 2023-12-01 9:12 ` Luis Machado 1 sibling, 1 reply; 10+ messages in thread From: John Baldwin @ 2023-11-30 23:44 UTC (permalink / raw) To: Simon Marchi, gdb-patches; +Cc: Luis Machado 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] gdb: add missing regcache_map_entry array null terminators in aarch64-linux-tdep.c 2023-11-30 23:44 ` John Baldwin @ 2023-12-01 0:03 ` Simon Marchi 0 siblings, 0 replies; 10+ messages in thread From: Simon Marchi @ 2023-12-01 0:03 UTC (permalink / raw) To: John Baldwin, Simon Marchi, gdb-patches; +Cc: Luis Machado 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] gdb: add missing regcache_map_entry array null terminators in aarch64-linux-tdep.c 2023-11-30 21:20 ` [PATCH 2/2] gdb: add missing regcache_map_entry array null terminators in aarch64-linux-tdep.c Simon Marchi 2023-11-30 23:44 ` John Baldwin @ 2023-12-01 9:12 ` Luis Machado 2023-12-01 16:21 ` Simon Marchi 1 sibling, 1 reply; 10+ messages in thread From: Luis Machado @ 2023-12-01 9:12 UTC (permalink / raw) To: Simon Marchi, gdb-patches; +Cc: John Baldwin 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> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] gdb: add missing regcache_map_entry array null terminators in aarch64-linux-tdep.c 2023-12-01 9:12 ` Luis Machado @ 2023-12-01 16:21 ` Simon Marchi 0 siblings, 0 replies; 10+ messages in thread From: Simon Marchi @ 2023-12-01 16:21 UTC (permalink / raw) To: Luis Machado, Simon Marchi, gdb-patches; +Cc: John Baldwin 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-12-01 16:21 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-11-30 21:20 [PATCH 0/2] Fix gdb.arch/aarch64-sme-core-*.exp failures Simon Marchi 2023-11-30 21:20 ` [PATCH 1/2] gdb: return when exceeding buffer size in regcache::transfer_regset Simon Marchi 2023-11-30 23:42 ` John Baldwin 2023-12-01 0:02 ` Simon Marchi 2023-12-01 9:13 ` Luis Machado 2023-11-30 21:20 ` [PATCH 2/2] gdb: add missing regcache_map_entry array null terminators in aarch64-linux-tdep.c Simon Marchi 2023-11-30 23:44 ` John Baldwin 2023-12-01 0:03 ` Simon Marchi 2023-12-01 9:12 ` Luis Machado 2023-12-01 16:21 ` Simon Marchi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).