Re: [PATCH 8/8] Ptrace support for Aarch64 SVE

[Simon Marchi <simon.marchi@ericsson.com>]

Hi Alan,

Since there is a good number of macros copied from the Linux kernel, it might be
a good idea to isolate in their own file.  It would also be good to identify
precisely which file they come from (the path relative to the root of the linux
repo) and the git commit you used.

Also, since the copied code is rather large and non-trivial, does it pose copyright
problems?  If we want to include it, maybe that separate file should not state that
the copyright is owned by the FSF, since it's not the case?  Maybe others have
experience with this kind of things, or maybe we should get an advice from the FSF directly.

> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
> index 9381786fda..84c7a41f40 100644
> --- a/gdb/nat/aarch64-sve-linux-ptrace.c
> +++ b/gdb/nat/aarch64-sve-linux-ptrace.c
> @@ -25,6 +25,13 @@
>  #include "aarch64-sve-linux-ptrace.h"
>  #include "arch/aarch64.h"
>  
> +#ifndef GDBSERVER
> +#include "defs.h"
> +#endif
> +#include "regcache.h"

Hmm we try not add any more "#ifdef GDBSERVER" in the common code.

https://sourceware.org/gdb/wiki/Common#Header_files_in_common_code_.28defs.h_vs_server.h.2C_etc..29

Instead, we should try defining a common interface (probably in common/common-regcache.h?) that the
common code will use, and that regcaches from GDB and GDBserver will implement.

> +
> +static bool vq_change_warned = false;
> +
>  /* Read VQ for the given tid using ptrace.  If SVE is not supported then zero
>     is returned (on a system that supports SVE, then VQ cannot be zeo).  */
>  
> @@ -50,3 +57,259 @@ aarch64_sve_get_vq (int tid)
>  
>    return vq;
>  }
> +
> +/* Read the current SVE register set using ptrace, allocating space as
> +   required.  */

Put a reference to the .h here.

Since this returns allocated memory, could we return an RAII object?  Either
std::vector, std::unique_ptr or gdb::unique_xmalloc_ptr.

> +
> +gdb_byte *
> +aarch64_sve_get_sveregs (int tid)
> +{
> +  int ret;
> +  struct iovec iovec;
> +  struct user_sve_header header;
> +  long 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);
> +  iovec.iov_base = xmalloc (iovec.iov_len);
> +
> +  ret = ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec);
> +  if (ret < 0)
> +    perror_with_name (_("Unable to fetch sve registers"));
> +
> +  return (gdb_byte *) iovec.iov_base;
> +}
> +
> +/* Put the registers from linux structure buf into regcache.  */
> +
> +void
> +aarch64_sve_regs_copy_to_regcache (struct regcache *regcache, const void *buf)
> +{
> +  char *base = (char*) buf;
> +  int i;
> +  struct user_sve_header *header = (struct user_sve_header *) buf;
> +  long vq, vg_regcache;
> +
> +  vq = sve_vq_from_vl (header->vl);
> +
> +  /* Sanity check the data in the header.  */
> +  gdb_assert (sve_vl_valid (header->vl));
> +  gdb_assert (SVE_PT_SIZE (vq, header->flags) == header->size);

Again, we shouldn't use gdb_assert here, since this validates external input.

> +
> +  regcache->raw_collect (AARCH64_SVE_VG_REGNUM, &vg_regcache);

When fetching registers, won't it be usually to fill a shiny new, empty regcache?
In that case, won't it always fall into the "if" branch?

In any case, should we check the status of the VG register to make sure it's REG_VALID
before we try to collect it?

> +  if (vg_regcache == 0)
> +    {
> +      /* VG has not been set.  */
> +      vg_regcache = sve_vg_from_vl (header->vl);
> +      regcache->raw_supply (AARCH64_SVE_VG_REGNUM, &vg_regcache);
> +    }
> +  else if (vg_regcache != 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 (_("Vector length has changed (%ld to %d). "
> +		 "Vector registers may show incorrect data."),

Perhaps mention "SVE vector length has changed..."?  Otherwise the user may wonder
what vectors we are talking about.

> +		 vg_regcache, 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++)
> +	regcache->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++)
> +	regcache->raw_supply (AARCH64_SVE_P0_REGNUM + i,
> +		    base + SVE_PT_SVE_PREG_OFFSET (vq, i));
> +
> +      regcache->raw_supply (AARCH64_SVE_FFR_REGNUM,
> +		  base + SVE_PT_SVE_FFR_OFFSET (vq));
> +      regcache->raw_supply (AARCH64_FPSR_REGNUM,
> +		  base + SVE_PT_SVE_FPSR_OFFSET (vq));
> +      regcache->raw_supply (AARCH64_FPCR_REGNUM,
> +		  base + SVE_PT_SVE_FPCR_OFFSET (vq));

Align the second line with the first argument.  Here's an example, though
using spaces instead of tabs to make sure (hopefully) the mail clients display it
correctly.

        regcache->raw_supply (AARCH64_SVE_Z0_REGNUM + i,
                              base + SVE_PT_SVE_ZREG_OFFSET (vq, i));

There are many instances throughout this file.

Simon