[PATCH 0/2] RISC-V: Improve FP register support.

[PATCH 0/2] RISC-V: Improve FP register support.

[Jim Wilson]

The first patch improves printing of values in FP registers.  A 64-bit FP
register can hold either float or double, and they are currently printed as
only double.  The patch changes this to print them as both float and
double.  Without the patch I see
(gdb) print $fa0
$1 = -nan(0xfffff3f828f5c)
With the patch I see
(gdb) print $fa0
$1 = {float = 1.01999998, double = -nan(0xfffff3f828f5c)}

The second patch fixes problems with inferior calls, when passing single
float values in 64-bit FP registers.  The hardware requires that they
be NaN-boxed, which means the upper 32-bits must be ones, so that they
look like a 64-bit NaN.  See the above for instance.  These values are
currently zero extended.

Tested with a gdb testsuite run on a HiFive Unleashed running fedora
core 29.  The first patch has no effect on the results.  The second
patch fixes about 74 testcases in gdb.base, for call*, infcall*, and
struct* testcases.

Jim

[PATCH 1/2] RISC-V: Print FP regs as union of float types.

[Jim Wilson]

A 64-bit FP register can hold either a single or double float value, so
print it as both types by using a union type for FP registers.  Likewise
for 128-bit regs which can also hold long double.

	gdb/
	* riscv-tdep.c (riscv_fpreg_d_type, riscv_fpreg_q_type): New.
	(riscv_register_type): Use them.
	(riscv_print_one_register_info): Handle union of floats same as float.
	* riscv-tdep.h (struct gdbarch_tdep): Add riscv_fpreg_d_type and
	riscv_fpreg_q_type fields.
---
 gdb/riscv-tdep.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++--
 gdb/riscv-tdep.h |  4 +++
 2 files changed, 86 insertions(+), 3 deletions(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 3402241b97..a4d732f6f9 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -495,6 +495,76 @@ riscv_register_name (struct gdbarch *gdbarch, int regnum)
   return NULL;
 }
 
+/* Construct a type for 64-bit FP registers.  */
+
+static struct type *
+riscv_fpreg_d_type (struct gdbarch *gdbarch)
+{
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
+  if (!tdep->riscv_fpreg_d_type)
+    {
+      const struct builtin_type *bt = builtin_type (gdbarch);
+
+      /* The type we're building is this: */
+#if 0
+      union __gdb_builtin_type_fpreg_d
+      {
+	float f;
+	double d;
+      };
+#endif
+
+      struct type *t;
+
+      t = arch_composite_type (gdbarch,
+			       "__gdb_builtin_type_fpreg_d", TYPE_CODE_UNION);
+      append_composite_type_field (t, "float", bt->builtin_float);
+      append_composite_type_field (t, "double", bt->builtin_double);
+      TYPE_VECTOR (t) = 1;
+      TYPE_NAME (t) = "builtin_type_fpreg_d";
+      tdep->riscv_fpreg_d_type = t;
+    }
+
+  return tdep->riscv_fpreg_d_type;
+}
+
+/* Construct a type for 128-bit FP registers.  */
+
+static struct type *
+riscv_fpreg_q_type (struct gdbarch *gdbarch)
+{
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+
+  if (!tdep->riscv_fpreg_q_type)
+    {
+      const struct builtin_type *bt = builtin_type (gdbarch);
+
+      /* The type we're building is this: */
+#if 0
+      union __gdb_builtin_type_fpreg_d
+      {
+	float f;
+	double d;
+	long double ld;
+      };
+#endif
+
+      struct type *t;
+
+      t = arch_composite_type (gdbarch,
+			       "__gdb_builtin_type_fpreg_q", TYPE_CODE_UNION);
+      append_composite_type_field (t, "float", bt->builtin_float);
+      append_composite_type_field (t, "double", bt->builtin_double);
+      append_composite_type_field (t, "long double", bt->builtin_long_double);
+      TYPE_VECTOR (t) = 1;
+      TYPE_NAME (t) = "builtin_type_fpreg_q";
+      tdep->riscv_fpreg_q_type = t;
+    }
+
+  return tdep->riscv_fpreg_q_type;
+}
+
 /* Implement the register_type gdbarch method.  */
 
 static struct type *
@@ -537,9 +607,9 @@ riscv_register_type (struct gdbarch *gdbarch, int regnum)
 	case 4:
 	  return builtin_type (gdbarch)->builtin_float;
 	case 8:
-	  return builtin_type (gdbarch)->builtin_double;
+	  return riscv_fpreg_d_type (gdbarch);
 	case 16:
-	  return builtin_type (gdbarch)->builtin_long_double;
+	  return riscv_fpreg_q_type (gdbarch);
 	default:
 	  internal_error (__FILE__, __LINE__,
 			  _("unknown isa regsize %i"), regsize);
@@ -591,7 +661,16 @@ riscv_print_one_register_info (struct gdbarch *gdbarch,
   print_raw_format = (value_entirely_available (val)
 		      && !value_optimized_out (val));
 
-  if (TYPE_CODE (regtype) == TYPE_CODE_FLT)
+  if (TYPE_CODE (regtype) == TYPE_CODE_FLT
+      || (TYPE_CODE (regtype) == TYPE_CODE_UNION
+	  && TYPE_NFIELDS (regtype) == 2
+	  && TYPE_CODE (TYPE_FIELD_TYPE (regtype, 0)) == TYPE_CODE_FLT
+	  && TYPE_CODE (TYPE_FIELD_TYPE (regtype, 1)) == TYPE_CODE_FLT)
+      || (TYPE_CODE (regtype) == TYPE_CODE_UNION
+	  && TYPE_NFIELDS (regtype) == 3
+	  && TYPE_CODE (TYPE_FIELD_TYPE (regtype, 0)) == TYPE_CODE_FLT
+	  && TYPE_CODE (TYPE_FIELD_TYPE (regtype, 1)) == TYPE_CODE_FLT
+	  && TYPE_CODE (TYPE_FIELD_TYPE (regtype, 2)) == TYPE_CODE_FLT))
     {
       struct value_print_options opts;
       const gdb_byte *valaddr = value_contents_for_printing (val);
diff --git a/gdb/riscv-tdep.h b/gdb/riscv-tdep.h
index 8a2454eb66..e04e728f32 100644
--- a/gdb/riscv-tdep.h
+++ b/gdb/riscv-tdep.h
@@ -78,6 +78,10 @@ struct gdbarch_tdep
      features that are supported on the target.  These could be cached from
      the target, or read from the executable when available.  */
   unsigned core_features;
+
+  /* ISA-specific data types.  */
+  struct type *riscv_fpreg_d_type;
+  struct type *riscv_fpreg_q_type;
 };
 
 /* Return the width in bytes of the general purpose registers for GDBARCH.  */
-- 
2.17.1

Re: [PATCH 1/2] RISC-V: Print FP regs as union of float types.

[Kevin Buettner]

On Fri, 19 Oct 2018 14:49:07 -0700
Jim Wilson <jimw@sifive.com> wrote:

> A 64-bit FP register can hold either a single or double float value, so
> print it as both types by using a union type for FP registers.  Likewise
> for 128-bit regs which can also hold long double.
> 
> 	gdb/
> 	* riscv-tdep.c (riscv_fpreg_d_type, riscv_fpreg_q_type): New.
> 	(riscv_register_type): Use them.
> 	(riscv_print_one_register_info): Handle union of floats same as float.
> 	* riscv-tdep.h (struct gdbarch_tdep): Add riscv_fpreg_d_type and
> 	riscv_fpreg_q_type fields.

This is okay, except for the following nits...

> +static struct type *
> +riscv_fpreg_d_type (struct gdbarch *gdbarch)
> +{
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +
> +  if (!tdep->riscv_fpreg_d_type)

The GDB coding standard say that this must be coded as:

     if (tdep->riscv_fpreg_d_type == NULL)

Though I think we prefer nullptr over NULL these days.

(I realize that there's code in GDB which does not conform to this
standard.)

Likewise, here:

> +static struct type *
> +riscv_fpreg_q_type (struct gdbarch *gdbarch)
> +{
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +
> +  if (!tdep->riscv_fpreg_q_type)

Kevin

Re: [PATCH 1/2] RISC-V: Print FP regs as union of float types.

[Jim Wilson]

On Sat, Oct 20, 2018 at 2:38 PM Kevin Buettner <kevinb@redhat.com> wrote:
>
> On Fri, 19 Oct 2018 14:49:07 -0700
> Jim Wilson <jimw@sifive.com> wrote:
>
> > A 64-bit FP register can hold either a single or double float value, so
> > print it as both types by using a union type for FP registers.  Likewise
> > for 128-bit regs which can also hold long double.
> >
> >       gdb/
> >       * riscv-tdep.c (riscv_fpreg_d_type, riscv_fpreg_q_type): New.
> >       (riscv_register_type): Use them.
> >       (riscv_print_one_register_info): Handle union of floats same as float.
> >       * riscv-tdep.h (struct gdbarch_tdep): Add riscv_fpreg_d_type and
> >       riscv_fpreg_q_type fields.
>
> The GDB coding standard say that this must be coded as:
>
>      if (tdep->riscv_fpreg_d_type == NULL)
>
> Though I think we prefer nullptr over NULL these days.

I updated the patch to use nullptr, retested it, and committed it.

Jim

[PATCH 2/2] RISC-V: NaN-box FP values smaller than an FP register.

[Jim Wilson]

The hardware requires that values in FP registers be NaN-boxed, so we must
extend them with 1's instead of 0's as we do for integer values.

	gdb/
	* riscv-tdep.c (riscv_push_dummy_call) <in_reg>: Check for value in
	FP reg smaller than FP reg size, and fill with -1 instead of 0.
---
 gdb/riscv-tdep.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index a4d732f6f9..5be889d300 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -2408,7 +2408,12 @@ riscv_push_dummy_call (struct gdbarch *gdbarch,
 	    gdb_byte tmp [sizeof (ULONGEST)];
 
 	    gdb_assert (info->argloc[0].c_length <= info->length);
-	    memset (tmp, 0, sizeof (tmp));
+	    /* FP values in FP registers must be NaN-boxed.  */
+	    if (riscv_is_fp_regno_p (info->argloc[0].loc_data.regno)
+		&& info->argloc[0].c_length < call_info.flen)
+	      memset (tmp, -1, sizeof (tmp));
+	    else
+	      memset (tmp, 0, sizeof (tmp));
 	    memcpy (tmp, info->contents, info->argloc[0].c_length);
 	    regcache->cooked_write (info->argloc[0].loc_data.regno, tmp);
 	    second_arg_length =
@@ -2447,7 +2452,12 @@ riscv_push_dummy_call (struct gdbarch *gdbarch,
 		gdb_assert ((riscv_is_fp_regno_p (info->argloc[1].loc_data.regno)
 			     && second_arg_length <= call_info.flen)
 			    || second_arg_length <= call_info.xlen);
-		memset (tmp, 0, sizeof (tmp));
+		/* FP values in FP registers must be NaN-boxed.  */
+		if (riscv_is_fp_regno_p (info->argloc[1].loc_data.regno)
+		    && second_arg_length < call_info.flen)
+		  memset (tmp, -1, sizeof (tmp));
+		else
+		  memset (tmp, 0, sizeof (tmp));
 		memcpy (tmp, second_arg_data, second_arg_length);
 		regcache->cooked_write (info->argloc[1].loc_data.regno, tmp);
 	      }
-- 
2.17.1

Re: [PATCH 2/2] RISC-V: NaN-box FP values smaller than an FP register.

[Kevin Buettner]

On Fri, 19 Oct 2018 14:49:53 -0700
Jim Wilson <jimw@sifive.com> wrote:

> The hardware requires that values in FP registers be NaN-boxed, so we must
> extend them with 1's instead of 0's as we do for integer values.
> 
> 	gdb/
> 	* riscv-tdep.c (riscv_push_dummy_call) <in_reg>: Check for value in
> 	FP reg smaller than FP reg size, and fill with -1 instead of 0.

Okay.

Kevin

Re: [PATCH 2/2] RISC-V: NaN-box FP values smaller than an FP register.

[Jim Wilson]

On Sat, Oct 20, 2018 at 2:39 PM Kevin Buettner <kevinb@redhat.com> wrote:
> On Fri, 19 Oct 2018 14:49:53 -0700
> Jim Wilson <jimw@sifive.com> wrote:
>
> > The hardware requires that values in FP registers be NaN-boxed, so we must
> > extend them with 1's instead of 0's as we do for integer values.
> >
> >       gdb/
> >       * riscv-tdep.c (riscv_push_dummy_call) <in_reg>: Check for value in
> >       FP reg smaller than FP reg size, and fill with -1 instead of 0.
>
> Okay.

Committed.

Jim

Re: [PATCH 0/2] RISC-V: Improve FP register support.

[Pedro Alves]

On 10/19/2018 10:46 PM, Jim Wilson wrote:
> The first patch improves printing of values in FP registers.  A 64-bit FP
> register can hold either float or double, and they are currently printed as
> only double.  The patch changes this to print them as both float and
> double.  Without the patch I see
> (gdb) print $fa0
> $1 = -nan(0xfffff3f828f5c)
> With the patch I see
> (gdb) print $fa0
> $1 = {float = 1.01999998, double = -nan(0xfffff3f828f5c)}

> Tested with a gdb testsuite run on a HiFive Unleashed running fedora
> core 29.  The first patch has no effect on the results.  

That screams "add some test then".  :-)

Thanks,
Pedro Alves

Re: [PATCH 0/2] RISC-V: Improve FP register support.

[Jim Wilson]

On Tue, Oct 23, 2018 at 4:18 AM Pedro Alves <palves@redhat.com> wrote:
> > Tested with a gdb testsuite run on a HiFive Unleashed running fedora
> > core 29.  The first patch has no effect on the results.
>
> That screams "add some test then".  :-)

Turns out that we do have a test now, as meanwhile Andrew added
gdb.arch/riscv-reg-aliases.exp which tests reading/writing registers
via their aliases, including FP registers, and I'm getting a lot of
failures from this new testcase because my patch conflicts with it.
It doesn't specifically test 32-bit float FP values though, but it
looks easy enough to add that in, or we could maybe use the framework
for a second related testcase.

Jim