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

[Andrew Burgess <aburgess@redhat.com>]

snatu@whileone.in writes:

> 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 picking this up.  It would be amazing to see vector support
land in GDB.

I'm a little rushed right now, I'll try to do a proper review soon, but
I do have some immediate questions related to register numbering, see
below...

> 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

I'm curious why these constants are needed in here at all?  It's been a
while since I worked on this corner of the code, but my understanding is
that the register numbering in the target description shouldn't have to
match any particular number at all, it's just a unique id to tell the
registers apart.

The riscv-tdep.c file will then map the unique id assigned here to GDB's
internal register numbering.

Now, there is a bit of a nit here; the existing xml files do include a
fixed numbering, and this was to work around some issues with early tool
versions that didn't send an XML target description, but instead assumed
a fixed numbering.  Thus, they relied on GDB always asking for register
number X when asking for e.g. fflags register.

My understanding is that QEMU has correctly been sending XML
descriptions for risc-v for a while now, so the fixed register numbering
should no longer be needed, but critically, I believe any version of
QEMU that has vector register support, has XML target description
support, so we really should be free to use any register numbering we
like here.

Like I said, it's been a while, so maybe I'm forgetting something.  If I
am, could you explain more why the fixed numbering is needed here.

> diff --git a/gdb/riscv-tdep.h b/gdb/riscv-tdep.h
> index ad1e9596b83..7b41dfbdcbc 100644
> --- a/gdb/riscv-tdep.h
> +++ b/gdb/riscv-tdep.h
> @@ -46,6 +46,15 @@ enum
>    RISCV_LAST_FP_REGNUM = 64,	/* Last Floating Point Register */
>  
>    RISCV_FIRST_CSR_REGNUM = 65,  /* First CSR */
> +
> +  RISCV_VSTART = 73,    /* Vector start position.  */
> +  RISCV_VXSAT = 74,     /* Fixed-Point Saturate Flag.  */
> +  RISCV_VXRM = 75,      /* Fixed-Point Rounding Mode.  */  
> +  RISCV_VCSR = 80,      /* Vector control and status register.  */
> +  RISCV_VL = 3169,      /* Vector length.  */
> +  RISCV_VTYPE = 3170,    /* Vector data type register.  */
> +  RISCV_VLENB = 3171,    /* VLEN/8 (vector register length in bytes) */
> +
>  #define DECLARE_CSR(name, num, class, define_version, abort_version) \
>    RISCV_ ## num ## _REGNUM = RISCV_FIRST_CSR_REGNUM + num,

Notice that RISCV_FIRST_CSR_REGNUM is 65, so vstart -> vcsr will overlap
with the RISCV_CSR_*_REGNUM constants.  Why not use the existing
constants?

Then vl/vtype/vlenb don't seem to match with the existing CSR constants,
unless I'm doing something wrong.  What's that all about?

What I suspect here is that this is evidence that what I say above
(about not neededing fixed numbering) is correct.  In the generated
target description we're using these incorrect(?) register numbers.  But
in riscv-tdep.c we map these to the correct CSR numbers.  Fetching
register state will be done using the actual CSR number, so it'll all
work out.

Anyway, any additional insights into the register numbering in this
patch would be great.

Thanks,
Andrew