refactor: Simplify SVE interface to read/write registers
authorLuis Machado <luis.machado@arm.com>
Tue, 7 Feb 2023 10:08:23 +0000 (10:08 +0000)
committerLuis Machado <luis.machado@arm.com>
Wed, 4 Oct 2023 15:23:39 +0000 (16:23 +0100)
commit78d6a7e98ccf5f788f23d49cbd95c45da7ee4660
tree2e2a2a4a1237b4a9e1b5e126f700576171a7dfcf
parent6ada909eaf5ebfbd7d8c5839bab521cb9525c94a
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>
gdb/aarch64-linux-nat.c
gdb/nat/aarch64-scalable-linux-ptrace.c
gdb/nat/aarch64-scalable-linux-ptrace.h
gdbserver/linux-aarch64-low.cc