Re: [PATCH v2 05/10] Ptrace support for Aarch64 SVE

Simon Marchi
2018-06-10T22:52:39Z

Hi Alan,

Mostly some nits, though there is what looks like an off by one error
in some loops. You can push after you have looked into it.

On 2018-06-06 11:16 AM, Alan Hayward wrote:
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index 1e4f937dc9..4db7d0d977 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -384,19 +384,62 @@ store_fpregs_to_thread (const struct regcache *regcache)
> }
> }
>
> +/* Fill GDB's register array with the sve register values
> + from the current thread. */
> +
> +static void
> +fetch_sveregs_from_thread (struct regcache *regcache)
> +{
> + std::unique_ptr<gdb_byte> base
> + = aarch64_sve_get_sveregs (ptid_get_lwp (regcache->ptid ()));
> + aarch64_sve_regs_copy_to_reg_buf (regcache, base.get ());
> +}
> +
> +/* Store to the current thread the valid sve register
> + values in the GDB's register array. */
> +
> +static void
> +store_sveregs_to_thread (struct regcache *regcache)
> +{
> + int ret;
> + struct iovec iovec;
> + int tid = ptid_get_lwp (regcache->ptid ());
> +
> + /* Obtain a dump of SVE registers from ptrace. */
> + std::unique_ptr<gdb_byte> base = aarch64_sve_get_sveregs (tid);

I am not sure if it matters in this case because no destructor is run, but
I think it should be std::unique_ptr<gdb_byte[]>.

> @@ -56,3 +60,268 @@ aarch64_sve_get_vq (int tid)
>
> return vq;
> }
> +
> +/* See nat/aarch64-sve-linux-ptrace.h. */
> +
> +std::unique_ptr<gdb_byte>
> +aarch64_sve_get_sveregs (int tid)
> +{
> + struct iovec iovec;
> + struct user_sve_header header;
> + uint64_t vq = aarch64_sve_get_vq (tid);
> +
> + if (vq == 0)
> + perror_with_name (_("Unable to fetch sve register header"));
> +
> + /* 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 ();
> +
> + if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec) < 0)
> + perror_with_name (_("Unable to fetch sve registers"));
> +
> + return buf;
> +}
> +
> +/* See nat/aarch64-sve-linux-ptrace.h. */
> +
> +void
> +aarch64_sve_regs_copy_to_reg_buf (struct reg_buffer_common *reg_buf,
> + const void *buf)

The indentation of the second line has one extra space.

> +{
> + char *base = (char*) buf;

Space after "char".

> + int i;
> + struct user_sve_header *header = (struct user_sve_header *) buf;
> + uint64_t vq, vg_reg_buf = 0;
> +
> + vq = sve_vq_from_vl (header->vl);
> +
> + /* Sanity check the data in the header. */
> + if (!sve_vl_valid (header->vl)
> + || SVE_PT_SIZE (vq, header->flags) != header->size)
> + error (_("Invalid SVE header from kernel."));
> +
> + if (REG_VALID == reg_buf->get_register_status (AARCH64_SVE_VG_REGNUM))
> + reg_buf->raw_collect (AARCH64_SVE_VG_REGNUM, &vg_reg_buf);
> +
> + if (vg_reg_buf == 0)
> + {
> + /* VG has not been set. */
> + vg_reg_buf = sve_vg_from_vl (header->vl);
> + reg_buf->raw_supply (AARCH64_SVE_VG_REGNUM, &vg_reg_buf);
> + }
> + else if (vg_reg_buf != sve_vg_from_vl (header->vl) && !vq_change_warned)
> + {
> + /* Vector length on the running process has changed. GDB currently does
> + not support this and will result in GDB showing incorrect partially
> + incorrect data for the vector registers. Warn once and continue. We
> + do not expect many programs to exhibit this behaviour. To fix this
> + we need to spot the change earlier and generate a new target
> + descriptor. */
> + warning (_("SVE Vector length has changed (%ld to %d). "
> + "Vector registers may show incorrect data."),
> + vg_reg_buf, sve_vg_from_vl (header->vl));
> + vq_change_warned = true;
> + }
> +
> + if (HAS_SVE_STATE (*header))
> + {
> + /* The register dump contains a set of SVE registers. */
> +
> + for (i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)

Declare the loop variables in the for (), when possible.

> + reg_buf->raw_supply (AARCH64_SVE_Z0_REGNUM + i,
> + base + SVE_PT_SVE_ZREG_OFFSET (vq, i));
> +
> + for (i = 0; i < AARCH64_SVE_P_REGS_NUM; i++)
> + reg_buf->raw_supply (AARCH64_SVE_P0_REGNUM + i,
> + base + SVE_PT_SVE_PREG_OFFSET (vq, i));
> +
> + reg_buf->raw_supply (AARCH64_SVE_FFR_REGNUM,
> + base + SVE_PT_SVE_FFR_OFFSET (vq));
> + reg_buf->raw_supply (AARCH64_FPSR_REGNUM,
> + base + SVE_PT_SVE_FPSR_OFFSET (vq));
> + reg_buf->raw_supply (AARCH64_FPCR_REGNUM,
> + base + SVE_PT_SVE_FPCR_OFFSET (vq));
> + }
> + else
> + {
> + /* There is no SVE state yet - the register dump contains a fpsimd
> + structure instead. These registers still exist in the hardware, but
> + the kernel has not yet initialised them, and so they will be null. */
> +
> + char *zero_reg = (char *) alloca (SVE_PT_SVE_ZREG_SIZE (vq));
> + struct user_fpsimd_state *fpsimd
> + = (struct user_fpsimd_state *)(base + SVE_PT_FPSIMD_OFFSET);
> +
> + /* Copy across the V registers from fpsimd structure to the Z registers,
> + ensuring the non overlapping state is set to null. */
> +
> + memset (zero_reg, 0, SVE_PT_SVE_ZREG_SIZE (vq));
> +
> + for (i = 0; i <= AARCH64_SVE_Z_REGS_NUM; i++)
> + {
> + memcpy (zero_reg, &fpsimd->vregs[i], sizeof (__int128_t));
> + reg_buf->raw_supply (AARCH64_SVE_Z0_REGNUM + i, zero_reg);
> + }

I'm getting this. Is the "i <= ..." on purpose?

CXX aarch64-sve-linux-ptrace.o
/home/simark/src/binutils-gdb/gdb/nat/aarch64-sve-linux-ptrace.c: In function 'void aarch64_sve_regs_copy_to_reg_buf(reg_buffer_common*, const void*)':
/home/simark/src/binutils-gdb/gdb/nat/aarch64-sve-linux-ptrace.c:168:61: error: iteration 32 invokes undefined behavior [-Werror=aggressive-loop-optimizations]
memcpy (zero_reg, &fpsimd->vregs[i], sizeof (__int128_t));
^
/home/simark/src/binutils-gdb/gdb/nat/aarch64-sve-linux-ptrace.c:166:21: note: within this loop
for (i = 0; i <= AARCH64_SVE_Z_REGS_NUM; i++)

> +
> + reg_buf->raw_supply (AARCH64_FPSR_REGNUM, &fpsimd->fpsr);
> + reg_buf->raw_supply (AARCH64_FPCR_REGNUM, &fpsimd->fpcr);
> +
> + /* Clear the SVE only registers. */
> +
> + for (i = 0; i <= AARCH64_SVE_P_REGS_NUM; i++)

Here too, "<="?

> + reg_buf->raw_supply (AARCH64_SVE_P0_REGNUM + i, zero_reg);
> +
> + reg_buf->raw_supply (AARCH64_SVE_FFR_REGNUM, zero_reg);
> + }
> +}
> +
> +/* See nat/aarch64-sve-linux-ptrace.h. */
> +
> +void
> +aarch64_sve_regs_copy_from_reg_buf (struct reg_buffer_common *reg_buf,
> + void *buf)

The indent is off by one here too.

reg_buf could be const.

> +{
> + struct user_sve_header *header = (struct user_sve_header *) buf;
> + char *base = (char*) buf;

Space after "char".

> + uint64_t vq, vg_reg_buf = 0;
> + int i;
> +
> + vq = sve_vq_from_vl (header->vl);
> +
> + /* Sanity check the data in the header. */
> + if (!sve_vl_valid (header->vl)
> + || SVE_PT_SIZE (vq, header->flags) != header->size)
> + error (_("Invalid SVE header from kernel."));
> +
> + if (REG_VALID == reg_buf->get_register_status (AARCH64_SVE_VG_REGNUM))
> + reg_buf->raw_collect (AARCH64_SVE_VG_REGNUM, &vg_reg_buf);
> +
> + if (vg_reg_buf != 0 && vg_reg_buf != sve_vg_from_vl (header->vl))
> + {
> + /* Vector length on the running process has changed. GDB currently does
> + not support this and will result in GDB writing invalid data back to
> + the vector registers. Error and exit. We do not expect many programs
> + to exhibit this behaviour. To fix this we need to spot the change
> + earlier and generate a new target descriptor. */
> + error (_("SVE Vector length has changed (%ld to %d). "
> + "Cannot write back registers."),
> + vg_reg_buf, sve_vg_from_vl (header->vl));
> + }
> +
> + if (!HAS_SVE_STATE (*header))
> + {
> + /* There is no SVE state yet - the register dump contains a fpsimd
> + structure instead. Where possible we want to write the reg_buf data
> + back to the kernel using the fpsimd structure. However, if we cannot
> + then we'll need to reformat the fpsimd into a full SVE structure,
> + resulting in the initialization of SVE state written back to the
> + kernel, which is why we try to avoid it. */
> +
> + int has_sve_state = 0;

This should become a bool, and tests should become:

if (has_sve_state)

if (!has_sve_state)

Thanks,

Simon