Re: [PATCH 3/8] gdb, gdbserver/aarch64-linux: Allow aarch64_sve_get_vq to return error

[Luis Machado <luis.machado@arm.com>]

On 9/8/22 07:41, Thiago Jung Bauermann via Gdb-patches wrote:
> If ptrace fails, aarch64_sve_get_vq assumes that SVE isn't supported.
> Because in a subsequent change this function will need to be called from
> low_new_thread (which can be called whe the inferior thread isn't stopped),
> it will need to distinguish between ptrace errors due to SVE not being
> supported from ptrace errors due to the inferior thread not being stopped.

My understanding is that we will always be able to validate supported features when we start/attach to a process. So if, say, we detect SVE
support through HWCAP's in the beginning of the debugging session, that will still hold for any thread that is spawned. What may change later
on is the VL (still, very unlikely).

When a thread is spawned, it will inherit SVE settings from its parent. From the Linux Kernel SVE documentation:

   In particular, on return from a fork() or clone(), the parent and new child
   process or thread share identical SVE configuration, matching that of the
   parent before the call.

Is that something we can use here? I suppose there may be corner cases where the parent spawned a thread and changed its VL size etc.

If we can't and a thread is running, we won't be able to tell the VL, but we have a state that is meant to indicate "unknown VL", don't we? Maybe I'm misremembering,
but -1 used to indicate that.

> 
> This patch changes the function to return -1 in the latter case and adjusts
> callers.  When a caller is allowed to propagate the error, it does so.  When
> that isn't possible an assertion is added to ensure the error isn't missed.
> ---
>   gdb/aarch64-linux-nat.c            | 14 ++++++++++++--
>   gdb/nat/aarch64-sve-linux-ptrace.c | 18 ++++++++++++------
>   gdb/nat/aarch64-sve-linux-ptrace.h |  2 +-
>   gdbserver/linux-aarch64-low.cc     |  6 +++++-
>   4 files changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index eda79ec6d35c..633cbc08796c 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -784,8 +784,14 @@ aarch64_linux_nat_target::read_description ()
>     CORE_ADDR hwcap = linux_get_hwcap (this);
>     CORE_ADDR hwcap2 = linux_get_hwcap2 (this);
>   
> +  int vq = aarch64_sve_get_vq (tid);
> +  if (vq < 0)
> +    /* A ptrace error happened so we can't determine the vq value.
> +       Give up trying to read a target description.  */
> +    return nullptr;
> +
>     aarch64_features features;
> -  features.vq = aarch64_sve_get_vq (tid);
> +  features.vq = vq;
>     features.pauth = hwcap & AARCH64_HWCAP_PACA;
>     features.mte = hwcap2 & HWCAP2_MTE;
>     features.tls = true;
> @@ -894,7 +900,11 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
>     /* Only return it if the current vector length matches the one in the tdep.  */
>     aarch64_gdbarch_tdep *tdep
>       = gdbarch_tdep<aarch64_gdbarch_tdep> (inf->gdbarch);
> -  uint64_t vq = aarch64_sve_get_vq (ptid.lwp ());
> +  int vq = aarch64_sve_get_vq (ptid.lwp ());
> +
> +  /* If ptrace fails we can't determine vq, but the thread_architecture method
> +     always succeeds so all we can do here is assert that vq is valid.  */
> +  gdb_assert (vq >= 0);
>     if (vq == tdep->vq)
>       return inf->gdbarch;
>   
> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.c b/gdb/nat/aarch64-sve-linux-ptrace.c
> index 019d2d65d89a..62f7fc5f56e1 100644
> --- a/gdb/nat/aarch64-sve-linux-ptrace.c
> +++ b/gdb/nat/aarch64-sve-linux-ptrace.c
> @@ -30,7 +30,7 @@
>   
>   /* See nat/aarch64-sve-linux-ptrace.h.  */
>   
> -uint64_t
> +int
>   aarch64_sve_get_vq (int tid)
>   {
>     struct iovec iovec;
> @@ -43,13 +43,19 @@ aarch64_sve_get_vq (int tid)
>        128bit chunks in a Z register.  We use VQ because 128bits is the minimum
>        a Z register can increase in size.  */
>   
> +  errno = 0;
>     if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_SVE, &iovec) < 0)
>       {
> +      if (errno == ESRCH)
> +	/* The process isn't stopped.  We can't determine SVE support or the
> +	   value of vq.  */
> +	return -1;
> +
>         /* SVE is not supported.  */
>         return 0;
>       }
>   
> -  uint64_t vq = sve_vq_from_vl (header.vl);
> +  int vq = sve_vq_from_vl (header.vl);
>   
>     if (!sve_vl_valid (header.vl))
>       {
> @@ -103,10 +109,10 @@ aarch64_sve_set_vq (int tid, struct reg_buffer_common *reg_buf)
>       {
>         /* If vg is not available yet, fetch it from ptrace.  The VG value from
>   	 ptrace is likely the correct one.  */
> -      uint64_t vq = aarch64_sve_get_vq (tid);
> +      int vq = aarch64_sve_get_vq (tid);
>   
>         /* If something went wrong, just bail out.  */
> -      if (vq == 0)
> +      if (vq <= 0)
>   	return false;
>   
>         reg_vg = sve_vg_from_vq (vq);
> @@ -123,9 +129,9 @@ std::unique_ptr<gdb_byte[]>
>   aarch64_sve_get_sveregs (int tid)
>   {
>     struct iovec iovec;
> -  uint64_t vq = aarch64_sve_get_vq (tid);
> +  int vq = aarch64_sve_get_vq (tid);
>   
> -  if (vq == 0)
> +  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
> diff --git a/gdb/nat/aarch64-sve-linux-ptrace.h b/gdb/nat/aarch64-sve-linux-ptrace.h
> index 5c264b395313..90920bc48ef3 100644
> --- a/gdb/nat/aarch64-sve-linux-ptrace.h
> +++ b/gdb/nat/aarch64-sve-linux-ptrace.h
> @@ -43,7 +43,7 @@
>   /* 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 zero).  */
>   
> -uint64_t aarch64_sve_get_vq (int tid);
> +int aarch64_sve_get_vq (int tid);
>   
>   /* Set VQ in the kernel for the given tid, using either the value VQ or
>      reading from the register VG in the register buffer.  */
> diff --git a/gdbserver/linux-aarch64-low.cc b/gdbserver/linux-aarch64-low.cc
> index 5d4d667dfa42..576925838f49 100644
> --- a/gdbserver/linux-aarch64-low.cc
> +++ b/gdbserver/linux-aarch64-low.cc
> @@ -823,8 +823,12 @@ aarch64_target::low_arch_setup ()
>     if (is_elf64)
>       {
>         struct aarch64_features features;
> +      int vq = aarch64_sve_get_vq (tid);
>   
> -      features.vq = aarch64_sve_get_vq (tid);
> +      /* If ptrace fails we can't determine vq, but the low_arch_setup method
> +	  always succeeds so all we can do here is assert that vq is valid.  */
> +      gdb_assert (vq >= 0);
> +      features.vq = vq;
>         /* A-profile PAC is 64-bit only.  */
>         features.pauth = linux_get_hwcap (current_thread, 8) & AARCH64_HWCAP_PACA;
>         /* A-profile MTE is 64-bit only.  */