[v2,3/3] Use partial register read/writes in transfer_regset
Commit Message
This avoids assert failures when the register is bigger than the
slot size. This happens on Aarch64 when truncating Z registers
into an fpsimd structure.
Also, when the register is smaller then the slot size, then
zero pad when writing to the slot, and truncate when writing
to the regcache. This happens on Aarch64 with the CPSR register.
Continue to ensure registers are invalidated when both buffers
are null.
2018-06-21 Alan Hayward <alan.hayward@arm.com>
* regcache.c (reg_buffer::raw_collect_part): New function.
(reg_buffer::raw_supply_part): Likewise.
(regcache::transfer_regset): Call new functions.
* regcache.h (reg_buffer::raw_collect_part): New declaration.
(reg_buffer::raw_supply_part): Likewise.
---
gdb/regcache.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++++++-----
gdb/regcache.h | 10 +++++++
2 files changed, 95 insertions(+), 8 deletions(-)
Comments
Hi Alan,
On 2018-06-21 05:38 AM, Alan Hayward wrote:
> @@ -1013,12 +1077,18 @@ regcache::transfer_regset (const struct regset *regset,
> if (offs + slot_size > size)
> break;
>
> + /* Use part versions to prevent possible overflow. */
> if (out_buf)
Can you update the pointer comparisons in the code you touch to use != NULL or != nullptr?
> diff --git a/gdb/regcache.h b/gdb/regcache.h
> index c17ce09dee..a69b67d513 100644
> --- a/gdb/regcache.h
> +++ b/gdb/regcache.h
> @@ -162,6 +162,11 @@ public:
> void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
> bool is_signed) const;
>
> + /* Collect register REGNUM from REGCACHE, starting at offset in REGCACHE,
> + reading only LEN. If this runs off the end of the register, then fill the
> + additional space with zeros. */
To have a consistent interface, I would be tempted to use the same behavior as
read_part and write_part for reads and writes that run off the end of the register
(not allow it). It would be the responsibility of the caller to ensure that they
don't overflow. I think it would just be a matter of
std::min (reg_size, slot_size)
?
Simon
On 2018-06-21 05:38 AM, Alan Hayward wrote:
> This avoids assert failures when the register is bigger than the
> slot size. This happens on Aarch64 when truncating Z registers
> into an fpsimd structure.
Oh, and can you mention in the commit log that this happens when using
generate-core-file? It could help if somebody wants to test it in the future.
Thanks,
Simon
> On 21 Jun 2018, at 15:14, Simon Marchi <simon.marchi@ericsson.com> wrote:
>
> Hi Alan,
>
> On 2018-06-21 05:38 AM, Alan Hayward wrote:
>> @@ -1013,12 +1077,18 @@ regcache::transfer_regset (const struct regset *regset,
>> if (offs + slot_size > size)
>> break;
>>
>> + /* Use part versions to prevent possible overflow. */
>> if (out_buf)
>
> Can you update the pointer comparisons in the code you touch to use != NULL or != nullptr?
>
Will do.
>> diff --git a/gdb/regcache.h b/gdb/regcache.h
>> index c17ce09dee..a69b67d513 100644
>> --- a/gdb/regcache.h
>> +++ b/gdb/regcache.h
>> @@ -162,6 +162,11 @@ public:
>> void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
>> bool is_signed) const;
>>
>> + /* Collect register REGNUM from REGCACHE, starting at offset in REGCACHE,
>> + reading only LEN. If this runs off the end of the register, then fill the
>> + additional space with zeros. */
>
> To have a consistent interface, I would be tempted to use the same behavior as
> read_part and write_part for reads and writes that run off the end of the register
> (not allow it). It would be the responsibility of the caller to ensure that they
> don't overflow. I think it would just be a matter of
>
> std::min (reg_size, slot_size)
>
> ?
I went back and too between doing it this way and the way I posted.
I settled on this way because it made transfer_regset neater. I agree it makes
sense to keep the interface the same. In addition I’d then have to pull the
memset of 0s into transfer_regset. I think it’ll look better if I pull the two
almost identical blobs of code into a helper function.
I’ll switch it around and repost as single V3 tomorrow, with all the unsigned stuff
dropped.
Alan.
@@ -807,6 +807,38 @@ readable_regcache::read_part (int regnum, int offset, int len,
/* See regcache.h. */
+void
+reg_buffer::raw_collect_part (int regnum, int offset, int len,
+ gdb_byte *out) const
+{
+ int reg_size = register_size (arch (), regnum);
+
+ gdb_assert (out != nullptr);
+ gdb_assert (offset >= 0 && len >= 0 && offset <= reg_size);
+
+ if (offset == 0 && len == 0)
+ return;
+
+ if (offset == 0 && len == reg_size)
+ return raw_collect (regnum, out);
+
+ /* Read to buffer, then write out. */
+ gdb_byte *reg = (gdb_byte *) alloca (reg_size);
+ raw_collect (regnum, reg);
+
+ if (offset + len <= reg_size)
+ memcpy (out, reg + offset, len);
+ else
+ {
+ /* Requested region runs off the end of the register. Clear the
+ additional space. */
+ memcpy (out, reg + offset, reg_size - offset);
+ memset (out + reg_size, 0, offset + len - reg_size);
+ }
+}
+
+/* See regcache.h. */
+
enum register_status
regcache::write_part (int regnum, int offset, int len,
const gdb_byte *in, bool is_raw)
@@ -841,6 +873,38 @@ regcache::write_part (int regnum, int offset, int len,
/* See regcache.h. */
+void
+reg_buffer::raw_supply_part (int regnum, int offset, int len,
+ const gdb_byte *in)
+{
+ int reg_size = register_size (arch (), regnum);
+
+ gdb_assert (in != nullptr);
+ gdb_assert (offset >= 0 && len >= 0 && offset <= reg_size);
+
+ if (offset == 0 && len == 0)
+ return;
+
+ if (offset + len > reg_size)
+ {
+ /* Truncate length to fit the size of the regcache register. */
+ len = reg_size - offset;
+ }
+
+ if (offset == 0 && len == reg_size)
+ return raw_supply (regnum, in);
+
+ gdb_byte *reg = (gdb_byte *) alloca (reg_size);
+
+ /* Read when needed. */
+ if (offset > 0 || offset + len < reg_size)
+ raw_collect (regnum, reg);
+
+ /* Write to buffer, then write out. */
+ memcpy (reg + offset, in, len);
+ raw_supply (regnum, reg);
+}
+
enum register_status
readable_regcache::raw_read_part (int regnum, int offset, int len,
gdb_byte *buf)
@@ -1013,12 +1077,18 @@ regcache::transfer_regset (const struct regset *regset,
if (offs + slot_size > size)
break;
+ /* Use part versions to prevent possible overflow. */
if (out_buf)
- raw_collect (regno, (gdb_byte *) out_buf + offs);
+ raw_collect_part (regno, 0, slot_size,
+ (gdb_byte *) out_buf + offs);
+ else if (in_buf)
+ out_regcache->raw_supply_part (regno, 0, slot_size,
+ (const gdb_byte *) in_buf + offs);
else
- out_regcache->raw_supply (regno, in_buf
- ? (const gdb_byte *) in_buf + offs
- : NULL);
+ {
+ /* Invalidate the register. */
+ out_regcache->raw_supply (regno, nullptr);
+ }
}
else
{
@@ -1027,12 +1097,19 @@ regcache::transfer_regset (const struct regset *regset,
if (offs + slot_size > size)
return;
+ /* Use part versions to prevent possible overflow. */
if (out_buf)
- raw_collect (regnum, (gdb_byte *) out_buf + offs);
+ raw_collect_part (regnum, 0, slot_size,
+ (gdb_byte *) out_buf + offs);
+ else if (in_buf)
+ out_regcache->raw_supply_part (regnum, 0, slot_size,
+ (const gdb_byte *) in_buf + offs);
else
- out_regcache->raw_supply (regnum, in_buf
- ? (const gdb_byte *) in_buf + offs
- : NULL);
+ {
+ /* Invalidate the register. */
+ out_regcache->raw_supply (regnum, nullptr);
+ }
+
return;
}
}
@@ -162,6 +162,11 @@ public:
void raw_collect_integer (int regnum, gdb_byte *addr, int addr_len,
bool is_signed) const;
+ /* Collect register REGNUM from REGCACHE, starting at offset in REGCACHE,
+ reading only LEN. If this runs off the end of the register, then fill the
+ additional space with zeros. */
+ void raw_collect_part (int regnum, int offset, int len, gdb_byte *out) const;
+
/* See common/common-regcache.h. */
void raw_supply (int regnum, const void *buf) override;
@@ -183,6 +188,11 @@ public:
unavailable). */
void raw_supply_zeroed (int regnum);
+ /* Supply register REGNUM to REGCACHE, starting at offset in REGCACHE, writing
+ only LEN, without editing the rest of the register. If the length of the
+ supplied value would overflow the register, then truncate. */
+ void raw_supply_part (int regnum, int offset, int len, const gdb_byte *in);
+
void invalidate (int regnum);
virtual ~reg_buffer () = default;