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

[Simon Marchi <simark@simark.ca>]

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