diff options
author | Luis Machado <luis.machado@arm.com> | 2023-02-07 10:08:23 +0000 |
---|---|---|
committer | Luis Machado <luis.machado@arm.com> | 2023-10-04 16:23:39 +0100 |
commit | 78d6a7e98ccf5f788f23d49cbd95c45da7ee4660 (patch) | |
tree | 2e2a2a4a1237b4a9e1b5e126f700576171a7dfcf /gdb/nat | |
parent | 6ada909eaf5ebfbd7d8c5839bab521cb9525c94a (diff) | |
download | gdb-78d6a7e98ccf5f788f23d49cbd95c45da7ee4660.zip gdb-78d6a7e98ccf5f788f23d49cbd95c45da7ee4660.tar.gz gdb-78d6a7e98ccf5f788f23d49cbd95c45da7ee4660.tar.bz2 |
refactor: Simplify SVE interface to read/write registers
This is a patch in preparation to upcoming patches enabling SME support. It
attempts to simplify the gdb/gdbserver shared interface used to read/write
SVE registers.
Where the current code makes use of unique_ptr, allocating a new buffer by
hand and passing a buffer around, this patch makes that code use
gdb::byte_vector and passes a reference to this byte vector to the functions,
allowing the functions to have ready access to the size of the buffer.
It also shares a bit more code between gdb and gdbserver, in particular around
handling of ptrace get/set requests for SVE.
I think gdbserver could be refactored to handle register reads/writes more
like gdb's native layer as opposed to letting the generic linux-low layer do
the ptrace calls. This is not very flexible and assumes one size for the
responses. If you have something like NT_ARM_SVE, where you can have either
FPSIMD or SVE contents, it doesn't work that well.
I didn't want to change that interface right now as it is a bit too much work
and touches all the targets, some of which I can't easily test.
Hence the reason why the buffer the generic linux-now passes down to
linux-aarch64-low is unused or ignored.
No user-visible changes should happen as part of this refactor other than a
slightly reworded warning message.
While doing the refactor, I also noticed what seems to be a mistake in checking
if the register cache contains active (non-zero) SVE data.
For instance, the original code did something like this in
aarch64_sve_regs_copy_from_reg_buf:
has_sve_state |= reg_buf->raw_compare (AARCH64_SVE_Z0_REGNUM + i
reg, sizeof (__int128_t));
"reg" is a zeroed-out buffer that we compare the Z register contents
past the first 128 bits. The problem here is that raw_compare returns
1 if the contents compare the same, which means has_sve_state will be
true. But if we compared the Z register contents to 0, it means we
*do not* have SVE state, and therefore has_sve_state should be false.
The consequence of this mistake is that we convert the initial
FPSIMD-formatted data we get from ptrace for the NT_ARM_SVE register
set to a SVE-formatted one.
In the end, this doesn't cause user-visible differences because the
values of both the Z and V registers will still be the same. But the
logic is not correct.
I used the opportunity to fix this, and it gets tested later on by
the additional SME tests.
I do plan on submitting some SVE-specific tests to make sure we have
a bit more coverage in GDB's testsuite.
Regression-tested on aarch64-linux Ubuntu 22.04/20.04.
Reviewed-by: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Diffstat (limited to 'gdb/nat')
-rw-r--r-- | gdb/nat/aarch64-scalable-linux-ptrace.c | 202 | ||||
-rw-r--r-- | gdb/nat/aarch64-scalable-linux-ptrace.h | 27 |
2 files changed, 147 insertions, 82 deletions
diff --git a/gdb/nat/aarch64-scalable-linux-ptrace.c b/gdb/nat/aarch64-scalable-linux-ptrace.c index cc43f51..6eea910 100644 --- a/gdb/nat/aarch64-scalable-linux-ptrace.c +++ b/gdb/nat/aarch64-scalable-linux-ptrace.c @@ -120,28 +120,43 @@ aarch64_sve_set_vq (int tid, struct reg_buffer_common *reg_buf) /* See nat/aarch64-scalable-linux-ptrace.h. */ -std::unique_ptr<gdb_byte[]> -aarch64_sve_get_sveregs (int tid) +gdb::byte_vector +aarch64_fetch_sve_regset (int tid) { - struct iovec iovec; uint64_t vq = aarch64_sve_get_vq (tid); if (vq == 0) - perror_with_name (_("Unable to fetch SVE register header")); + perror_with_name (_("Unable to fetch SVE vector length")); /* A ptrace call with NT_ARM_SVE will return a header followed by either a dump of all the SVE and FP registers, or an fpsimd structure (identical to the one returned by NT_FPREGSET) if the kernel has not yet executed any SVE code. Make sure we allocate enough space for a full SVE dump. */ - iovec.iov_len = SVE_PT_SIZE (vq, SVE_PT_REGS_SVE); - std::unique_ptr<gdb_byte[]> buf (new gdb_byte[iovec.iov_len]); - iovec.iov_base = buf.get (); + gdb::byte_vector sve_state (SVE_PT_SIZE (vq, SVE_PT_REGS_SVE), 0); + + struct iovec iovec; + iovec.iov_base = sve_state.data (); + iovec.iov_len = sve_state.size (); if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec) < 0) perror_with_name (_("Unable to fetch SVE registers")); - return buf; + return sve_state; +} + +/* See nat/aarch64-scalable-linux-ptrace.h. */ + +void +aarch64_store_sve_regset (int tid, const gdb::byte_vector &sve_state) +{ + struct iovec iovec; + /* We need to cast from (const void *) here. */ + iovec.iov_base = (void *) sve_state.data (); + iovec.iov_len = sve_state.size (); + + if (ptrace (PTRACE_SETREGSET, tid, NT_ARM_SVE, &iovec) < 0) + perror_with_name (_("Unable to store SVE registers")); } /* If we are running in BE mode, byteswap the contents @@ -165,11 +180,13 @@ aarch64_maybe_swab128 (gdb_byte *dst, const gdb_byte *src, size_t size) /* See nat/aarch64-scalable-linux-ptrace.h. */ void -aarch64_sve_regs_copy_to_reg_buf (struct reg_buffer_common *reg_buf, - const void *buf) +aarch64_sve_regs_copy_to_reg_buf (int tid, struct reg_buffer_common *reg_buf) { - char *base = (char *) buf; - struct user_sve_header *header = (struct user_sve_header *) buf; + gdb::byte_vector sve_state = aarch64_fetch_sve_regset (tid); + + char *base = (char *) sve_state.data (); + struct user_sve_header *header + = (struct user_sve_header *) sve_state.data (); uint64_t vq = sve_vq_from_vl (header->vl); uint64_t vg = sve_vg_from_vl (header->vl); @@ -249,18 +266,33 @@ aarch64_sve_regs_copy_to_reg_buf (struct reg_buffer_common *reg_buf, reg_buf->raw_supply (AARCH64_SVE_FFR_REGNUM, reg); } + + /* At this point we have updated the register cache with the contents of + the NT_ARM_SVE register set. */ } /* See nat/aarch64-scalable-linux-ptrace.h. */ void -aarch64_sve_regs_copy_from_reg_buf (const struct reg_buffer_common *reg_buf, - void *buf) +aarch64_sve_regs_copy_from_reg_buf (int tid, + struct reg_buffer_common *reg_buf) { - struct user_sve_header *header = (struct user_sve_header *) buf; - char *base = (char *) buf; + /* First store the vector length to the thread. This is done first to + ensure the ptrace buffers read from the kernel are the correct size. */ + if (!aarch64_sve_set_vq (tid, reg_buf)) + perror_with_name (_("Unable to set VG register")); + + /* Obtain a dump of SVE registers from ptrace. */ + gdb::byte_vector sve_state = aarch64_fetch_sve_regset (tid); + + struct user_sve_header *header = (struct user_sve_header *) sve_state.data (); uint64_t vq = sve_vq_from_vl (header->vl); + gdb::byte_vector new_state (SVE_PT_SIZE (32, SVE_PT_REGS_SVE), 0); + memcpy (new_state.data (), sve_state.data (), sve_state.size ()); + header = (struct user_sve_header *) new_state.data (); + char *base = (char *) new_state.data (); + /* Sanity check the data in the header. */ if (!sve_vl_valid (header->vl) || SVE_PT_SIZE (vq, header->flags) != header->size) @@ -275,36 +307,40 @@ aarch64_sve_regs_copy_from_reg_buf (const struct reg_buffer_common *reg_buf, resulting in the initialization of SVE state written back to the kernel, which is why we try to avoid it. */ - bool has_sve_state = false; - gdb_byte *reg = (gdb_byte *) alloca (SVE_PT_SVE_ZREG_SIZE (vq)); - struct user_fpsimd_state *fpsimd - = (struct user_fpsimd_state *)(base + SVE_PT_FPSIMD_OFFSET); - - memset (reg, 0, SVE_PT_SVE_ZREG_SIZE (vq)); + /* Buffer (using the maximum size a Z register) used to look for zeroed + out sve state. */ + gdb_byte reg[256]; + memset (reg, 0, sizeof (reg)); /* Check in the reg_buf if any of the Z registers are set after the first 128 bits, or if any of the other SVE registers are set. */ - + bool has_sve_state = false; for (int i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++) { - has_sve_state |= reg_buf->raw_compare (AARCH64_SVE_Z0_REGNUM + i, - reg, sizeof (__int128_t)); - if (has_sve_state) - break; + if (!reg_buf->raw_compare (AARCH64_SVE_Z0_REGNUM + i, reg, + V_REGISTER_SIZE)) + { + has_sve_state = true; + break; + } } if (!has_sve_state) for (int i = 0; i < AARCH64_SVE_P_REGS_NUM; i++) { - has_sve_state |= reg_buf->raw_compare (AARCH64_SVE_P0_REGNUM + i, - reg, 0); - if (has_sve_state) - break; + if (!reg_buf->raw_compare (AARCH64_SVE_P0_REGNUM + i, reg, 0)) + { + has_sve_state = true; + break; + } } if (!has_sve_state) - has_sve_state |= reg_buf->raw_compare (AARCH64_SVE_FFR_REGNUM, - reg, 0); + has_sve_state + = !reg_buf->raw_compare (AARCH64_SVE_FFR_REGNUM, reg, 0); + + struct user_fpsimd_state *fpsimd + = (struct user_fpsimd_state *)(base + SVE_PT_FPSIMD_OFFSET); /* If no SVE state exists, then use the existing fpsimd structure to write out state and return. */ @@ -344,50 +380,74 @@ aarch64_sve_regs_copy_from_reg_buf (const struct reg_buffer_common *reg_buf, if (REG_VALID == reg_buf->get_register_status (AARCH64_FPCR_REGNUM)) reg_buf->raw_collect (AARCH64_FPCR_REGNUM, &fpsimd->fpcr); - return; - } - - /* Otherwise, reformat the fpsimd structure into a full SVE set, by - expanding the V registers (working backwards so we don't splat - registers before they are copied) and using null for everything else. - Note that enough space for a full SVE dump was originally allocated - for base. */ + /* At this point we have collected all the data from the register + cache and we are ready to update the FPSIMD register content + of the thread. */ - header->flags |= SVE_PT_REGS_SVE; - header->size = SVE_PT_SIZE (vq, SVE_PT_REGS_SVE); + /* Fall through so we can update the thread's contents with the + FPSIMD register cache values. */ + } + else + { + /* Otherwise, reformat the fpsimd structure into a full SVE set, by + expanding the V registers (working backwards so we don't splat + registers before they are copied) and using zero for everything + else. + Note that enough space for a full SVE dump was originally allocated + for base. */ + + header->flags |= SVE_PT_REGS_SVE; + header->size = SVE_PT_SIZE (vq, SVE_PT_REGS_SVE); + + memcpy (base + SVE_PT_SVE_FPSR_OFFSET (vq), &fpsimd->fpsr, + sizeof (uint32_t)); + memcpy (base + SVE_PT_SVE_FPCR_OFFSET (vq), &fpsimd->fpcr, + sizeof (uint32_t)); + + for (int i = AARCH64_SVE_Z_REGS_NUM - 1; i >= 0 ; i--) + { + memcpy (base + SVE_PT_SVE_ZREG_OFFSET (vq, i), &fpsimd->vregs[i], + sizeof (__int128_t)); + } - memcpy (base + SVE_PT_SVE_FPSR_OFFSET (vq), &fpsimd->fpsr, - sizeof (uint32_t)); - memcpy (base + SVE_PT_SVE_FPCR_OFFSET (vq), &fpsimd->fpcr, - sizeof (uint32_t)); + /* At this point we have converted the FPSIMD layout to an SVE + layout and copied the register data. - for (int i = AARCH64_SVE_Z_REGS_NUM; i >= 0 ; i--) - { - memcpy (base + SVE_PT_SVE_ZREG_OFFSET (vq, i), &fpsimd->vregs[i], - sizeof (__int128_t)); + Fall through so we can update the thread's contents with the SVE + register cache values. */ } } + else + { + /* We already have SVE state for this thread, so we just need to update + the values of the registers. */ + for (int i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++) + if (REG_VALID == reg_buf->get_register_status (AARCH64_SVE_Z0_REGNUM + + i)) + reg_buf->raw_collect (AARCH64_SVE_Z0_REGNUM + i, + base + SVE_PT_SVE_ZREG_OFFSET (vq, i)); - /* Replace the kernel values with those from reg_buf. */ - - for (int i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++) - if (REG_VALID == reg_buf->get_register_status (AARCH64_SVE_Z0_REGNUM + i)) - reg_buf->raw_collect (AARCH64_SVE_Z0_REGNUM + i, - base + SVE_PT_SVE_ZREG_OFFSET (vq, i)); - - for (int i = 0; i < AARCH64_SVE_P_REGS_NUM; i++) - if (REG_VALID == reg_buf->get_register_status (AARCH64_SVE_P0_REGNUM + i)) - reg_buf->raw_collect (AARCH64_SVE_P0_REGNUM + i, - base + SVE_PT_SVE_PREG_OFFSET (vq, i)); + for (int i = 0; i < AARCH64_SVE_P_REGS_NUM; i++) + if (REG_VALID == reg_buf->get_register_status (AARCH64_SVE_P0_REGNUM + + i)) + reg_buf->raw_collect (AARCH64_SVE_P0_REGNUM + i, + base + SVE_PT_SVE_PREG_OFFSET (vq, i)); + + if (REG_VALID == reg_buf->get_register_status (AARCH64_SVE_FFR_REGNUM)) + reg_buf->raw_collect (AARCH64_SVE_FFR_REGNUM, + base + SVE_PT_SVE_FFR_OFFSET (vq)); + if (REG_VALID == reg_buf->get_register_status (AARCH64_FPSR_REGNUM)) + reg_buf->raw_collect (AARCH64_FPSR_REGNUM, + base + SVE_PT_SVE_FPSR_OFFSET (vq)); + if (REG_VALID == reg_buf->get_register_status (AARCH64_FPCR_REGNUM)) + reg_buf->raw_collect (AARCH64_FPCR_REGNUM, + base + SVE_PT_SVE_FPCR_OFFSET (vq)); + } - if (REG_VALID == reg_buf->get_register_status (AARCH64_SVE_FFR_REGNUM)) - reg_buf->raw_collect (AARCH64_SVE_FFR_REGNUM, - base + SVE_PT_SVE_FFR_OFFSET (vq)); - if (REG_VALID == reg_buf->get_register_status (AARCH64_FPSR_REGNUM)) - reg_buf->raw_collect (AARCH64_FPSR_REGNUM, - base + SVE_PT_SVE_FPSR_OFFSET (vq)); - if (REG_VALID == reg_buf->get_register_status (AARCH64_FPCR_REGNUM)) - reg_buf->raw_collect (AARCH64_FPCR_REGNUM, - base + SVE_PT_SVE_FPCR_OFFSET (vq)); + /* At this point we have collected all the data from the register cache and + we are ready to update the SVE/FPSIMD register contents of the thread. + sve_state should contain all the data in the correct format, ready to be + passed on to ptrace. */ + aarch64_store_sve_regset (tid, new_state); } diff --git a/gdb/nat/aarch64-scalable-linux-ptrace.h b/gdb/nat/aarch64-scalable-linux-ptrace.h index 2847c4e..167782c 100644 --- a/gdb/nat/aarch64-scalable-linux-ptrace.h +++ b/gdb/nat/aarch64-scalable-linux-ptrace.h @@ -52,22 +52,27 @@ uint64_t aarch64_sve_get_vq (int tid); bool aarch64_sve_set_vq (int tid, uint64_t vq); bool aarch64_sve_set_vq (int tid, struct reg_buffer_common *reg_buf); -/* Read the current SVE register set using ptrace, allocating space as - required. */ +/* Read the current SVE register set from thread TID and return its data + through a byte vector. */ -extern std::unique_ptr<gdb_byte[]> aarch64_sve_get_sveregs (int tid); +extern gdb::byte_vector aarch64_fetch_sve_regset (int tid); -/* Put the registers from linux structure buf into register buffer. Assumes the - vector lengths in the register buffer match the size in the kernel. */ +/* Write the SVE contents from SVE_STATE to thread TID. */ -extern void aarch64_sve_regs_copy_to_reg_buf (struct reg_buffer_common *reg_buf, - const void *buf); +extern void +aarch64_store_sve_regset (int tid, const gdb::byte_vector &sve_state); + +/* Given a thread id TID and a register buffer REG_BUF, update the register + buffer with the SVE state from thread TID. */ + +extern void +aarch64_sve_regs_copy_to_reg_buf (int tid, struct reg_buffer_common *reg_buf); -/* Put the registers from register buffer into linux structure buf. Assumes the - vector lengths in the register buffer match the size in the kernel. */ +/* Given a thread id TID and a register buffer REG_BUF containing SVE + register data, write the SVE data to thread TID. */ extern void -aarch64_sve_regs_copy_from_reg_buf (const struct reg_buffer_common *reg_buf, - void *buf); +aarch64_sve_regs_copy_from_reg_buf (int tid, + struct reg_buffer_common *reg_buf); #endif /* NAT_AARCH64_SCALABLE_LINUX_PTRACE_H */ |