Support large registers in regcache transfer_regset
Commit Message
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.
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(-)
Comments
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.
> 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".
Also, can they be "gdb_byte *" instead of "void *"?
> +{
> + 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.
> + /* 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.
> + /* 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.
Simon
@@ -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
+{
+ 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]);
+ /* 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]);
+ /* 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);
+ }
}
else
{
@@ -1030,12 +1092,26 @@ regcache::transfer_regset (const struct regset *regset,
if (offs + slot_size > size)
return;
- if (out_buf)
- raw_collect (regnum, (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 (regnum, 0, slot_size, out_loc);
+ else
+ out_regcache->raw_supply_part (regnum, 0, slot_size, in_loc);
+ }
else
- out_regcache->raw_supply (regnum, in_buf
- ? (const gdb_byte *) in_buf + offs
- : NULL);
+ {
+ if (out_buf)
+ raw_collect (regnum, out_loc);
+ else
+ out_regcache->raw_supply (regnum, in_loc);
+ }
return;
}
}
@@ -163,6 +163,10 @@ 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. */
+ void raw_collect_part (int regnum, int offset, int len, void *in) const;
+
/* See common/common-regcache.h. */
void raw_supply (int regnum, const void *buf) override;
@@ -184,6 +188,10 @@ public:
unavailable). */
void raw_supply_zeroed (int regnum);
+ /* Supply register REGNUM to REGCACHE, starting at offset in REGCACHE, writing
+ only LEN. */
+ void raw_supply_part (int regnum, int offset, int len, const gdb_byte *out);
+
void invalidate (int regnum);
virtual ~reg_buffer () = default;