[PATCH] gdb/riscv: Improve logic for when h/w float abi should be used

[PATCH] gdb/riscv: Improve logic for when h/w float abi should be used

[Andrew Burgess]

Currently, if the target announces that it has floating point
registers in its target description then GDB assumes that the hardware
float ABI should be used.  However, there's nothing stopping a user
compiling a program for the soft-float abi, and then trying to run
this on a target with hardware floating point registers.

This commit adjusts the logic that decides if GDB should use the
hardware float abi.  The primary decision now is based on what the ELF
currently being executed says in its headers.  If the file was
compiled for h/w float abi, then GDB uses h/w float abi, otherwise s/w
float is used.

If the current BFD is not an ELF then we don't currently have a
mechanism for figuring out if the file was compiled for float or not.
In this case we (currently) disable h/w float abi.  We might want to
improve this case in the future.

If there is NO current BFD (can this happen?) then we will enable h/w
float abi if the target has floating point hardware, otherwise, s/w
float abi is used.

This commit also adds some sanity checking that the features requested
in the BFD (xlen and flen) match the target description.

For testing I ran the testsuite on a target that returns a target
description containing both integer and floating point registers, but
used a compiler that didn't have floating point support.  Before this
commit I would see failures on may tests that made inferior calls
using floating point arguments, after this commit, all of these issues
are resolved.  One example from the testsuite is
gdb.base/infcall-nested-structs.exp.

gdb/ChangeLog:

	* riscv-tdep.c (riscv_features_from_gdbarch_info): New function.
	(riscv_find_default_target_description): Use new function to
	extract feature from gdbarch_info.
	(riscv_gdbarch_init): Add error checks for xlen and flen between
	target description and bfd headers.  Be smarter about when we
	think the hardware floating point abi should be used.
---
 gdb/ChangeLog    |  9 +++++++++
 gdb/riscv-tdep.c | 61 +++++++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 58 insertions(+), 12 deletions(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 30c777db6f2..d812b7c8693 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -2789,20 +2789,16 @@ static const struct frame_unwind riscv_frame_unwind =
   /*.prev_arch     =*/ NULL,
 };
 
-/* Find a suitable default target description.  Use the contents of INFO,
-   specifically the bfd object being executed, to guide the selection of a
-   suitable default target description.  */
+/* Extract a set of required target features out of INFO, specifically the
+   bfd being executed is examined to see what target features it requires.
+   IF there is no current bfd, or the bfd doesn't indicate any useful
+   features then a RISCV_GDBARCH_FEATURES is returned in its default state.  */
 
-static const struct target_desc *
-riscv_find_default_target_description (const struct gdbarch_info info)
+static struct riscv_gdbarch_features
+riscv_features_from_gdbarch_info (const struct gdbarch_info info)
 {
   struct riscv_gdbarch_features features;
 
-  /* Setup some arbitrary defaults.  */
-  features.xlen = 8;
-  features.flen = 0;
-  features.hw_float_abi = false;
-
   /* Now try to improve on the defaults by looking at the binary we are
      going to execute.  We assume the user knows what they are doing and
      that the target will match the binary.  Remember, this code path is
@@ -2847,6 +2843,26 @@ riscv_find_default_target_description (const struct gdbarch_info info)
 			binfo->bits_per_word);
     }
 
+  return features;
+}
+
+/* Find a suitable default target description.  Use the contents of INFO,
+   specifically the bfd object being executed, to guide the selection of a
+   suitable default target description.  */
+
+static const struct target_desc *
+riscv_find_default_target_description (const struct gdbarch_info info)
+{
+  /* Extract desired feature set from INFO.  */
+  struct riscv_gdbarch_features features
+    = riscv_features_from_gdbarch_info (info);
+
+  /* If the XLEN field is still 0 then we got nothing useful from INFO.  In
+     this case we fall back to a minimal useful target, 8-byte x-registers,
+     with no floating point.  */
+  if (features.xlen == 0)
+    features.xlen = 8;
+
   /* Now build a target description based on the feature set.  */
   return riscv_create_target_description (features);
 }
@@ -2981,7 +2997,6 @@ riscv_gdbarch_init (struct gdbarch_info info,
 
       int bitsize = tdesc_register_bitsize (feature_fpu, "ft0");
       features.flen = (bitsize / 8);
-      features.hw_float_abi = true;
 
       if (riscv_debug_gdbarch)
         fprintf_filtered
@@ -2991,7 +3006,6 @@ riscv_gdbarch_init (struct gdbarch_info info,
   else
     {
       features.flen = 0;
-      features.hw_float_abi = false;
 
       if (riscv_debug_gdbarch)
         fprintf_filtered
@@ -3015,6 +3029,29 @@ riscv_gdbarch_init (struct gdbarch_info info,
       return NULL;
     }
 
+  /* Have a look at what the supplied (if any) bfd object requires of the
+     target, then check that this matches with what the target is
+     providing.  */
+  struct riscv_gdbarch_features info_features
+    = riscv_features_from_gdbarch_info (info);
+  if (info_features.xlen != 0 && info_features.xlen != features.xlen)
+    error (_("bfd requires xlen %d, but target has xlen %d"),
+           info_features.xlen, features.xlen);
+  if (info_features.flen != 0 && info_features.flen != features.flen)
+    error (_("bfd requires flen %d, but target has flen %d"),
+           info_features.flen, features.flen);
+
+  /* If the xlen from INFO_FEATURES is 0 then this indicates either there
+     is no bfd object, or nothing useful could be extracted from it, in
+     this case we enable hardware float abi if the target has floating
+     point registers.
+
+     If the xlen from INFO_FEATURES is not 0, and the flen in
+     INFO_FEATURES is also not 0, then this indicates that the supplied
+     bfd does require hardware floating point abi.  */
+  if (info_features.xlen == 0 || info_features.flen != 0)
+    features.hw_float_abi = (features.flen > 0);
+
   /* Find a candidate among the list of pre-declared architectures.  */
   for (arches = gdbarch_list_lookup_by_info (arches, &info);
        arches != NULL;
-- 
2.14.5

Re: [PATCH] gdb/riscv: Improve logic for when h/w float abi should be used

[Jim Wilson]

On Tue, Dec 4, 2018 at 6:38 AM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
> If the current BFD is not an ELF then we don't currently have a
> mechanism for figuring out if the file was compiled for float or not.
> In this case we (currently) disable h/w float abi.  We might want to
> improve this case in the future.

The linker only supports ELF, and gives an error if the target is not
ELF.  You can use objcopy to convert to other formats, but that really
only works for raw formats which gdb probably can't use.

Jim

Re: [PATCH] gdb/riscv: Improve logic for when h/w float abi should be used

[Andrew Burgess]

* Jim Wilson <jimw@sifive.com> [2018-12-04 09:59:23 -0800]:

> On Tue, Dec 4, 2018 at 6:38 AM Andrew Burgess
> <andrew.burgess@embecosm.com> wrote:
> > If the current BFD is not an ELF then we don't currently have a
> > mechanism for figuring out if the file was compiled for float or not.
> > In this case we (currently) disable h/w float abi.  We might want to
> > improve this case in the future.
> 
> The linker only supports ELF, and gives an error if the target is not
> ELF.  You can use objcopy to convert to other formats, but that really
> only works for raw formats which gdb probably can't use.

Thanks for the feedback.  I pushed this patch with a slightly updated
commit message.

Andrew

---

gdb/riscv: Improve logic for when h/w float abi should be used

Currently, if the target announces that it has floating point
registers in its target description then GDB assumes that the hardware
float ABI should be used.  However, there's nothing stopping a user
compiling a program for the soft-float abi, and then trying to run
this on a target with hardware floating point registers.

This commit adjusts the logic that decides if GDB should use the
hardware float abi.  The primary decision now is based on what the ELF
currently being executed says in its headers.  If the file was
compiled for h/w float abi, then GDB uses h/w float abi, otherwise s/w
float is used.

If the current BFD is not an ELF then we don't currently have a
mechanism for figuring out if the file was compiled for float or not.
In this case we disable the h/w float abi.  This shouldn't be a
problem as, right now, the RISC-V linker can only produce ELFs.

If there is NO current BFD (can this happen?) then we will enable h/w
float abi if the target has floating point hardware, otherwise, s/w
float abi is used.

This commit also adds some sanity checking that the features requested
in the BFD (xlen and flen) match the target description.

For testing I ran the testsuite on a target that returns a target
description containing both integer and floating point registers, but
used a compiler that didn't have floating point support.  Before this
commit I would see failures on may tests that made inferior calls
using floating point arguments, after this commit, all of these issues
are resolved.  One example from the testsuite is
gdb.base/infcall-nested-structs.exp.

gdb/ChangeLog:

	* riscv-tdep.c (riscv_features_from_gdbarch_info): New function.
	(riscv_find_default_target_description): Use new function to
	extract feature from gdbarch_info.
	(riscv_gdbarch_init): Add error checks for xlen and flen between
	target description and bfd headers.  Be smarter about when we
	think the hardware floating point abi should be used.
---
 gdb/ChangeLog    |  9 +++++++++
 gdb/riscv-tdep.c | 61 +++++++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 58 insertions(+), 12 deletions(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 30c777db6f2..d812b7c8693 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -2789,20 +2789,16 @@ static const struct frame_unwind riscv_frame_unwind =
   /*.prev_arch     =*/ NULL,
 };
 
-/* Find a suitable default target description.  Use the contents of INFO,
-   specifically the bfd object being executed, to guide the selection of a
-   suitable default target description.  */
+/* Extract a set of required target features out of INFO, specifically the
+   bfd being executed is examined to see what target features it requires.
+   IF there is no current bfd, or the bfd doesn't indicate any useful
+   features then a RISCV_GDBARCH_FEATURES is returned in its default state.  */
 
-static const struct target_desc *
-riscv_find_default_target_description (const struct gdbarch_info info)
+static struct riscv_gdbarch_features
+riscv_features_from_gdbarch_info (const struct gdbarch_info info)
 {
   struct riscv_gdbarch_features features;
 
-  /* Setup some arbitrary defaults.  */
-  features.xlen = 8;
-  features.flen = 0;
-  features.hw_float_abi = false;
-
   /* Now try to improve on the defaults by looking at the binary we are
      going to execute.  We assume the user knows what they are doing and
      that the target will match the binary.  Remember, this code path is
@@ -2847,6 +2843,26 @@ riscv_find_default_target_description (const struct gdbarch_info info)
 			binfo->bits_per_word);
     }
 
+  return features;
+}
+
+/* Find a suitable default target description.  Use the contents of INFO,
+   specifically the bfd object being executed, to guide the selection of a
+   suitable default target description.  */
+
+static const struct target_desc *
+riscv_find_default_target_description (const struct gdbarch_info info)
+{
+  /* Extract desired feature set from INFO.  */
+  struct riscv_gdbarch_features features
+    = riscv_features_from_gdbarch_info (info);
+
+  /* If the XLEN field is still 0 then we got nothing useful from INFO.  In
+     this case we fall back to a minimal useful target, 8-byte x-registers,
+     with no floating point.  */
+  if (features.xlen == 0)
+    features.xlen = 8;
+
   /* Now build a target description based on the feature set.  */
   return riscv_create_target_description (features);
 }
@@ -2981,7 +2997,6 @@ riscv_gdbarch_init (struct gdbarch_info info,
 
       int bitsize = tdesc_register_bitsize (feature_fpu, "ft0");
       features.flen = (bitsize / 8);
-      features.hw_float_abi = true;
 
       if (riscv_debug_gdbarch)
         fprintf_filtered
@@ -2991,7 +3006,6 @@ riscv_gdbarch_init (struct gdbarch_info info,
   else
     {
       features.flen = 0;
-      features.hw_float_abi = false;
 
       if (riscv_debug_gdbarch)
         fprintf_filtered
@@ -3015,6 +3029,29 @@ riscv_gdbarch_init (struct gdbarch_info info,
       return NULL;
     }
 
+  /* Have a look at what the supplied (if any) bfd object requires of the
+     target, then check that this matches with what the target is
+     providing.  */
+  struct riscv_gdbarch_features info_features
+    = riscv_features_from_gdbarch_info (info);
+  if (info_features.xlen != 0 && info_features.xlen != features.xlen)
+    error (_("bfd requires xlen %d, but target has xlen %d"),
+           info_features.xlen, features.xlen);
+  if (info_features.flen != 0 && info_features.flen != features.flen)
+    error (_("bfd requires flen %d, but target has flen %d"),
+           info_features.flen, features.flen);
+
+  /* If the xlen from INFO_FEATURES is 0 then this indicates either there
+     is no bfd object, or nothing useful could be extracted from it, in
+     this case we enable hardware float abi if the target has floating
+     point registers.
+
+     If the xlen from INFO_FEATURES is not 0, and the flen in
+     INFO_FEATURES is also not 0, then this indicates that the supplied
+     bfd does require hardware floating point abi.  */
+  if (info_features.xlen == 0 || info_features.flen != 0)
+    features.hw_float_abi = (features.flen > 0);
+
   /* Find a candidate among the list of pre-declared architectures.  */
   for (arches = gdbarch_list_lookup_by_info (arches, &info);
        arches != NULL;
-- 
2.14.5