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

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

Commit Message

Maciej W. Rozycki Feb. 1, 2020, 8:20 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.
---
No changes from v4.

No changes from v3.

Changes from v2:

- Avoid an ambiguous FGR reference in 
  `riscv_linux_nat_target::read_description'.

- Advance through the buffer accessed in `supply_fpregset_regnum' and 
  `fill_fpregset' so as to avoid doing a variable multiplication in a 
  loop; also precalculate the buffer offset for single register accesses 
  to avoid excessive line wrapping and improve code readability.

- Move the `i' variable declaration to the beginning of `fill_fpregset', 
  for consistency with `supply_fpregset_regnum' (since the loop is being 
  rewritten anyway).

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 |  110 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 84 insertions(+), 26 deletions(-)

gdb-riscv-linux-nat-flen.diff

Comments

Andrew Burgess Feb. 2, 2020, 9:11 p.m. | #1
* Maciej W. Rozycki <macro@wdc.com> [2020-02-01 20:20:46 +0000]:

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


Thanks for working on this.

I'm happy for this patch to go in, with a couple of minor adjustments
that I've noted inline below.

Before you merge I just wanted to double check; given this patch is
from a wdc.com email, do you have a copyright assignment in place from
wdc?

Thanks,
Andrew

> ---

> No changes from v4.

> 

> No changes from v3.

> 

> Changes from v2:

> 

> - Avoid an ambiguous FGR reference in 

>   `riscv_linux_nat_target::read_description'.

> 

> - Advance through the buffer accessed in `supply_fpregset_regnum' and 

>   `fill_fpregset' so as to avoid doing a variable multiplication in a 

>   loop; also precalculate the buffer offset for single register accesses 

>   to avoid excessive line wrapping and improve code readability.

> 

> - Move the `i' variable declaration to the beginning of `fill_fpregset', 

>   for consistency with `supply_fpregset_regnum' (since the loop is being 

>   rewritten anyway).

> 

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

>  1 file changed, 84 insertions(+), 26 deletions(-)

> 

> gdb-riscv-linux-nat-flen.diff

> 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,35 @@ 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]);

> +      for (i = RISCV_FIRST_FP_REGNUM;

> +	   i <= RISCV_LAST_FP_REGNUM;

> +	   i++, fpbuf.buf += flen)

> +	regcache->raw_supply (i, fpbuf.buf);

>  

> -      regcache->raw_supply (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr);

> +      regcache->raw_supply (RISCV_CSR_FCSR_REGNUM, fpbuf.buf);

>      }

>    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);

> +      regcache->raw_supply (regnum, fpbuf.buf);

> +    }

>    else if (regnum == RISCV_CSR_FCSR_REGNUM)

> -    regcache->raw_supply (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr);

> +    {

> +      fpbuf.buf += flen * (RISCV_LAST_FP_REGNUM - RISCV_FIRST_FP_REGNUM + 1);

> +      regcache->raw_supply (RISCV_CSR_FCSR_REGNUM, fpbuf.buf);

> +    }

>  }

>  

>  /* Copy all floating point registers from regset FPREGS into REGCACHE.  */

> @@ -145,19 +164,35 @@ 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 };

> +  int i;

> +

>    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]);

> +      for (i = RISCV_FIRST_FP_REGNUM;

> +	   i <= RISCV_LAST_FP_REGNUM;

> +	   i++, fpbuf.buf += flen)

> +	regcache->raw_collect (i, fpbuf.buf);

>  

> -      regcache->raw_collect (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr);

> +      regcache->raw_collect (RISCV_CSR_FCSR_REGNUM, fpbuf.buf);

>      }

>    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);

> +      regcache->raw_collect (regnum, fpbuf.buf);

> +    }

>    else if (regnum == RISCV_CSR_FCSR_REGNUM)

> -    regcache->raw_collect (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr);

> +    {

> +      fpbuf.buf += flen * (RISCV_LAST_FP_REGNUM - RISCV_FIRST_FP_REGNUM + 1);

> +      regcache->raw_collect (RISCV_CSR_FCSR_REGNUM, fpbuf.buf);

> +    }

>  }

>  

>  /* Return a target description for the current target.  */

> @@ -166,8 +201,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 +210,40 @@ 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 FP data register.  */

> +      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 +284,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);


I think that this would be made clearer, and more robust if we added
this assertion:

  gdb_assert (iov.iov_len <= sizeof (regs));

>  

>        if (ptrace (PTRACE_GETREGSET, tid, NT_FPREGSET,

>  		  (PTRACE_TYPE_ARG3) &iov) == -1)

> @@ -289,7 +346,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);


And I think we should add the assertion here too.

With these assertions, and assuming you have a suitable copyright
assignment, then feel free to merge this.

Thanks,
Andrew


>  

>        if (ptrace (PTRACE_GETREGSET, tid, NT_FPREGSET,

>  		  (PTRACE_TYPE_ARG3) &iov) == -1)

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,35 @@  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]);
+      for (i = RISCV_FIRST_FP_REGNUM;
+	   i <= RISCV_LAST_FP_REGNUM;
+	   i++, fpbuf.buf += flen)
+	regcache->raw_supply (i, fpbuf.buf);
 
-      regcache->raw_supply (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr);
+      regcache->raw_supply (RISCV_CSR_FCSR_REGNUM, fpbuf.buf);
     }
   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);
+      regcache->raw_supply (regnum, fpbuf.buf);
+    }
   else if (regnum == RISCV_CSR_FCSR_REGNUM)
-    regcache->raw_supply (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr);
+    {
+      fpbuf.buf += flen * (RISCV_LAST_FP_REGNUM - RISCV_FIRST_FP_REGNUM + 1);
+      regcache->raw_supply (RISCV_CSR_FCSR_REGNUM, fpbuf.buf);
+    }
 }
 
 /* Copy all floating point registers from regset FPREGS into REGCACHE.  */
@@ -145,19 +164,35 @@  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 };
+  int i;
+
   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]);
+      for (i = RISCV_FIRST_FP_REGNUM;
+	   i <= RISCV_LAST_FP_REGNUM;
+	   i++, fpbuf.buf += flen)
+	regcache->raw_collect (i, fpbuf.buf);
 
-      regcache->raw_collect (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr);
+      regcache->raw_collect (RISCV_CSR_FCSR_REGNUM, fpbuf.buf);
     }
   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);
+      regcache->raw_collect (regnum, fpbuf.buf);
+    }
   else if (regnum == RISCV_CSR_FCSR_REGNUM)
-    regcache->raw_collect (RISCV_CSR_FCSR_REGNUM, &fpregs->__d.__fcsr);
+    {
+      fpbuf.buf += flen * (RISCV_LAST_FP_REGNUM - RISCV_FIRST_FP_REGNUM + 1);
+      regcache->raw_collect (RISCV_CSR_FCSR_REGNUM, fpbuf.buf);
+    }
 }
 
 /* Return a target description for the current target.  */
@@ -166,8 +201,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 +210,40 @@  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 FP data register.  */
+      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 +284,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 +346,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)