From 113b7b8142427cf7a9ad85fbc39e1319b52649b5 Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Thu, 13 Dec 2018 17:59:12 +0000 Subject: [PATCH] gdb/riscv: Split ISA and ABI features 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) : Delete. : Update with for removed field. : Likewise. * riscv-tdep.h (struct gdbarch_tdep) : Renamed to... : ...this. : 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) : 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/ChangeLog b/gdb/ChangeLog index a6a42e4e36..6bb9ce7fbb 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,32 @@ +2019-01-01 Andrew Burgess + + * arch/riscv.h (struct riscv_gdbarch_features) : + Delete. + : Update with for removed field. + : Likewise. + * riscv-tdep.h (struct gdbarch_tdep) : Renamed to... + : ...this. + : 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) : 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. + 2019-01-01 Philippe Waroquiers * source.c (search_command_helper): Stop reverse search diff --git a/gdb/arch/riscv.h b/gdb/arch/riscv.h index 4a9a80dc12..05c19054dc 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 ac3dd38bb9..33c1c4408e 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. */ @@ -1786,8 +1802,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)) @@ -1807,7 +1823,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; }; @@ -2148,9 +2164,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)) @@ -2174,9 +2187,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))); @@ -2889,15 +2899,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 { @@ -3115,25 +3119,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); @@ -3145,7 +3150,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; @@ -3160,7 +3166,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 212c1bbd30..4268839b47 100644 --- a/gdb/riscv-tdep.h +++ b/gdb/riscv-tdep.h @@ -68,9 +68,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; @@ -82,12 +87,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 riscv_software_single_step (struct regcache *regcache); -- 2.34.1