[PATCHv2] gdb/riscv: Split ISA and ABI features

  Changes since v1:

  - Fixed typo (stray 'x' character).
  
  - Updated comments to no longer say RV32 on RV64 can't be done in
    the future.

  - Made riscv_abi_xlen global like all the other
    riscv_{abi,isa}_{xlen,flen} functions.

Thanks,
Andrew
  

* Andrew Burgess <andrew.burgess@embecosm.com> [2018-12-19 16:42:21 +0000]:

> Changes since v1:
> 
>   - Fixed typo (stray 'x' character).
>   
>   - Updated comments to no longer say RV32 on RV64 can't be done in
>     the future.
> 
>   - Made riscv_abi_xlen global like all the other
>     riscv_{abi,isa}_{xlen,flen} functions.

I've now pushed this.

Thanks,
Andrew


> 
> Thanks,
> Andrew
> 
> -- 
> 
> The goal of this commit is to allow RV64 binaries compiled for the 'F'
> extension to run on a target that supports both the 'F' and 'D'
> extensions.
> 
> The 'D' extension depends on the 'F' extension and chapter 9 of the
> RISC-V ISA manual implies that running a program compiled for 'F' on
> a 'D' target should be fine.
> 
> To support this the gdbarch now holds two feature sets, one represents
> the features that are present on the target, and one represents the
> features requested in the ELF flags.
> 
> The existing error checks are relaxed slightly to allow binaries
> compiled for 32-bit 'F' extension to run on targets with the 64-bit
> 'D' extension.
> 
> A new set of functions called riscv_abi_{xlen,flen} are added to
> compliment the existing riscv_isa_{xlen,flen}, and some callers to the
> isa functions now call the abi functions when that is appropriate.
> 
> In riscv_call_arg_struct two asserts are removed, these asserts no
> longer make sense.  The asserts were both like this:
> 
>     gdb_assert (TYPE_LENGTH (ainfo->type)
>                 <= (cinfo->flen + cinfo->xlen));
> 
> And were made in two cases, when passing structures like these:
> 
>    struct {
>      integer field1;
>      float   field2;
>    };
> 
> or,
> 
>    struct {
>      float   field1;
>      integer field2;
>    };
> 
> When running on an RV64 target which only has 32-bit float then the
> integer field could be 64-bits, while if the float field is 32-bits
> the overall size of the structure can be 128-bits (with 32-bits of
> padding).  In this case the assertion would fail, however, the code
> isn't incorrect, so its safe to just remove the assertion.
> 
> This was tested by running on an RV64IMFDC target using a compiler
> configured for RV64IMFC, and comparing the results with those obtained
> when using a compiler configured for RV64IMFDC.  The only regressions
> I see (now) are in gdb.base/store.exp and are related too different
> code generation choices GCC makes between the two targets.
> 
> Finally, this commit does not make any attempt to support running
> binaries compiled for RV32 on an RV64 target, though nothing in here
> should prevent that being supported in the future.
> 
> gdb/ChangeLog:
> 
> 	* arch/riscv.h (struct riscv_gdbarch_features) <hw_float_abi>:
> 	Delete.
> 	<operator==>: Update with for removed field.
> 	<hash>: Likewise.
> 	* riscv-tdep.h (struct gdbarch_tdep) <features>: Renamed to...
> 	<isa_features>: ...this.
> 	<abi_features>: New field.
> 	(riscv_isa_flen): Update comment.
> 	(riscv_abi_xlen): New declaration.
> 	(riscv_abi_flen): New declaration.
> 	* riscv-tdep.c (riscv_isa_xlen): Update to get answer from
> 	isa_features.
> 	(riscv_abi_xlen): New function.
> 	(riscv_isa_flen): Update to get answer from isa_features.
> 	(riscv_abi_flen): New function.
> 	(riscv_has_fp_abi): Update to get answer from abi_features.
> 	(riscv_call_info::riscv_call_info): Use abi xlen and flen, not isa
> 	xlen and flen.
> 	(riscv_call_info) <xlen, flen>: Update comment.
> 	(riscv_call_arg_struct): Remove invalid assertions
> 	(riscv_features_from_gdbarch_info): Update now hw_float_abi field
> 	is removed.
> 	(riscv_gdbarch_init): Gather isa features and abi features
> 	separately, ensure both match on the gdbarch when reusing an old
> 	gdbarch.  Relax an error check to allow 32-bit abi float to run on
> 	a target with 64-bit float hardware.
> ---
>  gdb/ChangeLog    | 29 ++++++++++++++++++++
>  gdb/arch/riscv.h | 15 ++---------
>  gdb/riscv-tdep.c | 81 ++++++++++++++++++++++++++++++--------------------------
>  gdb/riscv-tdep.h | 37 +++++++++++++++++++++-----
>  4 files changed, 105 insertions(+), 57 deletions(-)
> 
> diff --git a/gdb/arch/riscv.h b/gdb/arch/riscv.h
> index 79f5ec622d4..68297a8b73e 100644
> --- a/gdb/arch/riscv.h
> +++ b/gdb/arch/riscv.h
> @@ -46,19 +46,10 @@ struct riscv_gdbarch_features
>       that there are no f-registers.  No other value is valid.  */
>    int flen = 0;
>  
> -  /* This indicates if hardware floating point abi is in use.  If the FLEN
> -     field is 0 then this value _must_ be false.  If the FLEN field is
> -     non-zero and this field is false then this indicates the target has
> -     floating point registers, but is still using the soft-float abi.  If
> -     this field is true then the hardware floating point abi is in use, and
> -     values are passed in f-registers matching the size of FLEN.  */
> -  bool hw_float_abi = false;
> -
>    /* Equality operator.  */
>    bool operator== (const struct riscv_gdbarch_features &rhs) const
>    {
> -    return (xlen == rhs.xlen && flen == rhs.flen
> -	    && hw_float_abi == rhs.hw_float_abi);
> +    return (xlen == rhs.xlen && flen == rhs.flen);
>    }
>  
>    /* Inequality operator.  */
> @@ -70,9 +61,7 @@ struct riscv_gdbarch_features
>    /* Used by std::unordered_map to hash feature sets.  */
>    std::size_t hash () const noexcept
>    {
> -    std::size_t val = ((xlen & 0x1f) << 6
> -                       | (flen & 0x1f) << 1
> -                       | (hw_float_abi ? 1 : 0));
> +    std::size_t val = ((xlen & 0x1f) << 5 | (flen & 0x1f) << 0);
>      return val;
>    }
>  };
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 88b79af866f..967dd4ddaab 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -361,7 +361,15 @@ static unsigned int riscv_debug_gdbarch = 0;
>  int
>  riscv_isa_xlen (struct gdbarch *gdbarch)
>  {
> -  return gdbarch_tdep (gdbarch)->features.xlen;
> +  return gdbarch_tdep (gdbarch)->isa_features.xlen;
> +}
> +
> +/* See riscv-tdep.h.  */
> +
> +int
> +riscv_abi_xlen (struct gdbarch *gdbarch)
> +{
> +  return gdbarch_tdep (gdbarch)->abi_features.xlen;
>  }
>  
>  /* See riscv-tdep.h.  */
> @@ -369,7 +377,15 @@ riscv_isa_xlen (struct gdbarch *gdbarch)
>  int
>  riscv_isa_flen (struct gdbarch *gdbarch)
>  {
> -  return gdbarch_tdep (gdbarch)->features.flen;
> +  return gdbarch_tdep (gdbarch)->isa_features.flen;
> +}
> +
> +/* See riscv-tdep.h.  */
> +
> +int
> +riscv_abi_flen (struct gdbarch *gdbarch)
> +{
> +  return gdbarch_tdep (gdbarch)->abi_features.flen;
>  }
>  
>  /* Return true if the target for GDBARCH has floating point hardware.  */
> @@ -385,7 +401,7 @@ riscv_has_fp_regs (struct gdbarch *gdbarch)
>  static bool
>  riscv_has_fp_abi (struct gdbarch *gdbarch)
>  {
> -  return gdbarch_tdep (gdbarch)->features.hw_float_abi;
> +  return gdbarch_tdep (gdbarch)->abi_features.flen > 0;
>  }
>  
>  /* Return true if REGNO is a floating pointer register.  */
> @@ -1777,8 +1793,8 @@ struct riscv_call_info
>      : int_regs (RISCV_A0_REGNUM, RISCV_A0_REGNUM + 7),
>        float_regs (RISCV_FA0_REGNUM, RISCV_FA0_REGNUM + 7)
>    {
> -    xlen = riscv_isa_xlen (gdbarch);
> -    flen = riscv_isa_flen (gdbarch);
> +    xlen = riscv_abi_xlen (gdbarch);
> +    flen = riscv_abi_flen (gdbarch);
>  
>      /* Disable use of floating point registers if needed.  */
>      if (!riscv_has_fp_abi (gdbarch))
> @@ -1798,7 +1814,7 @@ struct riscv_call_info
>    struct riscv_arg_reg float_regs;
>  
>    /* The XLEN and FLEN are copied in to this structure for convenience, and
> -     are just the results of calling RISCV_ISA_XLEN and RISCV_ISA_FLEN.  */
> +     are just the results of calling RISCV_ABI_XLEN and RISCV_ABI_FLEN.  */
>    int xlen;
>    int flen;
>  };
> @@ -2139,9 +2155,6 @@ riscv_call_arg_struct (struct riscv_arg_info *ainfo,
>  	{
>  	  int len0, len1, offset;
>  
> -	  gdb_assert (TYPE_LENGTH (ainfo->type)
> -		      <= (cinfo->flen + cinfo->xlen));
> -
>  	  len0 = TYPE_LENGTH (sinfo.field_type (0));
>  	  if (!riscv_assign_reg_location (&ainfo->argloc[0],
>  					  &cinfo->float_regs, len0, 0))
> @@ -2165,9 +2178,6 @@ riscv_call_arg_struct (struct riscv_arg_info *ainfo,
>  	{
>  	  int len0, len1, offset;
>  
> -	  gdb_assert (TYPE_LENGTH (ainfo->type)
> -		      <= (cinfo->flen + cinfo->xlen));
> -
>  	  len0 = TYPE_LENGTH (sinfo.field_type (0));
>  	  len1 = TYPE_LENGTH (sinfo.field_type (1));
>  	  offset = align_up (len0, riscv_type_alignment (sinfo.field_type (1)));
> @@ -2827,15 +2837,9 @@ riscv_features_from_gdbarch_info (const struct gdbarch_info info)
>  			_("unknown ELF header class %d"), eclass);
>  
>        if (e_flags & EF_RISCV_FLOAT_ABI_DOUBLE)
> -	{
> -	  features.flen = 8;
> -	  features.hw_float_abi = true;
> -	}
> +	features.flen = 8;
>        else if (e_flags & EF_RISCV_FLOAT_ABI_SINGLE)
> -	{
> -	  features.flen = 4;
> -	  features.hw_float_abi = true;
> -	}
> +	features.flen = 4;
>      }
>    else
>      {
> @@ -3039,25 +3043,26 @@ riscv_gdbarch_init (struct gdbarch_info info,
>    /* 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
> +  struct riscv_gdbarch_features abi_features
>      = riscv_features_from_gdbarch_info (info);
> -  if (info_features.xlen != 0 && info_features.xlen != features.xlen)
> +  /* In theory a binary compiled for RV32 could run on an RV64 target,
> +     however, this has not been tested in GDB yet, so for now we require
> +     that the requested xlen match the targets xlen.  */
> +  if (abi_features.xlen != 0 && abi_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)
> +            abi_features.xlen, features.xlen);
> +  /* We do support running binaries compiled for 32-bit float on targets
> +     with 64-bit float, so we only complain if the binary requires more
> +     than the target has available.  */
> +  if (abi_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.
> +            abi_features.flen, features.flen);
>  
> -     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);
> +  /* If the ABI_FEATURES xlen is 0 then this indicates we got no useful abi
> +     features from the INFO object.  In this case we assume that the xlen
> +     abi matches the hardware.  */
> +  if (abi_features.xlen == 0)
> +    abi_features.xlen = features.xlen;
>  
>    /* Find a candidate among the list of pre-declared architectures.  */
>    for (arches = gdbarch_list_lookup_by_info (arches, &info);
> @@ -3069,7 +3074,8 @@ riscv_gdbarch_init (struct gdbarch_info info,
>           gdbarch.  */
>        struct gdbarch_tdep *other_tdep = gdbarch_tdep (arches->gdbarch);
>  
> -      if (other_tdep->features != features)
> +      if (other_tdep->isa_features != features
> +	  || other_tdep->abi_features != abi_features)
>          continue;
>  
>        break;
> @@ -3084,7 +3090,8 @@ riscv_gdbarch_init (struct gdbarch_info info,
>    /* None found, so create a new architecture from the information provided.  */
>    tdep = new (struct gdbarch_tdep);
>    gdbarch = gdbarch_alloc (&info, tdep);
> -  tdep->features = features;
> +  tdep->isa_features = features;
> +  tdep->abi_features = abi_features;
>  
>    /* Target data types.  */
>    set_gdbarch_short_bit (gdbarch, 16);
> diff --git a/gdb/riscv-tdep.h b/gdb/riscv-tdep.h
> index 59ad37df6ce..67a1bb73488 100644
> --- a/gdb/riscv-tdep.h
> +++ b/gdb/riscv-tdep.h
> @@ -59,9 +59,14 @@ enum
>  /* RISC-V specific per-architecture information.  */
>  struct gdbarch_tdep
>  {
> -  /* Features about the target that impact how the gdbarch is configured.
> -     Two gdbarch instances are compatible only if this field matches.  */
> -  struct riscv_gdbarch_features features;
> +  /* Features about the target hardware that impact how the gdbarch is
> +     configured.  Two gdbarch instances are compatible only if this field
> +     matches.  */
> +  struct riscv_gdbarch_features isa_features;
> +
> +  /* Features about the abi that impact how the gdbarch is configured.  Two
> +     gdbarch instances are compatible only if this field matches.  */
> +  struct riscv_gdbarch_features abi_features;
>  
>    /* ISA-specific data types.  */
>    struct type *riscv_fpreg_d_type = nullptr;
> @@ -73,12 +78,30 @@ struct gdbarch_tdep
>     RV128.  */
>  extern int riscv_isa_xlen (struct gdbarch *gdbarch);
>  
> -/* Return the width in bytes of the floating point registers for GDBARCH.
> -   If this architecture has no floating point registers, then return 0.
> -   Possible values are 4, 8, or 16 for depending on which of single, double
> -   or quad floating point support is available.  */
> +/* Return the width in bytes of the hardware floating point registers for
> +   GDBARCH.  If this architecture has no floating point registers, then
> +   return 0.  Possible values are 4, 8, or 16 for depending on which of
> +   single, double or quad floating point support is available.  */
>  extern int riscv_isa_flen (struct gdbarch *gdbarch);
>  
> +/* Return the width in bytes of the general purpose register abi for
> +   GDBARCH.  This can be equal to, or less than RISCV_ISA_XLEN and reflects
> +   how the binary was compiled rather than the hardware that is available.
> +   It is possible that a binary compiled for RV32 is being run on an RV64
> +   target, in which case the isa xlen is 8-bytes, and the abi xlen is
> +   4-bytes.  This will impact how inferior functions are called.  */
> +extern int riscv_abi_xlen (struct gdbarch *gdbarch);
> +
> +/* Return the width in bytes of the floating point register abi for
> +   GDBARCH.  This reflects how the binary was compiled rather than the
> +   hardware that is available.  It is possible that a binary is compiled
> +   for single precision floating point, and then run on a target with
> +   double precision floating point.  A return value of 0 indicates that no
> +   floating point abi is in use (floating point arguments will be passed
> +   in integer registers) other possible return value are 4, 8, or 16 as
> +   with RISCV_ISA_FLEN.  */
> +extern int riscv_abi_flen (struct gdbarch *gdbarch);
> +
>  /* Single step based on where the current instruction will take us.  */
>  extern std::vector<CORE_ADDR> riscv_software_single_step
>    (struct regcache *regcache);
> -- 
> 2.14.5
>