Support large registers in regcache transfer_regset
Commit Message
> On 17 Jun 2018, at 03:52, Simon Marchi <simark@simark.ca> wrote:
>
> On 2018-06-12 04:03 AM, Alan Hayward wrote:
>> Regcache transfer_regset is used to copy registers to/from a regset.
>> If the size of a regcache register is greater than it's slot in the regset
>> then the writes will overflow into the next slot(s), causing general
>> overflow corruption with the latter slots.
>
> Hi Alan,
>
> Can you please remind me when this happens? When reading cores, we don't
> write to regsets, if I understand correctly, so it would not be a problem
> tben.
Type “generate-core-file” on the command line.
Not sure if there are other ways of triggering it.
>
>> Add raw_collect_part and raw_supply_part, which are simplified versions of
>> raw_read_part and raw_write_part. Use these in accordingly in transfer_regset.
>>
>> This is required to support the .reg2 core section on Aarch64 SVE, which
>> only has enough space to store the first 128bits of each vector register.
>>
>> This patch replaces [PATCH v2 10/10] Remove reg2 section from Aarch64 SVE cores
>>
>> Tested by generating cores on aarch64 and aarch64 sve, both via gdb and the
>> kernel, then ensuring the cores load back on the both systems.
>> Checked cores on x86 still look ok.
>> Ran make check on x86 and aarch64.
>>
>> 2018-06-12 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 | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++------
>> gdb/regcache.h | 8 +++++
>> 2 files changed, 94 insertions(+), 10 deletions(-)
>>
>> diff --git a/gdb/regcache.c b/gdb/regcache.c
>> index 750ea2ad30..babc0e1d43 100644
>> --- a/gdb/regcache.c
>> +++ b/gdb/regcache.c
>> @@ -812,6 +812,27 @@ readable_regcache::read_part (int regnum, int offset, int len, void *in,
>> return REG_VALID;
>> }
>>
>> +/* See regcache.h. */
>> +
>> +void
>> +reg_buffer::raw_collect_part (int regnum, int offset, int len, void *in) const
>
> I know this comes from raw_read_part/raw_write_part, but I find the naming of the
> in/out parameters confusing. I think it would make more sense if this one was
> called "out" and the one in raw_supply_part "in”.
>
Agreed. I’ll also update the original code too.
> Also, can they be "gdb_byte *" instead of "void *”?
Done.
>
>> +{
>> + struct gdbarch *gdbarch = arch ();
>> + gdb_byte *reg = (gdb_byte *) alloca (register_size (gdbarch, regnum));
>> +
>> + gdb_assert (in != NULL);
>> + gdb_assert (offset >= 0 && offset <= m_descr->sizeof_register[regnum]);
>> + gdb_assert (len >= 0 && offset + len <= m_descr->sizeof_register[regnum]);
>
> The "&& offset <= m_descr->sizeof_register[regnum]" is redundant, given the
> following line. Other than mimicking raw_read_part, is there a reason why
> these are signed integers? Having them unsigned would avoid having to assert
> they are >= 0.
Looking at regcache, int is used for regnum throughout. I’d rather not have a
mismatch, and wouldn’t want to update everything else either (at least not
in this patch). In addition, if this code is going to now call down to
raw_collect/raw_supply, they should match.
I’ve updated the asserts in both here and the original code too.
>
>> + /* Something to do? */
>> + if (offset + len == 0)
>> + return;
>> + /* Read (when needed) ... */
>> + raw_collect (regnum, reg);
>> +
>> + /* ... modify ... */
>> + memcpy (in, reg + offset, len);
>> +}
>> +
>> enum register_status
>> regcache::write_part (int regnum, int offset, int len,
>> const void *out, bool is_raw)
>> @@ -849,6 +870,33 @@ regcache::write_part (int regnum, int offset, int len,
>> return REG_VALID;
>> }
>>
>> +/* See regcache.h. */
>> +
>> +void
>> +reg_buffer::raw_supply_part (int regnum, int offset, int len,
>> + const gdb_byte *out)
>> +{
>> + struct gdbarch *gdbarch = arch ();
>> + gdb_byte *reg = (gdb_byte *) alloca (register_size (gdbarch, regnum));
>> +
>> + gdb_assert (offset >= 0 && offset <= m_descr->sizeof_register[regnum]);
>> + gdb_assert (len >= 0 && offset + len <= m_descr->sizeof_register[regnum]);
>
> "&& offset <= m_descr->sizeof_register[regnum]" is redundant here too.
Done.
>
>> + /* Something to do? */
>> + if (offset + len == 0)
>> + return;
>> + /* Read (when needed) ... */
>> + if (offset > 0
>> + || offset + len < m_descr->sizeof_register[regnum])
>> + raw_collect (regnum, reg);
>> +
>> + if (out == nullptr)
>> + memset (reg + offset, 0, len);
>> + else
>> + memcpy (reg + offset, out, len);
>> + /* ... write (when needed). */
>> + raw_supply (regnum, reg);
>> +}
>> +
>> enum register_status
>> readable_regcache::raw_read_part (int regnum, int offset, int len, gdb_byte *buf)
>> {
>> @@ -1016,12 +1064,26 @@ regcache::transfer_regset (const struct regset *regset,
>> if (offs + slot_size > size)
>> break;
>>
>> - if (out_buf)
>> - raw_collect (regno, (gdb_byte *) out_buf + offs);
>> + gdb_byte *out_loc = (gdb_byte *) out_buf + offs;
>> + const gdb_byte *in_loc = in_buf ? (const gdb_byte *) in_buf + offs
>> + : NULL;
>> +
>> + if (slot_size < m_descr->sizeof_register[regno])
>> + {
>> + /* Register is bigger than the size of the slot. Prevent
>> + possible overflow. */
>> + if (out_buf)
>> + raw_collect_part (regno, 0, slot_size, out_loc);
>> + else
>> + out_regcache->raw_supply_part (regno, 0, slot_size, in_loc);
>> + }
>> else
>> - out_regcache->raw_supply (regno, in_buf
>> - ? (const gdb_byte *) in_buf + offs
>> - : NULL);
>> + {
>> + if (out_buf)
>> + raw_collect (regno, out_loc);
>> + else
>> + out_regcache->raw_supply (regno, in_loc);
>> + }
>
> I think the code here could stay relatively simple if we always went
> through raw_collect_part/raw_supply_part. To avoid the unnecessary copy
> that would happen in raw_collect_part/raw_supply_part in the typical case
> where the full register is transferred, there could be a shortcut path
> that calls raw_collect/raw_supply when collecting/supplying the full
> reguster.
>
Ok, added this too.
However, this uncovered an issue. If the slot size is larger than the size of
the register then this causes raw_collect_part/raw_supply_part to assert.
This is triggered on aarch64 with CPSR, because the register is 32bits but
the core file reserves 64bits.
In the current code there is a bug here as the existing code doesn’t ensure
this extra space in the slot size is cleared when writing out cores.
(In reality, probably not much of a problem as we ignore that space when
reading the core back in. But it could be an issue if another program was
Reading gdb’s core files).
To fix this I allowed raw_collect_part/raw_supply_part to handle cases where
the length overflows the register, either filling with zeros or truncating
accordingly.
Also, because the existing transfer_register code would result in register
being set to invalid when writing null to the regcache, I also ensured
that was kept.
I’ve also made a few more changes to write_part/read_part to make them
consistent with the changes to the new functions. There is no functionality
change to them.
Is this ok?
2018-06-19 Alan Hayward <alan.hayward@arm.com>
* regcache.c (readable_regcache::read_part): Add full read
shortcut.
(reg_buffer::raw_collect_part): New function.
(regcache::write_part): Add full write shortcut.
(reg_buffer::raw_supply_part): New function.
(regcache::transfer_regset): Use new functions.
* regcache.h (reg_buffer::raw_collect_part): New declaration.
(reg_buffer::raw_supply_part): Likewise.
Comments
On 2018-06-19 07:27, Alan Hayward wrote:
>>> +/* See regcache.h. */
>>> +
>>> +void
>>> +reg_buffer::raw_collect_part (int regnum, int offset, int len, void
>>> *in) const
>>> +{
>>> + struct gdbarch *gdbarch = arch ();
>>> + gdb_byte *reg = (gdb_byte *) alloca (register_size (gdbarch,
>>> regnum));
>>> +
>>> + gdb_assert (in != NULL);
>>> + gdb_assert (offset >= 0 && offset <=
>>> m_descr->sizeof_register[regnum]);
>>> + gdb_assert (len >= 0 && offset + len <=
>>> m_descr->sizeof_register[regnum]);
>>
>> The "&& offset <= m_descr->sizeof_register[regnum]" is redundant,
>> given the
>> following line. Other than mimicking raw_read_part, is there a reason
>> why
>> these are signed integers? Having them unsigned would avoid having to
>> assert
>> they are >= 0.
>
> Looking at regcache, int is used for regnum throughout. I’d rather not
> have a
> mismatch, and wouldn’t want to update everything else either (at least
> not
> in this patch). In addition, if this code is going to now call down to
> raw_collect/raw_supply, they should match.
Sorry, I was talking about len and offset, not regnum.
Simon
> On 19 Jun 2018, at 15:52, Simon Marchi <simark@simark.ca> wrote:
>
> On 2018-06-19 07:27, Alan Hayward wrote:
>>>> +/* See regcache.h. */
>>>> +
>>>> +void
>>>> +reg_buffer::raw_collect_part (int regnum, int offset, int len, void *in) const
>>>> +{
>>>> + struct gdbarch *gdbarch = arch ();
>>>> + gdb_byte *reg = (gdb_byte *) alloca (register_size (gdbarch, regnum));
>>>> +
>>>> + gdb_assert (in != NULL);
>>>> + gdb_assert (offset >= 0 && offset <= m_descr->sizeof_register[regnum]);
>>>> + gdb_assert (len >= 0 && offset + len <= m_descr->sizeof_register[regnum]);
>>> The "&& offset <= m_descr->sizeof_register[regnum]" is redundant, given the
>>> following line. Other than mimicking raw_read_part, is there a reason why
>>> these are signed integers? Having them unsigned would avoid having to assert
>>> they are >= 0.
>> Looking at regcache, int is used for regnum throughout. I’d rather not have a
>> mismatch, and wouldn’t want to update everything else either (at least not
>> in this patch). In addition, if this code is going to now call down to
>> raw_collect/raw_supply, they should match.
>
> Sorry, I was talking about len and offset, not regnum.
>
> Simon
Ah, ok. If doing that, then it’d make sense to update regcache_map_entry to use
unsigned ints for count and size.
struct regcache_map_entry
{
int count;
int regno;
int size;
};
At that point it probably makes sense to repost the patch as v2 in smaller pieces?
Alan.
On 2018-06-19 11:46, Alan Hayward wrote:
> Ah, ok. If doing that, then it’d make sense to update
> regcache_map_entry to use
> unsigned ints for count and size.
>
> struct regcache_map_entry
> {
> int count;
> int regno;
> int size;
> };
>
> At that point it probably makes sense to repost the patch as v2 in
> smaller pieces?
If you end up doing this (I did not really dig much to see if it works),
then yes it might make sense to have a preparatory patch that changes
some types to be unsigned. As always, one logical change per patch is
appreciated and is much easier to review.
If you're going to send a proper v2, I'll wait for this one to look at
the new changes you sent as part of the "updated" v1.
Simon
@@ -163,6 +163,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;
@@ -184,6 +189,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;
@@ -254,8 +264,11 @@ public:
struct value *cooked_read_value (int regnum);
protected:
- enum register_status read_part (int regnum, int offset, int len, void *in,
- bool is_raw);
+
+ /* Perform a partial register transfer using a read, modify, write
+ operation. */
+ enum register_status read_part (int regnum, int offset, int len,
+ gdb_byte *out, bool is_raw);
};
/* Buffer of registers, can be read and written. */
@@ -356,8 +369,10 @@ private:
int regnum, const void *in_buf,
void *out_buf, size_t size) const;
+ /* Perform a partial register transfer using a read, modify, write
+ operation. */
enum register_status write_part (int regnum, int offset, int len,
- const void *out, bool is_raw);
+ const gdb_byte *out, bool is_raw);
/* The address space of this register cache (for registers where it
@@ -780,75 +780,138 @@ regcache::cooked_write (int regnum, const gdb_byte *buf)
regnum, buf);
}
-/* Perform a partial register transfer using a read, modify, write
- operation. */
+/* See regcache.h. */
enum register_status
-readable_regcache::read_part (int regnum, int offset, int len, void *in,
+readable_regcache::read_part (int regnum, int offset, int len, gdb_byte *out,
bool is_raw)
{
- struct gdbarch *gdbarch = arch ();
- gdb_byte *reg = (gdb_byte *) alloca (register_size (gdbarch, regnum));
+ int reg_size = register_size (arch (), regnum);
- gdb_assert (in != NULL);
- gdb_assert (offset >= 0 && offset <= m_descr->sizeof_register[regnum]);
- gdb_assert (len >= 0 && offset + len <= m_descr->sizeof_register[regnum]);
- /* Something to do? */
- if (offset + len == 0)
+ gdb_assert (out != NULL);
+ gdb_assert (offset >= 0 && len >= 0 && offset + len <= reg_size);
+
+ if (offset == 0 && len == 0)
return REG_VALID;
- /* Read (when needed) ... */
+
+ if (offset == 0 && len == reg_size)
+ return (is_raw) ? raw_read (regnum, out) : cooked_read (regnum, out);
+
+ /* Read to buffer, then write out. */
enum register_status status;
+ gdb_byte *reg = (gdb_byte *) alloca (reg_size);
- if (is_raw)
- status = raw_read (regnum, reg);
- else
- status = cooked_read (regnum, reg);
+ status = (is_raw) ? raw_read (regnum, reg) : cooked_read (regnum, reg);
if (status != REG_VALID)
return status;
- /* ... modify ... */
- memcpy (in, reg + offset, len);
-
+ memcpy (out, reg + offset, len);
return REG_VALID;
}
+/* 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 void *out, bool is_raw)
+regcache::write_part (int regnum, int offset, int len, const gdb_byte *in,
+ bool is_raw)
{
- struct gdbarch *gdbarch = arch ();
- gdb_byte *reg = (gdb_byte *) alloca (register_size (gdbarch, regnum));
+ int reg_size = register_size (arch (), regnum);
- gdb_assert (out != NULL);
- gdb_assert (offset >= 0 && offset <= m_descr->sizeof_register[regnum]);
- gdb_assert (len >= 0 && offset + len <= m_descr->sizeof_register[regnum]);
- /* Something to do? */
- if (offset + len == 0)
+ gdb_assert (in != NULL);
+ gdb_assert (offset >= 0 && len >= 0 && offset + len <= reg_size);
+
+ if (offset == 0 && len == 0)
return REG_VALID;
- /* Read (when needed) ... */
- if (offset > 0
- || offset + len < m_descr->sizeof_register[regnum])
+
+ if (offset == 0 && len == reg_size)
{
- enum register_status status;
+ (is_raw) ? raw_write (regnum, in) : cooked_write (regnum, in);
+ return REG_VALID;
+ }
- if (is_raw)
- status = raw_read (regnum, reg);
- else
- status = cooked_read (regnum, reg);
+ gdb_byte *reg = (gdb_byte *) alloca (reg_size);
+
+ /* Read when needed. */
+ if (offset > 0 || offset + len < reg_size)
+ {
+ enum register_status status;
+ status = (is_raw) ? raw_read (regnum, reg) : cooked_read (regnum, reg);
if (status != REG_VALID)
return status;
}
- memcpy (reg + offset, out, len);
- /* ... write (when needed). */
- if (is_raw)
- raw_write (regnum, reg);
- else
- cooked_write (regnum, reg);
-
+ /* Write to buffer, then write out. */
+ memcpy (reg + offset, in, len);
+ is_raw ? raw_write (regnum, reg) : cooked_write (regnum, reg);
return REG_VALID;
}
+/* 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)
{
@@ -994,6 +1057,7 @@ regcache::transfer_regset (const struct regset *regset,
{
const struct regcache_map_entry *map;
int offs = 0, count;
+ struct gdbarch *gdbarch = arch ();
for (map = (const struct regcache_map_entry *) regset->regmap;
(count = map->count) != 0;
@@ -1016,12 +1080,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
{
@@ -1030,12 +1100,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;
}
}