[v2,1/3] RISC-V/Linux/native: Determine FLEN dynamically

Message ID alpine.LFD.2.21.2001291410310.14118@redsun52.ssa.fujisawa.hgst.com
State New
Headers show
Series
  • RISC-V/Linux `gdbserver' support and associated fixes
Related show

Commit Message

Maciej W. Rozycki Jan. 29, 2020, 6:13 p.m.
Fix RISC-V native Linux support to handle a 64-bit FPU (FLEN == 64) with 
both RV32 and RV64 systems, which is a part of the current Linux ABI for 
hard-float systems, rather than assuming that (FLEN == XLEN) in target 
description determination and that (FLEN == 64) in register access.

We can do better however and not rely on any particular value of FLEN 
and probe for it dynamically, by observing that the PTRACE_GETREGSET 
ptrace(2) call will only accept an exact regset size, and that will 
reflect FLEN.  Therefore iterate over the call in target description 
determination with a geometrically increasing regset size until a match 
is marked by a successful ptrace(2) call completion or we run beyond the 
maximum size we can support.

Update register accessors accordingly, using FLEN determined to size the 
buffer used for NT_PRSTATUS requests and then to exchange data with the 
regcache.

Also handle a glibc bug where ELF_NFPREG is defined in terms of NFPREG, 
however NFPREG is nowhere defined.

	gdb/
	* riscv-linux-nat.c [!NFPREG] (NFPREG): New macro.
	(supply_fpregset_regnum, fill_fpregset): Handle regset buffer 
	offsets according to FLEN determined.
	(riscv_linux_nat_target::read_description): Determine FLEN 
	dynamically.
	(riscv_linux_nat_target::fetch_registers): Size regset buffer 
	according to FLEN determined.
	(riscv_linux_nat_target::store_registers): Likewise.
---
Hi,

 I'm not particularly happy with the lengthy lines in `fill_fpregset' and 
`supply_fpregset_regnum' causing multiple wrapping and deep indentation, 
but technically there is nothing wrong with it, so I'll leave it to a 
later clean-up.

  Maciej

Changes from v1:

- Also set the size of the regset buffer dynamically in 
  `riscv_linux_nat_target::fetch_registers' and 
  `riscv_linux_nat_target::store_registers', and update `fill_fpregset' 
  and `supply_fpregset_regnum' accordingly.
---
 gdb/riscv-linux-nat.c |   97 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 75 insertions(+), 22 deletions(-)

gdb-riscv-linux-nat-flen.diff

Comments

Jim Wilson Jan. 29, 2020, 11:24 p.m. | #1
On Wed, Jan 29, 2020 at 10:13 AM Maciej W. Rozycki <macro@wdc.com> wrote:
>         gdb/

>         * riscv-linux-nat.c [!NFPREG] (NFPREG): New macro.

>         (supply_fpregset_regnum, fill_fpregset): Handle regset buffer

>         offsets according to FLEN determined.

>         (riscv_linux_nat_target::read_description): Determine FLEN

>         dynamically.

>         (riscv_linux_nat_target::fetch_registers): Size regset buffer

>         according to FLEN determined.

>         (riscv_linux_nat_target::store_registers): Likewise.


Looks OK to me, though I'm not an official reviewer.

I did notice a reference to FGR in a comment that should presumably be
FPR, but that is a very minor issue.

Jim
Maciej W. Rozycki Jan. 29, 2020, 11:59 p.m. | #2
On Wed, 29 Jan 2020, Jim Wilson wrote:

> I did notice a reference to FGR in a comment that should presumably be

> FPR, but that is a very minor issue.


 FGR as in Floating-point General-purpose Register (as opposed to a 
floating-point control register).  Maybe it's too uncommon a use and need 
a full expansion here.

  Maciej
Jim Wilson Jan. 30, 2020, 10:56 p.m. | #3
On Wed, Jan 29, 2020 at 10:13 AM Maciej W. Rozycki <macro@wdc.com> wrote:
> We can do better however and not rely on any particular value of FLEN

> and probe for it dynamically, by observing that the PTRACE_GETREGSET

> ptrace(2) call will only accept an exact regset size, and that will

> reflect FLEN.


I forgot to mention this before, but our long term plans are to pass
info via the auxiliary vector from the kernel to the application, and
then in theory gdb should be able to get architecture info from there.
This may require support that hasn't been written yet, and we may
still need the fp probing for older systems that don't have all of the
support needed to make aux vec work.  Just with a quick check on my
4.15 kernel, I see
hifiveu017:1030$ ./a.out
AT_HWCAP = 0x1105
that is 'a', 'c', 'i', and 'm', each represented as 1<<(x-'a').  The
'f' and 'd' info is missing for some reason.  Maybe it hadn't been
implemented yet in this kernel version.

Jim
Andreas Schwab Jan. 30, 2020, 11:19 p.m. | #4
On Jan 30 2020, Jim Wilson wrote:

> Just with a quick check on my

> 4.15 kernel, I see

> hifiveu017:1030$ ./a.out

> AT_HWCAP = 0x1105

> that is 'a', 'c', 'i', and 'm', each represented as 1<<(x-'a').  The

> 'f' and 'd' info is missing for some reason. Maybe it hadn't been

> implemented yet in this kernel version.


Your kernel is just too archaic to include commit 732e8e4130ff.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."
Maciej W. Rozycki Jan. 31, 2020, 10:43 a.m. | #5
On Thu, 30 Jan 2020, Jim Wilson wrote:

> > We can do better however and not rely on any particular value of FLEN

> > and probe for it dynamically, by observing that the PTRACE_GETREGSET

> > ptrace(2) call will only accept an exact regset size, and that will

> > reflect FLEN.

> 

> I forgot to mention this before, but our long term plans are to pass

> info via the auxiliary vector from the kernel to the application, and

> then in theory gdb should be able to get architecture info from there.


 Thanks for the heads-up.  This will undoubtedly be useful for something, 
but I think we have a solution for GDB/gdbserver already, so unless the 
circumstances change, I think we're fine without the need to peek at the 
auxv.

 I'll be posting v3, which has just passed testing, right away.  It 
includes a few further improvements beyond what has been already 
discussed.

  Maciej

Patch

Index: binutils-gdb/gdb/riscv-linux-nat.c
===================================================================
--- binutils-gdb.orig/gdb/riscv-linux-nat.c
+++ binutils-gdb/gdb/riscv-linux-nat.c
@@ -28,6 +28,11 @@ 
 
 #include <sys/ptrace.h>
 
+/* Work around glibc header breakage causing ELF_NFPREG not to be usable.  */
+#ifndef NFPREG
+# define NFPREG 33
+#endif
+
 /* RISC-V Linux native additions to the default linux support.  */
 
 class riscv_linux_nat_target final : public linux_nat_target
@@ -88,21 +93,33 @@  static void
 supply_fpregset_regnum (struct regcache *regcache, const prfpregset_t *fpregs,
 			int regnum)
 {
+  int flen = register_size (regcache->arch (), RISCV_FIRST_FP_REGNUM);
+  union
+    {
+      const prfpregset_t *fpregs;
+      const gdb_byte *buf;
+    }
+  fpbuf = { .fpregs = fpregs };
   int i;
 
   if (regnum == -1)
     {
       /* We only support the FP registers and FCSR here.  */
       for (i = RISCV_FIRST_FP_REGNUM; i <= RISCV_LAST_FP_REGNUM; i++)
-	regcache->raw_supply (i, &fpregs->__d.__f[i - RISCV_FIRST_FP_REGNUM]);
+	regcache->raw_supply (i,
+			      fpbuf.buf + flen * (i - RISCV_FIRST_FP_REGNUM));
 
-      regcache->raw_supply (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr);
+      regcache->raw_supply (RISCV_CSR_FCSR_REGNUM,
+			    fpbuf.buf + flen * (RISCV_LAST_FP_REGNUM
+						- RISCV_FIRST_FP_REGNUM + 1));
     }
   else if (regnum >= RISCV_FIRST_FP_REGNUM && regnum <= RISCV_LAST_FP_REGNUM)
     regcache->raw_supply (regnum,
-			  &fpregs->__d.__f[regnum - RISCV_FIRST_FP_REGNUM]);
+			  fpbuf.buf + flen * (regnum - RISCV_FIRST_FP_REGNUM));
   else if (regnum == RISCV_CSR_FCSR_REGNUM)
-    regcache->raw_supply (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr);
+    regcache->raw_supply (RISCV_CSR_FCSR_REGNUM,
+			  fpbuf.buf + flen * (RISCV_LAST_FP_REGNUM
+					      - RISCV_FIRST_FP_REGNUM + 1));
 }
 
 /* Copy all floating point registers from regset FPREGS into REGCACHE.  */
@@ -145,19 +162,33 @@  void
 fill_fpregset (const struct regcache *regcache, prfpregset_t *fpregs,
 	       int regnum)
 {
+  int flen = register_size (regcache->arch (), RISCV_FIRST_FP_REGNUM);
+  union
+    {
+      prfpregset_t *fpregs;
+      gdb_byte *buf;
+    }
+  fpbuf = { .fpregs = fpregs };
+
   if (regnum == -1)
     {
       /* We only support the FP registers and FCSR here.  */
       for (int i = RISCV_FIRST_FP_REGNUM; i <= RISCV_LAST_FP_REGNUM; i++)
-	regcache->raw_collect (i, &fpregs->__d.__f[i - RISCV_FIRST_FP_REGNUM]);
+	regcache->raw_collect (i,
+			       fpbuf.buf + flen * (i - RISCV_FIRST_FP_REGNUM));
 
-      regcache->raw_collect (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr);
+      regcache->raw_collect (RISCV_CSR_FCSR_REGNUM,
+			     fpbuf.buf + flen * (RISCV_LAST_FP_REGNUM
+						 - RISCV_FIRST_FP_REGNUM + 1));
     }
   else if (regnum >= RISCV_FIRST_FP_REGNUM && regnum <= RISCV_LAST_FP_REGNUM)
     regcache->raw_collect (regnum,
-			   &fpregs->__d.__f[regnum - RISCV_FIRST_FP_REGNUM]);
+			   fpbuf.buf + flen * (regnum
+					       - RISCV_FIRST_FP_REGNUM));
   else if (regnum == RISCV_CSR_FCSR_REGNUM)
-    regcache->raw_collect (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr);
+    regcache->raw_collect (RISCV_CSR_FCSR_REGNUM,
+			   fpbuf.buf + flen * (RISCV_LAST_FP_REGNUM
+					       - RISCV_FIRST_FP_REGNUM + 1));
 }
 
 /* Return a target description for the current target.  */
@@ -166,8 +197,8 @@  const struct target_desc *
 riscv_linux_nat_target::read_description ()
 {
   struct riscv_gdbarch_features features;
-  struct iovec iov;
   elf_fpregset_t regs;
+  int flen;
   int tid;
 
   /* Figuring out xlen is easy.  */
@@ -175,19 +206,39 @@  riscv_linux_nat_target::read_description
 
   tid = inferior_ptid.lwp ();
 
-  iov.iov_base = &regs;
-  iov.iov_len = sizeof (regs);
+  /* Start with no f-registers.  */
+  features.flen = 0;
 
-  /* Can we fetch the f-registers?  */
-  if (ptrace (PTRACE_GETREGSET, tid, NT_FPREGSET,
-	      (PTRACE_TYPE_ARG3) &iov) == -1)
-    features.flen = 0;		/* No f-registers.  */
-  else
+  /* How much worth of f-registers can we fetch if any?  */
+  for (flen = sizeof (regs.__f.__f[0]); ; flen *= 2)
     {
-      /* TODO: We need a way to figure out the actual length of the
-	 f-registers.  We could have 64-bit x-registers, with 32-bit
-	 f-registers.  For now, just assumed xlen and flen match.  */
-      features.flen = features.xlen;
+      size_t regset_size;
+      struct iovec iov;
+
+      /* Regsets have a uniform slot size, so we count FSCR like an FGR.  */
+      regset_size = ELF_NFPREG * flen;
+      if (regset_size > sizeof (regs))
+	break;
+
+      iov.iov_base = &regs;
+      iov.iov_len = regset_size;
+      if (ptrace (PTRACE_GETREGSET, tid, NT_FPREGSET,
+		  (PTRACE_TYPE_ARG3) &iov) == -1)
+	{
+	  switch (errno)
+	    {
+	    case EINVAL:
+	      continue;
+	    case EIO:
+	      break;
+	    default:
+	      perror_with_name (_("Couldn't get registers"));
+	      break;
+	    }
+	}
+      else
+	features.flen = flen;
+      break;
     }
 
   return riscv_create_target_description (features);
@@ -228,7 +279,8 @@  riscv_linux_nat_target::fetch_registers
       elf_fpregset_t regs;
 
       iov.iov_base = &regs;
-      iov.iov_len = sizeof (regs);
+      iov.iov_len = ELF_NFPREG * register_size (regcache->arch (),
+						RISCV_FIRST_FP_REGNUM);
 
       if (ptrace (PTRACE_GETREGSET, tid, NT_FPREGSET,
 		  (PTRACE_TYPE_ARG3) &iov) == -1)
@@ -289,7 +341,8 @@  riscv_linux_nat_target::store_registers
       elf_fpregset_t regs;
 
       iov.iov_base = &regs;
-      iov.iov_len = sizeof (regs);
+      iov.iov_len = ELF_NFPREG * register_size (regcache->arch (),
+						RISCV_FIRST_FP_REGNUM);
 
       if (ptrace (PTRACE_GETREGSET, tid, NT_FPREGSET,
 		  (PTRACE_TYPE_ARG3) &iov) == -1)