Re: [PATCH] [PATCH v3] RISC-V: support for vector register accesses via ptrace() in RISC-V Linux native

[Charlie Jenkins <charlie@rivosinc.com>]

On Thu, Apr 24, 2025 at 12:19:14PM +0000, snatu@whileone.in wrote:
> From: Sameer Natu <snatu@whileone.in>
> 
> A v3 re-spin of the original patch.
> Tested with latest kernel 6.14.2 on RISCV QEMU.
> Removed Magic Numbers from v2 patch and worked on review comments of v2 patch.

Thanks for working on this!

Can you add a co-developed-by tag for the original author?

You also don't need to have [PATCH] twice in the header!

There are a handful of erroneous spaces at the end of lines.

I tested this patch and I noticed that the vector instructions are not
being decoded.

Breakpoint 1, vector () at main.S:4
4               vsetvli t0, a0, e32, m4, ta, ma
1: x/i $pc
=> 0x55555555566c <vector>:     .insn   4, 0x0d2572d

> 
> ---
>  gdb/arch/riscv.c             | 188 ++++++++++++++++++++++++++++++++++-
>  gdb/nat/riscv-linux-tdesc.c  |  68 +++++++++++++
>  gdb/nat/riscv-linux-tdesc.h  |  24 +++++
>  gdb/riscv-linux-nat.c        | 163 ++++++++++++++++++++++++++++++
>  gdb/riscv-linux-tdep.c       | 133 +++++++++++++++++++++++++
>  gdb/riscv-tdep.c             |  49 ++++++++-
>  gdb/riscv-tdep.h             |  14 +++
>  gdbserver/linux-riscv-low.cc | 110 ++++++++++++++++++++
>  include/elf/common.h         |   1 +
>  9 files changed, 743 insertions(+), 7 deletions(-)
> 
> diff --git a/gdb/arch/riscv.c b/gdb/arch/riscv.c
> index a6188ea3a8c..14fc85631e3 100644
> --- a/gdb/arch/riscv.c
> +++ b/gdb/arch/riscv.c
> @@ -25,12 +25,38 @@
>  #include "../features/riscv/64bit-fpu.c"
>  #include "../features/riscv/rv32e-xregs.c"
>  
> +#include "opcode/riscv-opc.h"
> +
>  #ifndef GDBSERVER
>  #define STATIC_IN_GDB static
>  #else
>  #define STATIC_IN_GDB
>  #endif
>  
> +#ifdef GDBSERVER
> +/* Work around issue where trying to include riscv-tdep.h (to get access to canonical RISCV_V0_REGNUM declaration
> +   from that header) is problamtic for gdbserver build.  */
> +//#include "riscv-tdep.h"
> +#define RISCV_VSTART 73
> +#define RISCV_VXSAT 74
> +#define RISCV_VXRM 75
> +#define RISCV_VCSR 80
> +#define RISCV_VL 3169 
> +#define RISCV_VTYPE 3170
> +#define RISCV_VLENB 3171
> +#define RISCV_V0_REGNUM 4162   
> +#else
> +#include "riscv-tdep.h"
> +#include "defs.h"
> +#endif
> +

As Andrew said, it seems valid to delete these hard-coded addresses.


> +static int
> +create_feature_riscv_vector_from_features (struct target_desc *result,
> +					   long regnum,
> +					   const struct riscv_gdbarch_features
> +					   features);
> +
> +
>  /* See arch/riscv.h.  */
>  
>  STATIC_IN_GDB target_desc_up
> @@ -83,15 +109,169 @@ riscv_create_target_description (const struct riscv_gdbarch_features features)
>    else if (features.flen == 8)
>      regnum = create_feature_riscv_64bit_fpu (tdesc.get (), regnum);
>  
> -  /* Currently GDB only supports vector features coming from remote
> -     targets.  We don't support creating vector features on native targets
> -     (yet).  */
>    if (features.vlen != 0)
> -    error (_("unable to create vector feature"));
> +    regnum =
> +      create_feature_riscv_vector_from_features (tdesc.get (),
> +						 RISCV_V0_REGNUM, features);
>  

After deleting the block, RISCV_V0_REGNUM can be replaced with regnum.

- Charlie