[PATCH 0/8] Add SVE support for Aarch64 GDB

classic Classic list List threaded Threaded
47 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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

Simon Marchi-2
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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/8] Add Aarch64 SVE target description

Alan Hayward
In reply to this post by Simon Marchi-2
Thanks for the reviews.
Pushed this with changes as below.

> On 31 May 2018, at 12:09, Simon Marchi <[hidden email]> wrote:
>
> On 2018-05-11 06:52 AM, Alan Hayward wrote:
>> This patch adds the SVE target description. However, no code will
>> yet use it - that comes in the later patches.
>>
>> The create_feature_aarch64_sve function is not generated from XML.
>> This is because we need to know the sve vector size (VQ) in order
>> to size the registers correctly.
>>
>> A VQ of 0 is used when the hardware does not support SVE.
>> (SVE hardware will always have a valid vector size). I considered
>> using a bool to indicate SVE in addition to the VQ. Whilst this
>> may be slightly more readable initially, I think it's a little
>> odd to have two variables, eg:
>> aarch64_create_target_description (bool sve_supported, long vq)
>>
>> Alan.
>
> Hi Alan,
>
> This patch LGTM, I just noted some nits.
>
>> /* Initialize the current architecture based on INFO.  If possible,
>> @@ -2864,7 +2875,8 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>>
>>   /* Ensure we always have a target descriptor.  */
>>   if (!tdesc_has_registers (tdesc))
>> -    tdesc = aarch64_read_description ();
>> +    /* SVE is not yet supported.  */
>> +    tdesc = aarch64_read_description (0);
>
> When there there is a comment above the single statement branch, braces become required:
>
>  if (!tdesc_has_registers (tdesc))
>    {
>      /* SVE is not yet supported.  */
>      tdesc = aarch64_read_description (0);
>    }

ok.

>
>> diff --git a/gdb/arch/aarch64.c b/gdb/arch/aarch64.c
>> index b85e460b6b..d1ec5cedf8 100644
>> --- a/gdb/arch/aarch64.c
>> +++ b/gdb/arch/aarch64.c
>> @@ -21,11 +21,13 @@
>>
>> #include "../features/aarch64-core.c"
>> #include "../features/aarch64-fpu.c"
>> +#include "../features/aarch64-sve.c"
>>
>> -/* Create the aarch64 target description.  */
>> +/* Create the aarch64 target description.  A non zero VQ value indicates both
>> +   the presence of SVE and the SVE vector quotient.  */
>
> What does "SVE vector quotient" mean?  Is there maybe a simpler way to say it?
>

It’s explained in a later patch, but I should have explained it here.
Updated to (and moved into header):

/* Create the aarch64 target description.  A non zero VQ value indicates both
   the presence of SVE and the Vector Quotient - the number of 128bit chunks in
   an SVE Z register.  */


> Could you move this comment to the .h and put
>
> /* See arch/aarch64.h.  */
>
> here?

Ok.

>
>> diff --git a/gdb/features/aarch64-sve.c b/gdb/features/aarch64-sve.c
>> new file mode 100644
>> index 0000000000..6442640a73
>> --- /dev/null
>> +++ b/gdb/features/aarch64-sve.c
>> @@ -0,0 +1,158 @@
>> +/* Copyright (C) 2017 Free Software Foundation, Inc.
>
> 2018
>

Ok.

Alan.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/8] Function for reading the Aarch64 SVE vector length.

Alan Hayward
In reply to this post by Simon Marchi-2

Pushed with changes as below.

> On 31 May 2018, at 12:44, Simon Marchi <[hidden email]> wrote:
>
> Hi Alan,
>
> LGTM, with some nits.
>
> On 2018-05-11 06:52 AM, Alan Hayward wrote:
>> Add a method for reading the SVE vector length using ptrace. This returns
>> 0 for systems without SVE support.
>>
>> Note the defines taken from Linux kernel headers in aarch64-sve-linux-ptrace.h.
>> See the covering email for details about this.
>
> Using ptrace to read the SVE registers (and the SVE register length) will work even
> for kernels that didn't have these macros?

(Answering both possible ways I thought the question was asking).

The macros were added to the kernel in the same batch of patches that
added all the Linux sve support. So, the macros should always exist
on SVE enabled kernels.

The macros are only calculating offsets within the variable length
structure. There’s is nothing preventing you using them on (say)
x86 linux 3.0 to parse the contents of a copy of the structure.
It’s possible that they might end up getting used for reading sve
core files.

>
>> There are multiple ways of expressing the vector length. Thankfully these are
>> all wll defined. I've added convertors for going from one to the other.
>> Hopefully this will help to prevent future confusion.
> Copyright 2017 -> 2018 in the new files.

Ok. (I started these patches quite a while ago!)

>
>>
>> 2018-05-11  Alan Hayward  <[hidden email]>
>>
>> gdb/
>> * Makefile.in: Add new header.
>> * gdb/arch/aarch64.h (sve_vg_from_vl): New macro.
>> (sve_vl_from_vg): Likewise.
>> (sve_vq_from_vl): Likewise.
>> (sve_vl_from_vq): Likewise.
>> (sve_vq_from_vg): Likewise.
>> (sve_vg_from_vq): Likewise.
>> * configure.nat: Add new c file.
>> * nat/aarch64-sve-linux-ptrace.c: New file.
>> * nat/aarch64-sve-linux-ptrace.h: New file.
>>
>> gdbserver/
>> * configure.srv: Add new c/h file.
>> ---
>> gdb/Makefile.in                    |  1 +
>> gdb/arch/aarch64.h                 | 17 +++++++++
>> gdb/configure.nat                  |  2 +-
>> gdb/gdbserver/configure.srv        |  1 +
>> gdb/nat/aarch64-sve-linux-ptrace.c | 52 +++++++++++++++++++++++++++
>> gdb/nat/aarch64-sve-linux-ptrace.h | 73 ++++++++++++++++++++++++++++++++++++++
>> 6 files changed, 145 insertions(+), 1 deletion(-)
>> create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.c
>> create mode 100644 gdb/nat/aarch64-sve-linux-ptrace.h
>>
>> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
>> index 87d74a7703..64042d1bd1 100644
>> --- a/gdb/Makefile.in
>> +++ b/gdb/Makefile.in
>> @@ -1478,6 +1478,7 @@ HFILES_NO_SRCDIR = \
>> mi/mi-parse.h \
>> nat/aarch64-linux.h \
>> nat/aarch64-linux-hw-point.h \
>> + nat/aarch64-sve-linux-ptrace.h \
>> nat/amd64-linux-siginfo.h \
>> nat/gdb_ptrace.h \
>> nat/gdb_thread_db.h \
>> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
>> index 1846e04163..af0b157c51 100644
>> --- a/gdb/arch/aarch64.h
>> +++ b/gdb/arch/aarch64.h
>> @@ -48,6 +48,23 @@ enum aarch64_regnum
>> #define AARCH64_V_REGS_NUM 32
>> #define AARCH64_NUM_REGS AARCH64_FPCR_REGNUM + 1
>>
>> +/* There are a number of ways of expressing the current SVE vector size:
>> +
>> +   VL : Vector Length.
>> + The number of bytes in an SVE Z register.
>> +   VQ : Vector Quotient.
>> + The number of 128bit chunks in an SVE Z register.
>> +   VG : Vector Gradient.
>> + The number of 64bit chunks in an SVE Z register.  */
>> +
>> +#define sve_vg_from_vl(vl) ((vl) / 8)
>> +#define sve_vl_from_vg(vg) ((vg) * 8)
>> +#define sve_vq_from_vl(vl) ((vl) / 0x10)
>> +#define sve_vl_from_vq(vq) ((vq) * 0x10)
>> +#define sve_vq_from_vg(vg) (sve_vq_from_vl (sve_vl_from_vg (vg)))
>> +#define sve_vg_from_vq(vq) (sve_vg_from_vl (sve_vl_from_vq (vq)))
>> +
>> +
>> /* Maximum supported VQ value.  Increase if required.  */
>> #define AARCH64_MAX_SVE_VQ  16
>>
>> diff --git a/gdb/configure.nat b/gdb/configure.nat
>> index 6b0f44fede..d7d79adaca 100644
>> --- a/gdb/configure.nat
>> +++ b/gdb/configure.nat
>> @@ -228,7 +228,7 @@ case ${gdb_host} in
>>    aarch64)
>> #  Host: AArch64 based machine running GNU/Linux
>> NATDEPFILES="${NATDEPFILES} aarch64-linux-nat.o \
>> - aarch32-linux-nat.o aarch64-linux-hw-point.o aarch64-linux.o"
>> + aarch32-linux-nat.o aarch64-linux-hw-point.o aarch64-linux.o aarch64-sve-linux-ptrace.o"
>
> Please wrap to 80 columns.

Ok.

>
>> ;;
>>    arm)
>> # Host: ARM based machine running GNU/Linux
>> diff --git a/gdb/gdbserver/configure.srv b/gdb/gdbserver/configure.srv
>> index ffeefb9b92..8bf0dcc650 100644
>> --- a/gdb/gdbserver/configure.srv
>> +++ b/gdb/gdbserver/configure.srv
>> @@ -54,6 +54,7 @@ case "${target}" in
>> srv_tgtobj="$srv_tgtobj arch/aarch64-insn.o"
>> srv_tgtobj="$srv_tgtobj arch/aarch64.o"
>> srv_tgtobj="$srv_tgtobj linux-aarch64-tdesc.o"
>> + srv_tgtobj="$srv_tgtobj aarch64-sve-linux-ptrace.o"
>> srv_tgtobj="${srv_tgtobj} $srv_linux_obj"
>> srv_linux_regsets=yes
>> srv_linux_thread_db=yes
>> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
>> new file mode 100644
>> index 0000000000..9381786fda
>> --- /dev/null
>> +++ b/gdb/nat/aarch64-sve-linux-ptrace.c
>> @@ -0,0 +1,52 @@
>> +/* Common target dependent for AArch64 systems.
>> +
>> +   Copyright (C) 2017 Free Software Foundation, Inc.
>> +
>> +   This file is part of GDB.
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +#include <sys/utsname.h>
>> +#include <sys/uio.h>
>> +#include "common-defs.h"
>> +#include "elf/external.h"
>> +#include "elf/common.h"
>> +#include "aarch64-sve-linux-ptrace.h"
>> +#include "arch/aarch64.h"
>> +
>> +/* 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).  */
>
> Here, put a reference to the .h as usual.

Ok.

>
>> +
>> +unsigned long
>> +aarch64_sve_get_vq (int tid)
>> +{
>> +  struct iovec iovec;
>> +  struct user_sve_header header;
>> +
>> +  iovec.iov_len = sizeof (header);
>> +  iovec.iov_base = &header;
>> +
>> +  /* Ptrace gives the vector length in bytes.  Convert it to VQ, the number of
>> +     128bit chunks in a Z register.  We use VQ because 128bits is the minimum
>> +     a Z register can increase in size.  */
>> +
>> +  if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec) < 0)
>> +    /* SVE is not supported.  */
>> +    return 0;
>
> Add braces here.

Ok.

>
>> +
>> +  long vq = sve_vq_from_vl (header.vl);
>> +  gdb_assert (sve_vl_valid (header.vl));
>
> We should avoid gdb_assert for bad input values (including what we receive from the
> kernel).  Could we display a warning and return 0?

Changed to:

+  if (!sve_vl_valid (header.vl))
+    {
+      warning (_("Invalid SVE state from kernel; SVE disabled."));
+      return 0;
+    }

>
>> +
>> +  return vq;
>> +}
>> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.h b/gdb/nat/aarch64-sve-linux-ptrace.h
>> new file mode 100644
>> index 0000000000..b318150ac1
>> --- /dev/null
>> +++ b/gdb/nat/aarch64-sve-linux-ptrace.h
>> @@ -0,0 +1,73 @@
>> +/* Common target dependent for AArch64 systems.
>> +
>> +   Copyright (C) 2017 Free Software Foundation, Inc.
>> +
>> +   This file is part of GDB.
>> +
>> +   This program is free software; you can redistribute it and/or modify
>> +   it under the terms of the GNU General Public License as published by
>> +   the Free Software Foundation; either version 3 of the License, or
>> +   (at your option) any later version.
>> +
>> +   This program is distributed in the hope that it will be useful,
>> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +   GNU General Public License for more details.
>> +
>> +   You should have received a copy of the GNU General Public License
>> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
>> +
>> +#ifndef AARCH64_SVE_LINUX_PTRACE_H
>> +#define AARCH64_SVE_LINUX_PTRACE_H
>> +
>> +/* Where indicated, this file contains defines and macros lifted directly from
>> +   the Linux kernel headers, with no modification.
>> +   Refer to Linux kernel documentation for details.  */
>> +
>> +#include <asm/sigcontext.h>
>> +#include <sys/utsname.h>
>> +#include <sys/ptrace.h>
>> +#include <asm/ptrace.h>
>> +
>> +/* 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).  */
>
> zeo -> zero.

Ok.

>
>> +extern unsigned long aarch64_sve_get_vq (int tid);
>> +
>> +/* Structures and defines taken from sigcontext.h.  */
>> +
>> +#ifndef SVE_SIG_ZREGS_SIZE
>> +
>> +#define SVE_VQ_BYTES 16 /* number of bytes per quadword */
>> +
>> +#define SVE_VQ_MIN 1
>> +#define SVE_VQ_MAX 512
>> +
>> +#define SVE_VL_MIN (SVE_VQ_MIN * SVE_VQ_BYTES)
>> +#define SVE_VL_MAX (SVE_VQ_MAX * SVE_VQ_BYTES)
>> +
>> +#define SVE_NUM_ZREGS 32
>> +#define SVE_NUM_PREGS 16
>> +
>> +#define sve_vl_valid(vl) \
>> + ((vl) % SVE_VQ_BYTES == 0 && (vl) >= SVE_VL_MIN && (vl) <= SVE_VL_MAX)
>> +
>> +#endif /* SVE_SIG_ZREGS_SIZE.  */
>> +
>> +
>> +/* Structures and defines taken from ptrace.h.  */
>> +
>> +#ifndef SVE_PT_SVE_ZREG_SIZE
>> +
>> +struct user_sve_header {
>> + __u32 size; /* total meaningful regset content in bytes */
>> + __u32 max_size; /* maxmium possible size for this thread */
>> + __u16 vl; /* current vector length */
>> + __u16 max_vl; /* maximum possible vector length */
>> + __u16 flags;
>> + __u16 __reserved;
>> +};
>> +
>> +#endif /* SVE_PT_SVE_ZREG_SIZE.  */
>> +
>> +#endif /* aarch64-sve-linux-ptrace.h */

Reply | Threaded
Open this post in threaded view
|

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

Alan Hayward
In reply to this post by Simon Marchi-2


> On 31 May 2018, at 14:22, Simon Marchi <[hidden email]> wrote:
>
> 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.

A new file makes sense (especially with regards to copyright). However, the plan was
to eventually remove the macros when "enough" time has passed - keeping everything
in aarch64-sve-linux-ptrace.h made it simpler than removing files.

Is there a minimum kernel version that gdb requires to link against in order to build?
The relevant defines are in 4.15. The kernel is now on 4.16.9.

I’m guessing this must have come up before when other targets added new kernel features?
I didn’t want to suddenly destroy everyone’s aarch64/targetall build trees without
warning.


>> 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.

I tried using common-defs.h, but gdb/regcache.h requires defines from
defs.h - RequireLongest and maybe others.
Putting defs.h at the top of gdb/regcache.h then broke in a weird way.
A lot of fiddling later, and I hadn’t found a way to make it work.

Creating common/common-regcache.h gets a bit odd because, the functions
I need for gdbserver (raw_supply, raw_collect and get_register_status)
on gdb come from:


class reg_buffer
...
  enum register_status get_register_status (int regnum) const;
...


class readable_regcache : public reg_buffer
...


class detached_regcache : public readable_regcache
...
  void raw_supply (int regnum, const void *buf);
...


class regcache : public detached_regcache
...
  void raw_collect (int regnum, void *buf) const;
...


I don’t think that this would work:
class regcache : public detached_regcache, common_regcache


>
>> +
>> +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.

Yes, I’ll update with one of those. Is there any documentation explaining
when to use which of these?

>
>> +
>> +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.

Ok.

>
>> +
>> +  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?

I thought the same regcache was used each time registers needed reading?
I’ll double check this.
Either way, yes, should probably check REG_VALID

>
>> +  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.

Agreed.

>
>> + 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.
>

I think that’s leftover from when I changed from "raw_supply (regcache,”
to “regcache->raw_supply (“.
I’ll fix it up.


Alan.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/8] Function for reading the Aarch64 SVE vector length.

Pedro Alves-7
In reply to this post by Alan Hayward
On 05/11/2018 11:52 AM, Alan Hayward wrote:
> Add a method for reading the SVE vector length using ptrace. This returns
> 0 for systems without SVE support.
>
> Note the defines taken from Linux kernel headers in aarch64-sve-linux-ptrace.h.
> See the covering email for details about this.

Note that since the cover email doesn't make it to the git repo, it's better to
do things the other way around, leave the details in the commit, and a
summary in the cover letter.  Someone looking at git log won't have a reference
to the cover letter.  Or, if it makes sense for readers of the code, as
opposed to just a rationale for a change, put it in comments instead.  (I don't
know whether that's true, just stating a principle.)

>
> There are multiple ways of expressing the vector length. Thankfully these are
> all wll defined. I've added convertors for going from one to the other.

"well", "converters".

> +struct user_sve_header {
> + __u32 size; /* total meaningful regset content in bytes */
> + __u32 max_size; /* maxmium possible size for this thread */

"maximum"

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 7/8] Add methods to gdbserver regcache and raw_compare

Pedro Alves-7
In reply to this post by Alan Hayward
On 05/11/2018 11:52 AM, Alan Hayward wrote:

> +/* Compare the contents of the register stored in the regcache (ignoring the
> +   first OFFSET bytes) to the contents of BUF (without any offset).  Returns 0
> +   if identical.  */

Is this a tristate return, like memcmp?  If yes, that should be explicitly documented.
If not, I'd suggest renaming the function to something like raw_equals or
raw_contents_eq, flipping the logic around and return a bool, with true
meaning equal.

The description should probably be moved to the .h file.

> +
> +int
> +regcache::raw_compare (int regnum, const void *buf, int offset) const
> +{
> +  gdb_assert (register_size (tdesc, regnum) > offset);
> +  return memcmp (buf, register_data (this, regnum, 1) + offset,
> + register_size (tdesc, regnum) - offset);
> +}
Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/8] Enable SVE for GDB

Pedro Alves-7
In reply to this post by Alan Hayward
On 05/11/2018 11:52 AM, Alan Hayward wrote:
> +  if (feature_sve != NULL)
> +    {
> +      gdb_assert (feature_fpu == NULL);
> +
> +      /* Validate the descriptor provides the mandatory sve registers

"description".  Please "grep -i" your patches for "descriptor", since
this appears in other spots.

Uppercase "SVE", I'd assume.  I noticed that at least patch #8 uses
lowercase "sve" in an error message, which should probably be fixed.
Suggest grepping the patches for that too.

> + and allocate their numbers.  */
> +      for (i = 0; i < ARRAY_SIZE (aarch64_sve_register_names); i++)
> + valid_p &= tdesc_numbered_register (feature_sve, tdesc_data,

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 6/8] Aarch64 SVE pseudo register support

Pedro Alves-7
In reply to this post by Alan Hayward
On 05/11/2018 11:52 AM, Alan Hayward wrote:

> +/* Return the type for an AdvSISD V register.  */
> +
> +static struct type *
> +aarch64_vnv_type (struct gdbarch *gdbarch)
> +{
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +
> +  if (tdep->vnv_type == NULL)
> +    {
> +      struct type *t;
> +      struct type *elem;

ELEM appears unused.

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 5/8] Add aarch64 psuedo help functions

Pedro Alves-7
In reply to this post by Simon Marchi-2
On 05/31/2018 01:06 PM, Simon Marchi wrote:

>> +/* Inner version of aarch64_pseudo_read_value.  */
>> +
>> +static struct value *
>> +aarch64_pseudo_read_value_2 (readable_regcache *regcache, int regnum_offset,
>> +     int regsize, struct value *result_value)
>
> It's not a big deal, but in other GDB parts we actually often use the _1 suffix
> for the helper functions.
>

Also, I'd think "Helper for aarch64_pseudo_read_value." would be clearer,
because "inner version" sounds a bit odd to me.

> But otherwise, LGTM.

To me too.

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/8] Enable SVE for GDB

Pedro Alves-7
In reply to this post by Alan Hayward
On 05/11/2018 11:52 AM, Alan Hayward wrote:

> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -2873,6 +2873,26 @@ aarch64_read_description (long vq)
>    return tdesc;
>  }
>  
> +/* Return the VQ used when creating the target description TDESC.  */
> +
> +static long
> +aarch64_get_tdesc_vq (const struct target_desc *tdesc)

Is this use of "long" significant?  I mean, is it assuming 64-bit?
I ask because longs are not 64-bit on x64 Windows, so it would
do the wrong thing when cross debugging.

> +{
> +  const struct tdesc_feature *feature_sve;
> +
> +  if (!tdesc_has_registers (tdesc))
> +    return 0;
> +

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/8] Enable SVE for GDB

Alan Hayward


> On 31 May 2018, at 15:59, Pedro Alves <[hidden email]> wrote:
>
> On 05/11/2018 11:52 AM, Alan Hayward wrote:
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -2873,6 +2873,26 @@ aarch64_read_description (long vq)
>>   return tdesc;
>> }
>>
>> +/* Return the VQ used when creating the target description TDESC.  */
>> +
>> +static long
>> +aarch64_get_tdesc_vq (const struct target_desc *tdesc)
>
> Is this use of "long" significant?  I mean, is it assuming 64-bit?
> I ask because longs are not 64-bit on x64 Windows, so it would
> do the wrong thing when cross debugging.

In the kernel structure it’s a 16bit value.

However, the VG "register" in the regcache is a 64bit value. (It’s not
a real system register, but helps a great deal to see it as one in gdb,
and we need it as a dwarf register.) I made it 64bits to match the
minimum size of all the other aarch64 registers.
I did have it as 16bits at one point, but it went wrong when using it
with dwarf as it expects all the dwarf registers to be the minimum
register.

I chose to do the conversion from 16bits to 64bits at the point vg is
read from the kernel. This makes the code easy as from then on as it’s
always 64bits throughout.

Alternatively, I could have kept as 16bits up to the point it hits
the regcache. But that leaves two different types around the code.


Alan.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/8] Enable SVE for GDB

Pedro Alves-7
On 05/31/2018 05:13 PM, Alan Hayward wrote:

>
>
>> On 31 May 2018, at 15:59, Pedro Alves <[hidden email]> wrote:
>>
>> On 05/11/2018 11:52 AM, Alan Hayward wrote:
>>> --- a/gdb/aarch64-tdep.c
>>> +++ b/gdb/aarch64-tdep.c
>>> @@ -2873,6 +2873,26 @@ aarch64_read_description (long vq)
>>>   return tdesc;
>>> }
>>>
>>> +/* Return the VQ used when creating the target description TDESC.  */
>>> +
>>> +static long
>>> +aarch64_get_tdesc_vq (const struct target_desc *tdesc)
>>
>> Is this use of "long" significant?  I mean, is it assuming 64-bit?
>> I ask because longs are not 64-bit on x64 Windows, so it would
>> do the wrong thing when cross debugging.
>
> In the kernel structure it’s a 16bit value.
>
> However, the VG "register" in the regcache is a 64bit value. (It’s not
> a real system register, but helps a great deal to see it as one in gdb,
> and we need it as a dwarf register.) I made it 64bits to match the
> minimum size of all the other aarch64 registers.

OK, fine to make it 64-bit, but then the "do the wrong thing when
cross debugging" point applies.  "long" is 32-bit on some hosts.
I don't really know what values VG can take -- can they be
negative?  I.e., is sign extension expected?  You did not seem to address
this in your reply.  Why not use host-independent ULONGEST, for example?

> I did have it as 16bits at one point, but it went wrong when using it
> with dwarf as it expects all the dwarf registers to be the minimum
> register.
>
> I chose to do the conversion from 16bits to 64bits at the point vg is
> read from the kernel. This makes the code easy as from then on as it’s
> always 64bits throughout.
>
> Alternatively, I could have kept as 16bits up to the point it hits
> the regcache. But that leaves two different types around the code.

Thanks,
Pedro Alves
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/8] Enable SVE for GDB

Alan Hayward


> On 31 May 2018, at 17:18, Pedro Alves <[hidden email]> wrote:
>
> On 05/31/2018 05:13 PM, Alan Hayward wrote:
>>
>>
>>> On 31 May 2018, at 15:59, Pedro Alves <[hidden email]> wrote:
>>>
>>> On 05/11/2018 11:52 AM, Alan Hayward wrote:
>>>> --- a/gdb/aarch64-tdep.c
>>>> +++ b/gdb/aarch64-tdep.c
>>>> @@ -2873,6 +2873,26 @@ aarch64_read_description (long vq)
>>>>  return tdesc;
>>>> }
>>>>
>>>> +/* Return the VQ used when creating the target description TDESC.  */
>>>> +
>>>> +static long
>>>> +aarch64_get_tdesc_vq (const struct target_desc *tdesc)
>>>
>>> Is this use of "long" significant?  I mean, is it assuming 64-bit?
>>> I ask because longs are not 64-bit on x64 Windows, so it would
>>> do the wrong thing when cross debugging.
>>
>> In the kernel structure it’s a 16bit value.
>>
>> However, the VG "register" in the regcache is a 64bit value. (It’s not
>> a real system register, but helps a great deal to see it as one in gdb,
>> and we need it as a dwarf register.) I made it 64bits to match the
>> minimum size of all the other aarch64 registers.
>
> OK, fine to make it 64-bit, but then the "do the wrong thing when
> cross debugging" point applies.  "long" is 32-bit on some hosts.
> I don't really know what values VG can take -- can they be
> negative?  I.e., is sign extension expected?  You did not seem to address
> this in your reply.  Why not use host-independent ULONGEST, for example?

Right, I didn’t spot that. It can’t be negative, only 0 or a value
divisible by 2.
To match the register, I’m thinking uint64_t would be right then.


Alan.
Reply | Threaded
Open this post in threaded view
|

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

Simon Marchi-2
In reply to this post by Alan Hayward
On 2018-05-11 06:52 AM, Alan Hayward wrote:

> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index 1270b7e0c0..bc570ade6d 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -388,19 +388,65 @@ 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)
> +{
> +  gdb_byte *base = aarch64_sve_get_sveregs (ptid_get_lwp (inferior_ptid));
> +  aarch64_sve_regs_copy_to_regcache (regcache, base);
> +  xfree (base);
> +}
> +
> +/* 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)
> +{
> +  gdb_byte *base;
> +  int ret, tid;
> +  struct iovec iovec;
> +
> +  tid = ptid_get_lwp (inferior_ptid);

I've just noticed these two uses of inferior_ptid.  I think we should get the ptid
from the regcache instead.

Simon
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/8] Add SVE register defines

Alan Hayward
In reply to this post by Alan Hayward
Pedro, Simon:
Thanks for all the reviews.

Part 3 slipped under the net. Could some ok this one?

(Looking at it again, I’m not keen on the ! in the comment, but was
just duplicating the previous comment.)

Thanks,
Alan.

> On 11 May 2018, at 11:52, Alan Hayward <[hidden email]> wrote:
>
> Add all the SVE register defines used by the later patches.
>
> In order to prevent gaps in the register numbering, the Z registers
> reuse the V register numbers (which become pseudos on SVE).
>
> 2018-05-11  Alan Hayward  <[hidden email]>
>
> * aarch64-tdep.c (aarch64_sve_register_names): New const
> var.
> * arch/aarch64.h (enum aarch64_regnum): Add SVE entries.
> (AARCH64_SVE_Z_REGS_NUM): New define.
> (AARCH64_SVE_P_REGS_NUM): Likewise.
> (AARCH64_SVE_NUM_REGS): Likewise.
> ---
> gdb/aarch64-tdep.c | 21 +++++++++++++++++++++
> gdb/arch/aarch64.h | 15 ++++++++++++++-
> 2 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 806a3dac55..1dc31a43bd 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -156,6 +156,27 @@ static const char *const aarch64_v_register_names[] =
>   "fpcr"
> };
>
> +/* The SVE 'Z' and 'P' registers.  */
> +static const char *const aarch64_sve_register_names[] =
> +{
> +  /* These registers must appear in consecutive RAW register number
> +     order and they must begin with AARCH64_SVE_Z0_REGNUM! */
> +  "z0", "z1", "z2", "z3",
> +  "z4", "z5", "z6", "z7",
> +  "z8", "z9", "z10", "z11",
> +  "z12", "z13", "z14", "z15",
> +  "z16", "z17", "z18", "z19",
> +  "z20", "z21", "z22", "z23",
> +  "z24", "z25", "z26", "z27",
> +  "z28", "z29", "z30", "z31",
> +  "fpsr", "fpcr",
> +  "p0", "p1", "p2", "p3",
> +  "p4", "p5", "p6", "p7",
> +  "p8", "p9", "p10", "p11",
> +  "p12", "p13", "p14", "p15",
> +  "ffr", "vg"
> +};
> +
> /* AArch64 prologue cache structure.  */
> struct aarch64_prologue_cache
> {
> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
> index af0b157c51..9855e6f286 100644
> --- a/gdb/arch/aarch64.h
> +++ b/gdb/arch/aarch64.h
> @@ -24,7 +24,9 @@
>
> target_desc *aarch64_create_target_description (long vq);
>
> -/* Register numbers of various important registers.  */
> +/* Register numbers of various important registers.
> +   Note that on SVE, the Z registers reuse the V register numbers and the V
> +   registers become pseudo registers.  */
> enum aarch64_regnum
> {
>   AARCH64_X0_REGNUM, /* First integer register.  */
> @@ -35,8 +37,15 @@ enum aarch64_regnum
>   AARCH64_CPSR_REGNUM, /* Current Program Status Register.  */
>   AARCH64_V0_REGNUM, /* First fp/vec register.  */
>   AARCH64_V31_REGNUM = AARCH64_V0_REGNUM + 31, /* Last fp/vec register.  */
> +  AARCH64_SVE_Z0_REGNUM = AARCH64_V0_REGNUM, /* First SVE Z register.  */
> +  AARCH64_SVE_Z31_REGNUM = AARCH64_V31_REGNUM,  /* Last SVE Z register.  */
>   AARCH64_FPSR_REGNUM, /* Floating Point Status Register.  */
>   AARCH64_FPCR_REGNUM, /* Floating Point Control Register.  */
> +  AARCH64_SVE_P0_REGNUM, /* First SVE predicate register.  */
> +  AARCH64_SVE_P15_REGNUM = AARCH64_SVE_P0_REGNUM + 15, /* Last SVE predicate
> +   register.  */
> +  AARCH64_SVE_FFR_REGNUM, /* SVE First Fault Register.  */
> +  AARCH64_SVE_VG_REGNUM, /* SVE Vector Gradient.  */
>
>   /* Other useful registers.  */
>   AARCH64_LAST_X_ARG_REGNUM = AARCH64_X0_REGNUM + 7,
> @@ -46,7 +55,11 @@ enum aarch64_regnum
>
> #define AARCH64_X_REGS_NUM 31
> #define AARCH64_V_REGS_NUM 32
> +#define AARCH64_SVE_Z_REGS_NUM AARCH64_V_REGS_NUM
> +#define AARCH64_SVE_P_REGS_NUM 16
> #define AARCH64_NUM_REGS AARCH64_FPCR_REGNUM + 1
> +#define AARCH64_SVE_NUM_REGS AARCH64_SVE_VG_REGNUM + 1
> +
>
> /* There are a number of ways of expressing the current SVE vector size:
>
> --
> 2.15.1 (Apple Git-101)
>

Reply | Threaded
Open this post in threaded view
|

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

Simon Marchi-2
In reply to this post by Alan Hayward
On 2018-05-31 10:46 AM, Alan Hayward wrote:

>
>
>> On 31 May 2018, at 14:22, Simon Marchi <[hidden email]> wrote:
>>
>> 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.
>
> A new file makes sense (especially with regards to copyright). However, the plan was
> to eventually remove the macros when "enough" time has passed - keeping everything
> in aarch64-sve-linux-ptrace.h made it simpler than removing files.

How come?  Removing a file is not complicated.

> Is there a minimum kernel version that gdb requires to link against in order to build?
> The relevant defines are in 4.15. The kernel is now on 4.16.9.
>
> I’m guessing this must have come up before when other targets added new kernel features?
> I didn’t want to suddenly destroy everyone’s aarch64/targetall build trees without
> warning.

I don't think there's a specific Linux kernel version we require to build GDB, but it's
good to support as far back as possible with a reasonable effort.  Since they have only
been added very recently, I would keep our copy of the macros for a while, if it's not
in the way of anything.

>>> 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.
>
> I tried using common-defs.h, but gdb/regcache.h requires defines from
> defs.h - RequireLongest and maybe others.
> Putting defs.h at the top of gdb/regcache.h then broke in a weird way.
> A lot of fiddling later, and I hadn’t found a way to make it work.
>
> Creating common/common-regcache.h gets a bit odd because, the functions
> I need for gdbserver (raw_supply, raw_collect and get_register_status)
> on gdb come from:
>
>
> class reg_buffer
> ...
>   enum register_status get_register_status (int regnum) const;
> ...
>
>
> class readable_regcache : public reg_buffer
> ...
>
>
> class detached_regcache : public readable_regcache
> ...
>   void raw_supply (int regnum, const void *buf);
> ...
>
>
> class regcache : public detached_regcache
> ...
>   void raw_collect (int regnum, void *buf) const;
> ...
>
>
> I don’t think that this would work:
> class regcache : public detached_regcache, common_regcache

I did some quick hacking and it seems to work to have this in common/common-regcache.h

struct reg_buffer_common
{
  virtual ~reg_buffer_common () = default;
  virtual void raw_supply (int regnum, const void *buf) = 0;
  virtual void raw_collect (int regnum, void *buf) const = 0;
  virtual bool raw_compare (int regnum, const void *buf, int offset) const = 0;
  virtual register_status get_register_status (int regnum) const = 0;
};

and make your code in nat/ take a pointer to reg_buffer_common.  gdb's reg_buffer
and gdbserver's regcache should inherit from reg_buffer_common.  I  built
it on other regcache refactor patches I have in the pipeline, so maybe some other
changes in there are needed, maybe not.  I pushed the result on this branch, it's
a bit raw but it should be enough to show the idea.

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/simark/regcache-for-alan

>> Since this returns allocated memory, could we return an RAII object?  Either
>> std::vector, std::unique_ptr or gdb::unique_xmalloc_ptr.
>
> Yes, I’ll update with one of those. Is there any documentation explaining
> when to use which of these?

I don't think so, if you think it would be relevant we could add it to the wiki.

In short:

std::vector: Allocates and manages its own memory.  This is good when we allocate
the space ourselves in GDB, and is especially good for buffers that need to change
size over time.  By default, an std::vector<gdb_byte> would zero-out the buffer
on allocation.  To avoid that when we don't need it, we have a specialization,
gdb::byte_vector, that leaves the memory uninitialized.  You could

gdb::byte_vector buf (iovec.iov_len);
iovec.iov_base = buf.data ();

...

return buf;

std::unique_ptr: It is used to wrap a pointer to something that has been allocated
with "new", it will free it with "delete".  You could do

std::unique_ptr<gdb_byte> buf (new gdb_byte[iovec.iov_len]);
iovec.iov_base = buf.get ()

...

return buf;

gdb::unique_xmalloc_ptr: It is like std::unique_ptr, but frees the memory using
xfree.  It is useful to gradually add some RAII to GDB code that uses xmalloc.

For new code, I would either use std::vector or std::unique_ptr.

I just noticed that in the code in the current patch leaks the allocated memory
if ptrace fails and we call perror_with_name.  Using an object that manages
memory will fix that.

>> 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?
>
> I thought the same regcache was used each time registers needed reading?
> I’ll double check this.
> Either way, yes, should probably check REG_VALID

Hmm I'm not sure either.  Indeed it would make sense that we re-use the same regcache.
Thanks for double-checking.

Thanks,

Simon
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/8] Add SVE register defines

Simon Marchi-2
In reply to this post by Alan Hayward
On 2018-06-01 04:33 AM, Alan Hayward wrote:

> Pedro, Simon:
> Thanks for all the reviews.
>
> Part 3 slipped under the net. Could some ok this one?
>
> (Looking at it again, I’m not keen on the ! in the comment, but was
> just duplicating the previous comment.)
>
> Thanks,
> Alan.
>
>> On 11 May 2018, at 11:52, Alan Hayward <[hidden email]> wrote:
>>
>> Add all the SVE register defines used by the later patches.
>>
>> In order to prevent gaps in the register numbering, the Z registers
>> reuse the V register numbers (which become pseudos on SVE).

Ah sorry, yes this LGTM.

Simon
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/8] Enable SVE for GDB

Alan Hayward
In reply to this post by Simon Marchi-2
Push with minor changes as suggested below.

> On 31 May 2018, at 12:55, Simon Marchi <[hidden email]> wrote:
>
> On 2018-05-11 06:52 AM, Alan Hayward wrote:
>> This patch enables SVE support for GDB by reading the VQ when
>> creating a target description.
>>
>> It also ensures that SVE is taken into account when creating
>> the tdep structure, and stores the current VQ value directly
>> in tdep.
>>
>> With this patch, gdb on an aarch64 system with SVE will now detect
>> SVE. The SVE registers will be displayed (but the contents will be
>> invalid). The following patches fill out the contents.
>
> LGTM, with some nits.
>
>> @@ -2911,25 +2933,45 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>>   /* Validate the descriptor provides the mandatory core R registers
>>      and allocate their numbers.  */
>>   for (i = 0; i < ARRAY_SIZE (aarch64_r_register_names); i++)
>> -    valid_p &=
>> -      tdesc_numbered_register (feature, tdesc_data, AARCH64_X0_REGNUM + i,
>> -       aarch64_r_register_names[i]);
>> +    valid_p &= tdesc_numbered_register (feature_core, tdesc_data,
>> + AARCH64_X0_REGNUM + i,
>> + aarch64_r_register_names[i]);
>>
>>   num_regs = AARCH64_X0_REGNUM + i;
>>
>> -  /* Look for the V registers.  */
>> -  feature = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.fpu");
>> -  if (feature)
>> +  /* Add the V registers.  */
>> +  if (feature_fpu != NULL)
>>     {
>> +      gdb_assert (feature_sve == NULL);
>
> Again, if this situation can result from a bad input passed to GDB (a bad tdesc
> sent by the remote), we shouldn't gdb_assert on it, but show an error message
> and gracefully fail.

Ok. Updated to use _error.

      if (feature_sve != NULL)
        error (_("Program contains both fpu and SVE features."));

>
>> +
>>       /* Validate the descriptor provides the mandatory V registers
>> -         and allocate their numbers.  */
>> + and allocate their numbers.  */
>>       for (i = 0; i < ARRAY_SIZE (aarch64_v_register_names); i++)
>> - valid_p &=
>> -  tdesc_numbered_register (feature, tdesc_data, AARCH64_V0_REGNUM + i,
>> -   aarch64_v_register_names[i]);
>> + valid_p &= tdesc_numbered_register (feature_fpu, tdesc_data,
>> +    AARCH64_V0_REGNUM + i,
>> +    aarch64_v_register_names[i]);
>>
>>       num_regs = AARCH64_V0_REGNUM + i;
>> +    }
>> +
>> +  /* Add the SVE registers.  */
>> +  if (feature_sve != NULL)
>> +    {
>> +      gdb_assert (feature_fpu == NULL);
>
> Same here.

I removed this one as it’s redundant - it’ll be caught by the error above.

>
>> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
>> index c9fd7b3578..046de6228f 100644
>> --- a/gdb/aarch64-tdep.h
>> +++ b/gdb/aarch64-tdep.h
>> @@ -73,6 +73,15 @@ struct gdbarch_tdep
>>
>>   /* syscall record.  */
>>   int (*aarch64_syscall_record) (struct regcache *regcache, unsigned long svc_number);
>> +
>> +  /* The VQ value for SVE targets, or zero if SVE is not supported.  */
>> +  long vq;
>> +
>> +  /* Returns true if the target supports SVE.  */
>> +  bool has_sve ()
>> +  {
>> +    return vq != 0;
>> +  }
>
> This method can be const.
>

Done.


> On 31 May 2018, at 15:59, Pedro Alves <[hidden email]> wrote:
>
> On 05/11/2018 11:52 AM, Alan Hayward wrote:
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -2873,6 +2873,26 @@ aarch64_read_description (long vq)
>>   return tdesc;
>> }
>>
>> +/* Return the VQ used when creating the target description TDESC.  */
>> +
>> +static long
>> +aarch64_get_tdesc_vq (const struct target_desc *tdesc)
>
> Is this use of "long" significant?  I mean, is it assuming 64-bit?
> I ask because longs are not 64-bit on x64 Windows, so it would
> do the wrong thing when cross debugging.
>

Fixed by using both follow on patch "[PATCH] Use uint64_t for SVE VQ” and
obvious patch below:


diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index 6674b7654e..0172e4ccd1 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -2875,7 +2875,7 @@ aarch64_read_description (uint64_t vq)

 /* Return the VQ used when creating the target description TDESC.  */

-static long
+static uint64_t
 aarch64_get_tdesc_vq (const struct target_desc *tdesc)
 {
   const struct tdesc_feature *feature_sve;
@@ -2888,7 +2888,8 @@ aarch64_get_tdesc_vq (const struct target_desc *tdesc)
   if (feature_sve == nullptr)
     return 0;

-  long vl = tdesc_register_size (feature_sve, aarch64_sve_register_names[0]);
+  uint64_t vl = tdesc_register_size (feature_sve,
+                                    aarch64_sve_register_names[0]);
   return sve_vq_from_vl (vl);
 }

diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
index b6b9b30e71..598a0aafa2 100644
--- a/gdb/aarch64-tdep.h
+++ b/gdb/aarch64-tdep.h
@@ -75,7 +75,7 @@ struct gdbarch_tdep
   int (*aarch64_syscall_record) (struct regcache *regcache, unsigned long svc_number);

   /* The VQ value for SVE targets, or zero if SVE is not supported.  */
-  long vq;
+  uint64_t vq;

   /* Returns true if the target supports SVE.  */
   bool has_sve () const



Thanks!
Alan.


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 5/8] Add aarch64 psuedo help functions

Alan Hayward
In reply to this post by Simon Marchi-2
Pushed with minor changes as suggested below.

> On 31 May 2018, at 13:06, Simon Marchi <[hidden email]> wrote:
>
> psuedo -> pseudo in the title.

Ok.

>
> On 2018-05-11 06:52 AM, Alan Hayward wrote:
>> Reduce code copy/paste by adding two helper functions for
>> aarch64_pseudo_read_value and aarch64_pseudo_write
>> The patch does not change any functionality.
>>
>> 2018-05-11  Alan Hayward  <[hidden email]>
>>
>> * aarch64-tdep.c (aarch64_pseudo_read_value_2): New helper func.
>> (aarch64_pseudo_write_2): Likewise.
>> (aarch64_pseudo_read_value): Use helper.
>> (aarch64_pseudo_write): Likewise.
>> ---
>> gdb/aarch64-tdep.c | 173 +++++++++++++++++------------------------------------
>> 1 file changed, 54 insertions(+), 119 deletions(-)
>>
>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>> index 7893579d58..003fefb3c9 100644
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -2247,109 +2247,67 @@ aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
>>   return group == all_reggroup;
>> }
>>
>> +/* Inner version of aarch64_pseudo_read_value.  */
>> +
>> +static struct value *
>> +aarch64_pseudo_read_value_2 (readable_regcache *regcache, int regnum_offset,
>> +     int regsize, struct value *result_value)
>
> It's not a big deal, but in other GDB parts we actually often use the _1 suffix
> for the helper functions.

Changed to aarch64_pseudo_read_value_1 and aarch64_pseudo_write_1.

>
> But otherwise, LGTM.
>
> Simon


>
> On 31 May 2018, at 15:57, Pedro Alves <[hidden email]> wrote:
>
> On 05/31/2018 01:06 PM, Simon Marchi wrote:
>
>>> +/* Inner version of aarch64_pseudo_read_value.  */
>>> +
>>> +static struct value *
>>> +aarch64_pseudo_read_value_2 (readable_regcache *regcache, int regnum_offset,
>>> +     int regsize, struct value *result_value)
>>
>> It's not a big deal, but in other GDB parts we actually often use the _1 suffix
>> for the helper functions.
>>
>
> Also, I'd think "Helper for aarch64_pseudo_read_value." would be clearer,
> because "inner version" sounds a bit odd to me.

Updated as suggested. Also updated write function too.

>
>> But otherwise, LGTM.
>
> To me too.
>
> Thanks,
> Pedro Alves


Thanks!
Alan.



Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 6/8] Aarch64 SVE pseudo register support

Alan Hayward
In reply to this post by Simon Marchi-2
Update of patch posted below.


> On 31 May 2018, at 15:57, Pedro Alves <[hidden email]> wrote:
>
> On 05/11/2018 11:52 AM, Alan Hayward wrote:
>
>> +/* Return the type for an AdvSISD V register.  */
>> +
>> +static struct type *
>> +aarch64_vnv_type (struct gdbarch *gdbarch)
>> +{
>> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>> +
>> +  if (tdep->vnv_type == NULL)
>> +    {
>> +      struct type *t;
>> +      struct type *elem;
>
> ELEM appears unused.
>

Leftover from copy paste of previous block of code. Removed.



> On 31 May 2018, at 13:21, Simon Marchi <[hidden email]> wrote:
>
> On 2018-05-11 06:52 AM, Alan Hayward wrote:
>> Add the functionality for reading/writing psuedo registers.
>
> “pseudo"

Done.

>>
>> static struct value *
>> -aarch64_pseudo_read_value_2 (readable_regcache *regcache, int regnum_offset,
>> +aarch64_pseudo_read_value_2 (struct gdbarch *gdbarch,
>> +     readable_regcache *regcache, int regnum_offset,
>>     int regsize, struct value *result_value)
>> {
>> -  gdb_byte reg_buf[V_REGISTER_SIZE];
>> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>> +  gdb_byte v_buf[V_REGISTER_SIZE], *reg_buf;
>> +  gdb_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
>>   unsigned v_regnum = AARCH64_V0_REGNUM + regnum_offset;
>>
>> +  /* Enough space to read a full vector register.  */
>> +  if (tdep->has_sve ())
>> +    reg_buf = (gdb_byte *) xmalloc (register_size (gdbarch, AARCH64_V0_REGNUM));
>> +  else
>> +    reg_buf = v_buf;
>
> If the size of a register (even with SVE) will always be reasonable to allocate on the
> stack, maybe you could just use
>
>  gdb_byte reg_buf[register_size (gdbarch, AARCH64_V0_REGNUM)];
>
> (If there is always a register with number AARCH64_V0_REGNUM, that is)
>
> Otherwise, it would be good to use an std::unique_ptr/gdb::unique_xmalloc_ptr to avoid
> the manual xfree.
>
> Same in aarch64_pseudo_write_2.
>

Thinking about this a little more, on an SVE system, a compiler
spill of a SVE register is going to result in it going to the
stack anyway.
Max size of an SVE register we support is 256 bytes, which
doesn't seem unreasonable to add to the stack.

Updated to use an array. Are you happy with the new version?


Thanks,
Alan.


diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
index b6b9b30e715291fb0512f214a88ce49445fb290b..76243b7de53483bf8394adc1f7c493798da25b06 100644
--- a/gdb/aarch64-tdep.h
+++ b/gdb/aarch64-tdep.h
@@ -70,6 +70,7 @@ struct gdbarch_tdep
   struct type *vns_type;
   struct type *vnh_type;
   struct type *vnb_type;
+  struct type *vnv_type;

   /* syscall record.  */
   int (*aarch64_syscall_record) (struct regcache *regcache, unsigned long svc_number);
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index b752338178f28a1169234a76941a1546bbc9dfa7..f04729f402ec56416cd772e5ca94f0681ea85beb 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -69,6 +69,7 @@
 #define AARCH64_S0_REGNUM (AARCH64_D0_REGNUM + 32)
 #define AARCH64_H0_REGNUM (AARCH64_S0_REGNUM + 32)
 #define AARCH64_B0_REGNUM (AARCH64_H0_REGNUM + 32)
+#define AARCH64_SVE_V0_REGNUM (AARCH64_B0_REGNUM + 32)

 /* All possible aarch64 target descriptors.  */
 struct target_desc *tdesc_aarch64_list[AARCH64_MAX_SVE_VQ + 1];
@@ -1766,6 +1767,30 @@ aarch64_vnb_type (struct gdbarch *gdbarch)
   return tdep->vnb_type;
 }

+/* Return the type for an AdvSISD V register.  */
+
+static struct type *
+aarch64_vnv_type (struct gdbarch *gdbarch)
+{
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
+  if (tdep->vnv_type == NULL)
+    {
+      struct type *t = arch_composite_type (gdbarch, "__gdb_builtin_type_vnv",
+    TYPE_CODE_UNION);
+
+      append_composite_type_field (t, "d", aarch64_vnd_type (gdbarch));
+      append_composite_type_field (t, "s", aarch64_vns_type (gdbarch));
+      append_composite_type_field (t, "h", aarch64_vnh_type (gdbarch));
+      append_composite_type_field (t, "b", aarch64_vnb_type (gdbarch));
+      append_composite_type_field (t, "q", aarch64_vnq_type (gdbarch));
+
+      tdep->vnv_type = t;
+    }
+
+  return tdep->vnv_type;
+}
+
 /* Implement the "dwarf2_reg_to_regnum" gdbarch method.  */

 static int
@@ -2114,6 +2139,8 @@ aarch64_gen_return_address (struct gdbarch *gdbarch,
 static const char *
 aarch64_pseudo_register_name (struct gdbarch *gdbarch, int regnum)
 {
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
   static const char *const q_name[] =
     {
       "q0", "q1", "q2", "q3",
@@ -2191,6 +2218,25 @@ aarch64_pseudo_register_name (struct gdbarch *gdbarch, int regnum)
   if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
     return b_name[regnum - AARCH64_B0_REGNUM];

+  if (tdep->has_sve ())
+    {
+      static const char *const sve_v_name[] =
+ {
+  "v0", "v1", "v2", "v3",
+  "v4", "v5", "v6", "v7",
+  "v8", "v9", "v10", "v11",
+  "v12", "v13", "v14", "v15",
+  "v16", "v17", "v18", "v19",
+  "v20", "v21", "v22", "v23",
+  "v24", "v25", "v26", "v27",
+  "v28", "v29", "v30", "v31",
+ };
+
+      if (regnum >= AARCH64_SVE_V0_REGNUM
+  && regnum < AARCH64_SVE_V0_REGNUM + AARCH64_V_REGS_NUM)
+ return sve_v_name[regnum - AARCH64_SVE_V0_REGNUM];
+    }
+
   internal_error (__FILE__, __LINE__,
   _("aarch64_pseudo_register_name: bad register number %d"),
   regnum);
@@ -2201,6 +2247,8 @@ aarch64_pseudo_register_name (struct gdbarch *gdbarch, int regnum)
 static struct type *
 aarch64_pseudo_register_type (struct gdbarch *gdbarch, int regnum)
 {
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
   regnum -= gdbarch_num_regs (gdbarch);

   if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32)
@@ -2218,6 +2266,10 @@ aarch64_pseudo_register_type (struct gdbarch *gdbarch, int regnum)
   if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
     return aarch64_vnb_type (gdbarch);

+  if (tdep->has_sve () && regnum >= AARCH64_SVE_V0_REGNUM
+      && regnum < AARCH64_SVE_V0_REGNUM + 32)
+    return aarch64_vnv_type (gdbarch);
+
   internal_error (__FILE__, __LINE__,
   _("aarch64_pseudo_register_type: bad register number %d"),
   regnum);
@@ -2229,6 +2281,8 @@ static int
 aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
     struct reggroup *group)
 {
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
   regnum -= gdbarch_num_regs (gdbarch);

   if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32)
@@ -2243,6 +2297,9 @@ aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
     return group == all_reggroup || group == vector_reggroup;
   else if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
     return group == all_reggroup || group == vector_reggroup;
+  else if (tdep->has_sve () && regnum >= AARCH64_SVE_V0_REGNUM
+   && regnum < AARCH64_SVE_V0_REGNUM + 32)
+    return group == all_reggroup || group == vector_reggroup;

   return group == all_reggroup;
 }
@@ -2250,17 +2307,22 @@ aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
 /* Helper for aarch64_pseudo_read_value.  */

 static struct value *
-aarch64_pseudo_read_value_1 (readable_regcache *regcache, int regnum_offset,
+aarch64_pseudo_read_value_1 (struct gdbarch *gdbarch,
+     readable_regcache *regcache, int regnum_offset,
      int regsize, struct value *result_value)
 {
-  gdb_byte reg_buf[V_REGISTER_SIZE];
   unsigned v_regnum = AARCH64_V0_REGNUM + regnum_offset;

+  /* Enough space for a full vector register.  */
+  gdb_byte reg_buf[register_size (gdbarch, AARCH64_V0_REGNUM)];
+  gdb_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
+
   if (regcache->raw_read (v_regnum, reg_buf) != REG_VALID)
     mark_value_bytes_unavailable (result_value, 0,
   TYPE_LENGTH (value_type (result_value)));
   else
     memcpy (value_contents_raw (result_value), reg_buf, regsize);
+
   return result_value;
  }

@@ -2270,6 +2332,7 @@ static struct value *
 aarch64_pseudo_read_value (struct gdbarch *gdbarch, readable_regcache *regcache,
    int regnum)
 {
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
   struct value *result_value = allocate_value (register_type (gdbarch, regnum));

   VALUE_LVAL (result_value) = lval_register;
@@ -2278,42 +2341,56 @@ aarch64_pseudo_read_value (struct gdbarch *gdbarch, readable_regcache *regcache,
   regnum -= gdbarch_num_regs (gdbarch);

   if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32)
-    return aarch64_pseudo_read_value_1 (regcache, regnum - AARCH64_Q0_REGNUM,
+    return aarch64_pseudo_read_value_1 (gdbarch, regcache,
+ regnum - AARCH64_Q0_REGNUM,
  Q_REGISTER_SIZE, result_value);

   if (regnum >= AARCH64_D0_REGNUM && regnum < AARCH64_D0_REGNUM + 32)
-    return aarch64_pseudo_read_value_1 (regcache, regnum - AARCH64_D0_REGNUM,
+    return aarch64_pseudo_read_value_1 (gdbarch, regcache,
+ regnum - AARCH64_D0_REGNUM,
  D_REGISTER_SIZE, result_value);

   if (regnum >= AARCH64_S0_REGNUM && regnum < AARCH64_S0_REGNUM + 32)
-    return aarch64_pseudo_read_value_1 (regcache, regnum - AARCH64_S0_REGNUM,
+    return aarch64_pseudo_read_value_1 (gdbarch, regcache,
+ regnum - AARCH64_S0_REGNUM,
  S_REGISTER_SIZE, result_value);

   if (regnum >= AARCH64_H0_REGNUM && regnum < AARCH64_H0_REGNUM + 32)
-    return aarch64_pseudo_read_value_1 (regcache, regnum - AARCH64_H0_REGNUM,
+    return aarch64_pseudo_read_value_1 (gdbarch, regcache,
+ regnum - AARCH64_H0_REGNUM,
  H_REGISTER_SIZE, result_value);

   if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
-    return aarch64_pseudo_read_value_1 (regcache, regnum - AARCH64_B0_REGNUM,
+    return aarch64_pseudo_read_value_1 (gdbarch, regcache,
+ regnum - AARCH64_B0_REGNUM,
  B_REGISTER_SIZE, result_value);

+  if (tdep->has_sve () && regnum >= AARCH64_SVE_V0_REGNUM
+      && regnum < AARCH64_SVE_V0_REGNUM + 32)
+    return aarch64_pseudo_read_value_1 (gdbarch, regcache,
+ regnum - AARCH64_SVE_V0_REGNUM,
+ V_REGISTER_SIZE, result_value);
+
   gdb_assert_not_reached ("regnum out of bound");
 }

 /* Helper for aarch64_pseudo_write.  */

 static void
-aarch64_pseudo_write_1 (struct regcache *regcache, int regnum_offset,
- int regsize, const gdb_byte *buf)
+aarch64_pseudo_write_1 (struct gdbarch *gdbarch, struct regcache *regcache,
+ int regnum_offset, int regsize, const gdb_byte *buf)
 {
-  gdb_byte reg_buf[V_REGISTER_SIZE];
   unsigned v_regnum = AARCH64_V0_REGNUM + regnum_offset;

+  /* Enough space for a full vector register.  */
+  gdb_byte reg_buf[register_size (gdbarch, AARCH64_V0_REGNUM)];
+  gdb_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);
+
   /* Ensure the register buffer is zero, we want gdb writes of the
      various 'scalar' pseudo registers to behavior like architectural
      writes, register width bytes are written the remainder are set to
      zero.  */
-  memset (reg_buf, 0, sizeof (reg_buf));
+  memset (reg_buf, 0, register_size (gdbarch, AARCH64_V0_REGNUM));

   memcpy (reg_buf, buf, regsize);
   regcache->raw_write (v_regnum, reg_buf);
@@ -2325,27 +2402,39 @@ static void
 aarch64_pseudo_write (struct gdbarch *gdbarch, struct regcache *regcache,
       int regnum, const gdb_byte *buf)
 {
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
   regnum -= gdbarch_num_regs (gdbarch);

   if (regnum >= AARCH64_Q0_REGNUM && regnum < AARCH64_Q0_REGNUM + 32)
-    return aarch64_pseudo_write_1 (regcache, regnum - AARCH64_Q0_REGNUM,
-   Q_REGISTER_SIZE, buf);
+    return aarch64_pseudo_write_1 (gdbarch, regcache,
+   regnum - AARCH64_Q0_REGNUM, Q_REGISTER_SIZE,
+   buf);

   if (regnum >= AARCH64_D0_REGNUM && regnum < AARCH64_D0_REGNUM + 32)
-    return aarch64_pseudo_write_1 (regcache, regnum - AARCH64_D0_REGNUM,
-   D_REGISTER_SIZE, buf);
+    return aarch64_pseudo_write_1 (gdbarch, regcache,
+   regnum - AARCH64_D0_REGNUM, D_REGISTER_SIZE,
+   buf);

   if (regnum >= AARCH64_S0_REGNUM && regnum < AARCH64_S0_REGNUM + 32)
-    return aarch64_pseudo_write_1 (regcache, regnum - AARCH64_S0_REGNUM,
-   S_REGISTER_SIZE, buf);
+    return aarch64_pseudo_write_1 (gdbarch, regcache,
+   regnum - AARCH64_S0_REGNUM, S_REGISTER_SIZE,
+   buf);

   if (regnum >= AARCH64_H0_REGNUM && regnum < AARCH64_H0_REGNUM + 32)
-    return aarch64_pseudo_write_1 (regcache, regnum - AARCH64_H0_REGNUM,
-   H_REGISTER_SIZE, buf);
+    return aarch64_pseudo_write_1 (gdbarch, regcache,
+   regnum - AARCH64_H0_REGNUM, H_REGISTER_SIZE,
+   buf);

   if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
-    return aarch64_pseudo_write_1 (regcache, regnum - AARCH64_B0_REGNUM,
-   B_REGISTER_SIZE, buf);
+    return aarch64_pseudo_write_1 (gdbarch, regcache,
+   regnum - AARCH64_B0_REGNUM, B_REGISTER_SIZE,
+   buf);
+
+  if (tdep->has_sve () && regnum >= AARCH64_SVE_V0_REGNUM
+      && regnum < AARCH64_SVE_V0_REGNUM + 32)
+    return aarch64_pseudo_write_1 (gdbarch, regcache,
+   regnum - AARCH64_SVE_V0_REGNUM,
+   V_REGISTER_SIZE, buf);

   gdb_assert_not_reached ("regnum out of bound");
 }




123