[PATCH] gdb/riscv: rewrite target description validation, add rv32e support

[PATCH] gdb/riscv: rewrite target description validation, add rv32e support

[Andrew Burgess]

This commit started as adding rv32e support to gdb.  The rv32e
architecture is a cut-down rv32i, it only has 16 x-registers compared
to the usual 32, and an rv32e target should not have any floating
point registers.

In order to add this I needed to adjust the target description
validation checks that are performed from riscv_gdbarch_init, and I
finally got fed up with the current scheme of doing these checks and
rewrote this code.

Unfortunately the rv32e changes are currently mixed in with the
rewrite of the validation scheme.  I could split these apart if anyone
is really interested in seeing these two ideas as separate patches.

The main idea behind this change is that where previously I tried to
have a purely data driven approach, a set of tables one for each
expected feature, and then a single generic function that would
validate a feature given a table, I have created a new class for each
feature.  Each class has its own check member function which allows
the logic for how to check each feature to be different.  I think the
new scheme is much easier to follow.

There are some other changes that I made to the validation code as
part of this commit.

I've relaxed some of the checks related to the floating point CSRs.
Previously the 3 CSRs fflags, frm, and fcsr all had to be present in
either the fpu feature or the csr feature.  This requirement is now
relaxed, if the CSRs are not present then gdb will not reject the
target description.  My thinking here is that there's no gdb
functionality that specifically requires these registers, and so, if a
target offers a description without these registers nothing else in
gdb should stop working.

And as part of the rv32e support targets now only have to provide the
first 16 x-registers and $pc.  The second half of the x-registers (x16
-> x31) are now optional.

gdb/ChangeLog:

	* arch/riscv.c: Include 'rv32e-xregs.c'.
	(riscv_create_target_description): Update to handle rv32e.
	* arch/riscv.h (struct riscv_gdbarch_features) <embedded>: New
	member variable.
	<operator==>: Update to account for new field.
	<hash>: Likewise.
	* features/Makefile (FEATURE_XMLFILES): Add riscv/rv32e-xregs.xml.
	* features/riscv/rv32e-xregs.c: Generated.
	* features/riscv/rv32e-xregs.xml: New file.
	* riscv-tdep.c (riscv_debug_breakpoints): Move from later in the
	file.
	(riscv_debug_infcall): Likewise.
	(riscv_debug_unwinder): Likewise.
	(riscv_debug_gdbarch): Likewise.
	(enum riscv_register_required_status): Delete.
	(struct riscv_register_feature): Add constructor, delete default
	constructor, copy, and assign constructors.
	(struct riscv_register_feature::register_info) <required>: Delete.
	<check>: Update comment and arguments.
	(struct riscv_register_feature) <name>: Change to member function.
	<prefer_first_name>: Delete.
	<tdesc_feature>: New member function.
	<registers>: Rename to...
	<m_registers>: ...this.
	<m_feature_name>: New member variable.
	(riscv_register_feature::register_info::check): Update arguments.
	(riscv_xreg_feature): Rewrite as class, create a single static
	instance of the class.
	(riscv_freg_feature): Likewise.
	(riscv_virtual_feature): Likewise.
	(riscv_csr_feature): Likewise.
	(riscv_create_csr_aliases): Has become a member function inside
	riscv_csr_feature class.
	(riscv_abi_embedded): New function definition.
	(riscv_register_name): Adjust to use new feature objects.
	(struct riscv_call_info) <riscv_call_info>: Check for rv32e abi,
	and adjust available argument registers.
	(riscv_features_from_gdbarch_info): Check for EF_RISCV_RVE flag.
	(riscv_check_tdesc_feature): Delete.
	(riscv_tdesc_unknown_reg): Adjust to use new feature objects.
	(riscv_gdbarch_init): Delete target description checking code, and
	instead call to the new feature objects to perform the checks.
	Reorder handling of no abi information case, allows small code
	simplification.
	(_initialize_riscv_tdep): Remove call, this is now done in the
	riscv_csr_feature constructor.
	* riscv-tdep.h (riscv_abi_embedded): Declare.
---
 gdb/ChangeLog                      |  50 ++
 gdb/arch/riscv.c                   |  15 +-
 gdb/arch/riscv.h                   |   9 +-
 gdb/features/Makefile              |   1 +
 gdb/features/riscv/rv32e-xregs.c   |  30 ++
 gdb/features/riscv/rv32e-xregs.xml |  31 ++
 gdb/riscv-tdep.c                   | 729 ++++++++++++++++-------------
 gdb/riscv-tdep.h                   |   5 +
 8 files changed, 542 insertions(+), 328 deletions(-)
 create mode 100644 gdb/features/riscv/rv32e-xregs.c
 create mode 100644 gdb/features/riscv/rv32e-xregs.xml

diff --git a/gdb/arch/riscv.c b/gdb/arch/riscv.c
index a6538dee541..64f394035c3 100644
--- a/gdb/arch/riscv.c
+++ b/gdb/arch/riscv.c
@@ -24,6 +24,7 @@
 #include "../features/riscv/64bit-cpu.c"
 #include "../features/riscv/32bit-fpu.c"
 #include "../features/riscv/64bit-fpu.c"
+#include "../features/riscv/rv32e-xregs.c"
 
 #ifndef GDBSERVER
 #define STATIC_IN_GDB static
@@ -43,7 +44,12 @@ riscv_create_target_description (const struct riscv_gdbarch_features features)
   std::string arch_name = "riscv";
 
   if (features.xlen == 4)
-    arch_name.append (":rv32i");
+    {
+      if (features.embedded)
+	arch_name.append (":rv32e");
+      else
+	arch_name.append (":rv32i");
+    }
   else if (features.xlen == 8)
     arch_name.append (":rv64i");
   else if (features.xlen == 16)
@@ -63,7 +69,12 @@ riscv_create_target_description (const struct riscv_gdbarch_features features)
 
   /* For now we only support creating 32-bit or 64-bit x-registers.  */
   if (features.xlen == 4)
-    regnum = create_feature_riscv_32bit_cpu (tdesc.get (), regnum);
+    {
+      if (features.embedded)
+	regnum = create_feature_riscv_rv32e_xregs (tdesc.get (), regnum);
+      else
+	regnum = create_feature_riscv_32bit_cpu (tdesc.get (), regnum);
+    }
   else if (features.xlen == 8)
     regnum = create_feature_riscv_64bit_cpu (tdesc.get (), regnum);
 
diff --git a/gdb/arch/riscv.h b/gdb/arch/riscv.h
index 26db0dab0e0..f91c077a001 100644
--- a/gdb/arch/riscv.h
+++ b/gdb/arch/riscv.h
@@ -46,10 +46,13 @@ struct riscv_gdbarch_features
      that there are no f-registers.  No other value is valid.  */
   int flen = 0;
 
+  /* When true this target is RV32E.  */
+  bool embedded = false;
+
   /* Equality operator.  */
   bool operator== (const struct riscv_gdbarch_features &rhs) const
   {
-    return (xlen == rhs.xlen && flen == rhs.flen);
+    return (xlen == rhs.xlen && flen == rhs.flen && embedded == rhs.embedded);
   }
 
   /* Inequality operator.  */
@@ -61,7 +64,9 @@ 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) << 5 | (flen & 0x1f) << 0);
+    std::size_t val = ((embedded ? 1 : 0) << 10
+		       | (xlen & 0x1f) << 5
+		       | (flen & 0x1f) << 0);
     return val;
   }
 };
diff --git a/gdb/features/Makefile b/gdb/features/Makefile
index d5b3236242d..61a4fe783f2 100644
--- a/gdb/features/Makefile
+++ b/gdb/features/Makefile
@@ -229,6 +229,7 @@ FEATURE_XMLFILES = aarch64-core.xml \
 	i386/64bit-pkeys.xml \
 	i386/64bit-sse.xml \
 	i386/x32-core.xml \
+	riscv/rv32e-xregs.xml \
 	riscv/32bit-cpu.xml \
 	riscv/32bit-fpu.xml \
 	riscv/64bit-cpu.xml \
diff --git a/gdb/features/riscv/rv32e-xregs.c b/gdb/features/riscv/rv32e-xregs.c
new file mode 100644
index 00000000000..cda33195db7
--- /dev/null
+++ b/gdb/features/riscv/rv32e-xregs.c
@@ -0,0 +1,30 @@
+/* THIS FILE IS GENERATED.  -*- buffer-read-only: t -*- vi:set ro:
+  Original: rv32e-xregs.xml */
+
+#include "gdbsupport/tdesc.h"
+
+static int
+create_feature_riscv_rv32e_xregs (struct target_desc *result, long regnum)
+{
+  struct tdesc_feature *feature;
+
+  feature = tdesc_create_feature (result, "org.gnu.gdb.riscv.cpu");
+  tdesc_create_reg (feature, "zero", regnum++, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "ra", regnum++, 1, NULL, 32, "code_ptr");
+  tdesc_create_reg (feature, "sp", regnum++, 1, NULL, 32, "data_ptr");
+  tdesc_create_reg (feature, "gp", regnum++, 1, NULL, 32, "data_ptr");
+  tdesc_create_reg (feature, "tp", regnum++, 1, NULL, 32, "data_ptr");
+  tdesc_create_reg (feature, "t0", regnum++, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "t1", regnum++, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "t2", regnum++, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "fp", regnum++, 1, NULL, 32, "data_ptr");
+  tdesc_create_reg (feature, "s1", regnum++, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "a0", regnum++, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "a1", regnum++, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "a2", regnum++, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "a3", regnum++, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "a4", regnum++, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "a5", regnum++, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "pc", regnum++, 1, NULL, 32, "code_ptr");
+  return regnum;
+}
diff --git a/gdb/features/riscv/rv32e-xregs.xml b/gdb/features/riscv/rv32e-xregs.xml
new file mode 100644
index 00000000000..8d313e871fd
--- /dev/null
+++ b/gdb/features/riscv/rv32e-xregs.xml
@@ -0,0 +1,31 @@
+<?xml version="1.0"?>
+<!-- Copyright (C) 2018-2020 Free Software Foundation, Inc.
+
+     Copying and distribution of this file, with or without modification,
+     are permitted in any medium without royalty provided the copyright
+     notice and this notice are preserved.  -->
+
+<!-- Register numbers are hard-coded in order to maintain backward
+     compatibility with older versions of tools that didn't use xml
+     register descriptions.  -->
+
+<!DOCTYPE feature SYSTEM "gdb-target.dtd">
+<feature name="org.gnu.gdb.riscv.cpu">
+  <reg name="zero" bitsize="32" type="int" regnum="0"/>
+  <reg name="ra" bitsize="32" type="code_ptr"/>
+  <reg name="sp" bitsize="32" type="data_ptr"/>
+  <reg name="gp" bitsize="32" type="data_ptr"/>
+  <reg name="tp" bitsize="32" type="data_ptr"/>
+  <reg name="t0" bitsize="32" type="int"/>
+  <reg name="t1" bitsize="32" type="int"/>
+  <reg name="t2" bitsize="32" type="int"/>
+  <reg name="fp" bitsize="32" type="data_ptr"/>
+  <reg name="s1" bitsize="32" type="int"/>
+  <reg name="a0" bitsize="32" type="int"/>
+  <reg name="a1" bitsize="32" type="int"/>
+  <reg name="a2" bitsize="32" type="int"/>
+  <reg name="a3" bitsize="32" type="int"/>
+  <reg name="a4" bitsize="32" type="int"/>
+  <reg name="a5" bitsize="32" type="int"/>
+  <reg name="pc" bitsize="32" type="code_ptr"/>
+</feature>
diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 4e255056863..62be5678a39 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -74,6 +74,26 @@ static inline bool is_ ## INSN_NAME ## _insn (long insn) \
 #include "opcode/riscv-opc.h"
 #undef DECLARE_INSN
 
+/* When this is set to non-zero debugging information about breakpoint
+   kinds will be printed.  */
+
+static unsigned int riscv_debug_breakpoints = 0;
+
+/* When this is set to non-zero debugging information about inferior calls
+   will be printed.  */
+
+static unsigned int riscv_debug_infcall = 0;
+
+/* When this is set to non-zero debugging information about stack unwinding
+   will be printed.  */
+
+static unsigned int riscv_debug_unwinder = 0;
+
+/* When this is set to non-zero debugging information about gdbarch
+   initialisation will be printed.  */
+
+static unsigned int riscv_debug_gdbarch = 0;
+
 /* Cached information about a frame.  */
 
 struct riscv_unwind_cache
@@ -141,31 +161,18 @@ class riscv_pending_register_alias
   const void *m_baton;
 };
 
-/* Registers in the RISCV_REGISTER_FEATURE lists below are either optional,
-   or required.  For example the $pc register is always going to be a
-   required register, you can't do much debugging without that.  In
-   contrast, most of the CSRs are optional, GDB doesn't require them in
-   order to have a useful debug session.  This enum models the difference
-   between these register types.  */
-
-enum riscv_register_required_status
-{
-  /* This register is optional within this feature.  */
-  RISCV_REG_OPTIONAL,
-
-  /* This register is required within this feature.  */
-  RISCV_REG_REQUIRED,
-
-  /* This register is required, the register must either be in this
-     feature, or it could appear within the CSR feature.  */
-  RISCV_REG_REQUIRED_MAYBE_CSR
-};
-
 /* A set of registers that we expect to find in a tdesc_feature.  These
    are use in RISCV_GDBARCH_INIT when processing the target description.  */
 
 struct riscv_register_feature
 {
+  explicit riscv_register_feature (const char *feature_name)
+    : m_feature_name (feature_name)
+  { /* Delete.  */ }
+
+  riscv_register_feature () = delete;
+  DISABLE_COPY_AND_ASSIGN (riscv_register_feature);
+
   /* Information for a single register.  */
   struct register_info
   {
@@ -177,41 +184,39 @@ struct riscv_register_feature
        register.  */
     std::vector<const char *> names;
 
-    /* Is this register required within this feature?  In some cases the
-       register could be required, but might also be in the CSR feature.  */
-    riscv_register_required_status required;
-
     /* Look in FEATURE for a register with a name from this classes names
        list.  If the register is found then register its number with
-       TDESC_DATA and add all its aliases to the ALIASES list.  REG_SET is
-       used to help create the aliases.  */
+       TDESC_DATA and add all its aliases to the ALIASES list.
+       PREFER_FIRST_NAME_P is used when deciding which aliases to create.  */
     bool check (struct tdesc_arch_data *tdesc_data,
 		const struct tdesc_feature *feature,
-		const struct riscv_register_feature *reg_set,
+		bool prefer_first_name_p,
 		std::vector<riscv_pending_register_alias> *aliases) const;
   };
 
-  /* The name for this feature.  This is the name used to find this feature
-     within the target description.  */
-  const char *name;
+  /* Return the name of this feature.  */
+  const char *name () const
+  { return m_feature_name; }
 
-  /* For x-regs and f-regs we always force GDB to use the first name from
-     the REGISTERS.NAMES vector, it is therefore important that we create
-     user-register aliases for all of the remaining names at indexes 1+ in
-     the names vector.
+protected:
 
-     For CSRs we take a different approach, we prefer whatever name the
-     target description uses, in this case we want to create user-register
-     aliases for any other names that aren't the target description
-     provided name.
-
-     When this flag is true we are dealing with the first case, and when
-     this is false we are dealing with the latter.  */
-  bool prefer_first_name;
+  /* Return a target description feature extracted from TDESC for this
+     register feature.  Will return nullptr if there is no feature in TDESC
+     with the name M_FEATURE_NAME.  */
+  const struct tdesc_feature *tdesc_feature (const struct target_desc *tdesc) const
+  {
+    return tdesc_find_feature (tdesc, name ());
+  }
 
   /* List of all the registers that we expect that we might find in this
      register set.  */
-  std::vector<struct register_info> registers;
+  std::vector<struct register_info> m_registers;
+
+private:
+
+  /* The name for this feature.  This is the name used to find this feature
+     within the target description.  */
+  const char *m_feature_name;
 };
 
 /* See description in the class declaration above.  */
@@ -220,7 +225,7 @@ bool
 riscv_register_feature::register_info::check
 	(struct tdesc_arch_data *tdesc_data,
 	 const struct tdesc_feature *feature,
-	 const struct riscv_register_feature *reg_set,
+	 bool prefer_first_name_p,
 	 std::vector<riscv_pending_register_alias> *aliases) const
 {
   for (const char *name : this->names)
@@ -233,12 +238,11 @@ riscv_register_feature::register_info::check
 	     register.  In RISCV_REGISTER_NAME we ensure that GDB
 	     always uses the first name for each register, so here we
 	     add aliases for all of the remaining names.  */
-	  bool prefer_first_name = reg_set->prefer_first_name;
-	  int start_index = prefer_first_name ? 1 : 0;
+	  int start_index = prefer_first_name_p ? 1 : 0;
 	  for (int i = start_index; i < this->names.size (); ++i)
 	    {
 	      const char *alias = this->names[i];
-	      if (alias == name && !prefer_first_name)
+	      if (alias == name && !prefer_first_name_p)
 		continue;
 	      aliases->emplace_back (alias, (void *) &this->regnum);
 	    }
@@ -248,146 +252,332 @@ riscv_register_feature::register_info::check
   return false;
 }
 
-/* The general x-registers feature set.  */
-
-static const struct riscv_register_feature riscv_xreg_feature =
-{
- "org.gnu.gdb.riscv.cpu", true,
- {
-   { RISCV_ZERO_REGNUM + 0, { "zero", "x0" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 1, { "ra", "x1" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 2, { "sp", "x2" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 3, { "gp", "x3" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 4, { "tp", "x4" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 5, { "t0", "x5" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 6, { "t1", "x6" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 7, { "t2", "x7" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 8, { "fp", "x8", "s0" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 9, { "s1", "x9" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 10, { "a0", "x10" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 11, { "a1", "x11" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 12, { "a2", "x12" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 13, { "a3", "x13" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 14, { "a4", "x14" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 15, { "a5", "x15" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 16, { "a6", "x16" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 17, { "a7", "x17" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 18, { "s2", "x18" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 19, { "s3", "x19" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 20, { "s4", "x20" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 21, { "s5", "x21" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 22, { "s6", "x22" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 23, { "s7", "x23" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 24, { "s8", "x24" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 25, { "s9", "x25" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 26, { "s10", "x26" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 27, { "s11", "x27" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 28, { "t3", "x28" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 29, { "t4", "x29" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 30, { "t5", "x30" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 31, { "t6", "x31" }, RISCV_REG_REQUIRED },
-   { RISCV_ZERO_REGNUM + 32, { "pc" }, RISCV_REG_REQUIRED }
- }
+/* Class representing the x-registers feature set.  */
+
+struct riscv_xreg_feature : public riscv_register_feature
+{
+  riscv_xreg_feature ()
+    : riscv_register_feature ("org.gnu.gdb.riscv.cpu")
+  {
+    m_registers =  {
+      { RISCV_ZERO_REGNUM + 0, { "zero", "x0" } },
+      { RISCV_ZERO_REGNUM + 1, { "ra", "x1" } },
+      { RISCV_ZERO_REGNUM + 2, { "sp", "x2" } },
+      { RISCV_ZERO_REGNUM + 3, { "gp", "x3" } },
+      { RISCV_ZERO_REGNUM + 4, { "tp", "x4" } },
+      { RISCV_ZERO_REGNUM + 5, { "t0", "x5" } },
+      { RISCV_ZERO_REGNUM + 6, { "t1", "x6" } },
+      { RISCV_ZERO_REGNUM + 7, { "t2", "x7" } },
+      { RISCV_ZERO_REGNUM + 8, { "fp", "x8", "s0" } },
+      { RISCV_ZERO_REGNUM + 9, { "s1", "x9" } },
+      { RISCV_ZERO_REGNUM + 10, { "a0", "x10" } },
+      { RISCV_ZERO_REGNUM + 11, { "a1", "x11" } },
+      { RISCV_ZERO_REGNUM + 12, { "a2", "x12" } },
+      { RISCV_ZERO_REGNUM + 13, { "a3", "x13" } },
+      { RISCV_ZERO_REGNUM + 14, { "a4", "x14" } },
+      { RISCV_ZERO_REGNUM + 15, { "a5", "x15" } },
+      { RISCV_ZERO_REGNUM + 16, { "a6", "x16" } },
+      { RISCV_ZERO_REGNUM + 17, { "a7", "x17" } },
+      { RISCV_ZERO_REGNUM + 18, { "s2", "x18" } },
+      { RISCV_ZERO_REGNUM + 19, { "s3", "x19" } },
+      { RISCV_ZERO_REGNUM + 20, { "s4", "x20" } },
+      { RISCV_ZERO_REGNUM + 21, { "s5", "x21" } },
+      { RISCV_ZERO_REGNUM + 22, { "s6", "x22" } },
+      { RISCV_ZERO_REGNUM + 23, { "s7", "x23" } },
+      { RISCV_ZERO_REGNUM + 24, { "s8", "x24" } },
+      { RISCV_ZERO_REGNUM + 25, { "s9", "x25" } },
+      { RISCV_ZERO_REGNUM + 26, { "s10", "x26" } },
+      { RISCV_ZERO_REGNUM + 27, { "s11", "x27" } },
+      { RISCV_ZERO_REGNUM + 28, { "t3", "x28" } },
+      { RISCV_ZERO_REGNUM + 29, { "t4", "x29" } },
+      { RISCV_ZERO_REGNUM + 30, { "t5", "x30" } },
+      { RISCV_ZERO_REGNUM + 31, { "t6", "x31" } },
+      { RISCV_ZERO_REGNUM + 32, { "pc" } }
+    };
+  }
+
+  /* Return the preferred name for the register with gdb register number
+     REGNUM, which must be in the inclusive range RISCV_ZERO_REGNUM to
+     RISCV_PC_REGNUM.  */
+  const char *register_name (int regnum) const
+  {
+    gdb_assert (regnum >= RISCV_ZERO_REGNUM && regnum <= m_registers.size ());
+    return m_registers[regnum].names[0];
+  }
+
+  /* Check this feature within TDESC, record the registers from this
+     feature into TDESC_DATA and update ALIASES and FEATURES.  */
+  bool check (const struct target_desc *tdesc,
+	      struct tdesc_arch_data *tdesc_data,
+	      std::vector<riscv_pending_register_alias> *aliases,
+	      struct riscv_gdbarch_features *features) const
+  {
+    const struct tdesc_feature *feature_cpu = tdesc_feature (tdesc);
+
+    if (feature_cpu == nullptr)
+      return false;
+
+    bool seen_an_optional_reg_p = false;
+    for (const auto &reg : m_registers)
+      {
+	bool found = reg.check (tdesc_data, feature_cpu, true, aliases);
+
+	bool is_optional_reg_p = (reg.regnum >= RISCV_ZERO_REGNUM + 16
+				  && reg.regnum < RISCV_ZERO_REGNUM + 32);
+
+	if (!found && (!is_optional_reg_p || seen_an_optional_reg_p))
+	  return false;
+	else if (found && is_optional_reg_p)
+	  seen_an_optional_reg_p = true;
+      }
+
+    /* Check that all of the core cpu registers have the same bitsize.  */
+    int xlen_bitsize = tdesc_register_bitsize (feature_cpu, "pc");
+
+    bool valid_p = true;
+    for (auto &tdesc_reg : feature_cpu->registers)
+      valid_p &= (tdesc_reg->bitsize == xlen_bitsize);
+
+    features->xlen = (xlen_bitsize / 8);
+    features->embedded = !seen_an_optional_reg_p;
+
+    return valid_p;
+  }
 };
 
-/* The f-registers feature set.  */
-
-static const struct riscv_register_feature riscv_freg_feature =
-{
- "org.gnu.gdb.riscv.fpu", true,
- {
-   { RISCV_FIRST_FP_REGNUM + 0, { "ft0", "f0" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 1, { "ft1", "f1" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 2, { "ft2", "f2" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 3, { "ft3", "f3" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 4, { "ft4", "f4" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 5, { "ft5", "f5" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 6, { "ft6", "f6" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 7, { "ft7", "f7" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 8, { "fs0", "f8" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 9, { "fs1", "f9" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 10, { "fa0", "f10" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 11, { "fa1", "f11" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 12, { "fa2", "f12" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 13, { "fa3", "f13" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 14, { "fa4", "f14" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 15, { "fa5", "f15" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 16, { "fa6", "f16" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 17, { "fa7", "f17" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 18, { "fs2", "f18" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 19, { "fs3", "f19" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 20, { "fs4", "f20" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 21, { "fs5", "f21" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 22, { "fs6", "f22" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 23, { "fs7", "f23" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 24, { "fs8", "f24" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 25, { "fs9", "f25" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 26, { "fs10", "f26" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 27, { "fs11", "f27" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 28, { "ft8", "f28" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 29, { "ft9", "f29" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 30, { "ft10", "f30" }, RISCV_REG_REQUIRED },
-   { RISCV_FIRST_FP_REGNUM + 31, { "ft11", "f31" }, RISCV_REG_REQUIRED },
-
-   { RISCV_CSR_FFLAGS_REGNUM, { "fflags", "csr1" }, RISCV_REG_REQUIRED_MAYBE_CSR },
-   { RISCV_CSR_FRM_REGNUM, { "frm", "csr2" }, RISCV_REG_REQUIRED_MAYBE_CSR },
-   { RISCV_CSR_FCSR_REGNUM, { "fcsr", "csr3" }, RISCV_REG_REQUIRED_MAYBE_CSR },
-
- }
+/* An instance of the x-register feature set.  */
+
+static const struct riscv_xreg_feature riscv_xreg_feature;
+
+/* Class representing the f-registers feature set.  */
+
+struct riscv_freg_feature : public riscv_register_feature
+{
+  riscv_freg_feature ()
+    : riscv_register_feature ("org.gnu.gdb.riscv.fpu")
+  {
+    m_registers =  {
+      { RISCV_FIRST_FP_REGNUM + 0, { "ft0", "f0" } },
+      { RISCV_FIRST_FP_REGNUM + 1, { "ft1", "f1" } },
+      { RISCV_FIRST_FP_REGNUM + 2, { "ft2", "f2" } },
+      { RISCV_FIRST_FP_REGNUM + 3, { "ft3", "f3" } },
+      { RISCV_FIRST_FP_REGNUM + 4, { "ft4", "f4" } },
+      { RISCV_FIRST_FP_REGNUM + 5, { "ft5", "f5" } },
+      { RISCV_FIRST_FP_REGNUM + 6, { "ft6", "f6" } },
+      { RISCV_FIRST_FP_REGNUM + 7, { "ft7", "f7" } },
+      { RISCV_FIRST_FP_REGNUM + 8, { "fs0", "f8" } },
+      { RISCV_FIRST_FP_REGNUM + 9, { "fs1", "f9" } },
+      { RISCV_FIRST_FP_REGNUM + 10, { "fa0", "f10" } },
+      { RISCV_FIRST_FP_REGNUM + 11, { "fa1", "f11" } },
+      { RISCV_FIRST_FP_REGNUM + 12, { "fa2", "f12" } },
+      { RISCV_FIRST_FP_REGNUM + 13, { "fa3", "f13" } },
+      { RISCV_FIRST_FP_REGNUM + 14, { "fa4", "f14" } },
+      { RISCV_FIRST_FP_REGNUM + 15, { "fa5", "f15" } },
+      { RISCV_FIRST_FP_REGNUM + 16, { "fa6", "f16" } },
+      { RISCV_FIRST_FP_REGNUM + 17, { "fa7", "f17" } },
+      { RISCV_FIRST_FP_REGNUM + 18, { "fs2", "f18" } },
+      { RISCV_FIRST_FP_REGNUM + 19, { "fs3", "f19" } },
+      { RISCV_FIRST_FP_REGNUM + 20, { "fs4", "f20" } },
+      { RISCV_FIRST_FP_REGNUM + 21, { "fs5", "f21" } },
+      { RISCV_FIRST_FP_REGNUM + 22, { "fs6", "f22" } },
+      { RISCV_FIRST_FP_REGNUM + 23, { "fs7", "f23" } },
+      { RISCV_FIRST_FP_REGNUM + 24, { "fs8", "f24" } },
+      { RISCV_FIRST_FP_REGNUM + 25, { "fs9", "f25" } },
+      { RISCV_FIRST_FP_REGNUM + 26, { "fs10", "f26" } },
+      { RISCV_FIRST_FP_REGNUM + 27, { "fs11", "f27" } },
+      { RISCV_FIRST_FP_REGNUM + 28, { "ft8", "f28" } },
+      { RISCV_FIRST_FP_REGNUM + 29, { "ft9", "f29" } },
+      { RISCV_FIRST_FP_REGNUM + 30, { "ft10", "f30" } },
+      { RISCV_FIRST_FP_REGNUM + 31, { "ft11", "f31" } },
+      { RISCV_CSR_FFLAGS_REGNUM, { "fflags", "csr1" } },
+      { RISCV_CSR_FRM_REGNUM, { "frm", "csr2" } },
+      { RISCV_CSR_FCSR_REGNUM, { "fcsr", "csr3" } },
+    };
+  }
+
+  /* Return the preferred name for the register with gdb register number
+     REGNUM, which must be in the inclusive range RISCV_FIRST_FP_REGNUM to
+     RISCV_LAST_FP_REGNUM.  */
+  const char *register_name (int regnum) const
+  {
+    gdb_static_assert (RISCV_LAST_FP_REGNUM == RISCV_FIRST_FP_REGNUM + 31);
+    gdb_assert (regnum >= RISCV_FIRST_FP_REGNUM
+		&& regnum <= RISCV_LAST_FP_REGNUM);
+    regnum -= RISCV_FIRST_FP_REGNUM;
+    return m_registers[regnum].names[0];
+  }
+
+  /* Check this feature within TDESC, record the registers from this
+     feature into TDESC_DATA and update ALIASES and FEATURES.  */
+  bool check (const struct target_desc *tdesc,
+	      struct tdesc_arch_data *tdesc_data,
+	      std::vector<riscv_pending_register_alias> *aliases,
+	      struct riscv_gdbarch_features *features) const
+  {
+    const struct tdesc_feature *feature_fpu = tdesc_feature (tdesc);
+
+    /* It's fine if this feature is missing.  Update the architecture
+       feature set and return.  */
+    if (feature_fpu == nullptr)
+      {
+	features->flen = 0;
+	return true;
+      }
+
+    /* Check all of the floating pointer registers are present.  We also
+       check that the floating point CSRs are present too, though if these
+       are missing this is not fatal.  */
+    for (const auto &reg : m_registers)
+      {
+	bool found = reg.check (tdesc_data, feature_fpu, true, aliases);
+
+	bool is_ctrl_reg_p = reg.regnum > RISCV_LAST_FP_REGNUM;
+
+	if (!found && !is_ctrl_reg_p)
+	  return false;
+      }
+
+    /* Look through all of the floating point registers (not the FP CSRs
+       though), and check they all have the same bitsize.  Use this bitsize
+       to update the feature set for this gdbarch.  */
+    int fp_bitsize = -1;
+    for (const auto &reg : m_registers)
+      {
+	/* Stop once we get to the CSRs which are at the end of the
+	   M_REGISTERS list.  */
+	if (reg.regnum > RISCV_LAST_FP_REGNUM)
+	  break;
+
+	int reg_bitsize = -1;
+	for (const char *name : reg.names)
+	  {
+	    if (tdesc_unnumbered_register (feature_fpu, name))
+	      {
+		reg_bitsize = tdesc_register_bitsize (feature_fpu, name);
+		break;
+	      }
+	  }
+	gdb_assert (reg_bitsize != -1);
+	if (fp_bitsize == -1)
+	  fp_bitsize = reg_bitsize;
+	else if (fp_bitsize != reg_bitsize)
+	  return false;
+      }
+
+    features->flen = (fp_bitsize / 8);
+    return true;
+  }
 };
 
-/* Set of virtual registers.  These are not physical registers on the
-   hardware, but might be available from the target.  These are not pseudo
-   registers, reading these really does result in a register read from the
-   target, it is just that there might not be a physical register backing
-   the result.  */
+/* An instance of the f-register feature set.  */
 
-static const struct riscv_register_feature riscv_virtual_feature =
+static const struct riscv_freg_feature riscv_freg_feature;
+
+/* Class representing the virtual registers.  These are not physical
+   registers on the hardware, but might be available from the target.
+   These are not pseudo registers, reading these really does result in a
+   register read from the target, it is just that there might not be a
+   physical register backing the result.  */
+
+struct riscv_virtual_feature : public riscv_register_feature
 {
- "org.gnu.gdb.riscv.virtual", false,
- {
-   { RISCV_PRIV_REGNUM, { "priv" }, RISCV_REG_OPTIONAL }
- }
+  riscv_virtual_feature ()
+    : riscv_register_feature ("org.gnu.gdb.riscv.virtual")
+  {
+    m_registers =  {
+      { RISCV_PRIV_REGNUM, { "priv" } }
+    };
+  }
+
+  bool check (const struct target_desc *tdesc,
+	      struct tdesc_arch_data *tdesc_data,
+	      std::vector<riscv_pending_register_alias> *aliases,
+	      struct riscv_gdbarch_features *features) const
+  {
+    const struct tdesc_feature *feature_virtual = tdesc_feature (tdesc);
+
+    /* It's fine if this feature is missing.  */
+    if (feature_virtual == nullptr)
+      return true;
+
+    /* We don't check the return value from the call to check here, all the
+       registers in this feature are optional.  */
+    for (const auto &reg : m_registers)
+      reg.check (tdesc_data, feature_virtual, true, aliases);
+
+    return true;
+  }
 };
 
-/* Feature set for CSRs.  This set is NOT constant as the register names
-   list for each register is not complete.  The aliases are computed
-   during RISCV_CREATE_CSR_ALIASES.  */
+/* An instance of the virtual register feature.  */
+
+static const struct riscv_virtual_feature riscv_virtual_feature;
 
-static struct riscv_register_feature riscv_csr_feature =
+/* Class representing the CSR feature.  */
+
+struct riscv_csr_feature : public riscv_register_feature
 {
- "org.gnu.gdb.riscv.csr", false,
- {
-#define DECLARE_CSR(NAME,VALUE,CLASS,DEFINE_VER,ABORT_VER) \
-  { RISCV_ ## VALUE ## _REGNUM, { # NAME }, RISCV_REG_OPTIONAL },
+  riscv_csr_feature ()
+    : riscv_register_feature ("org.gnu.gdb.riscv.csr")
+  {
+    m_registers = {
+#define DECLARE_CSR(NAME,VALUE,CLASS,DEFINE_VER,ABORT_VER)		\
+      { RISCV_ ## VALUE ## _REGNUM, { # NAME } },
 #include "opcode/riscv-opc.h"
 #undef DECLARE_CSR
- }
-};
+    };
+    riscv_create_csr_aliases ();
+  }
+
+  bool check (const struct target_desc *tdesc,
+	      struct tdesc_arch_data *tdesc_data,
+	      std::vector<riscv_pending_register_alias> *aliases,
+	      struct riscv_gdbarch_features *features) const
+  {
+    const struct tdesc_feature *feature_csr = tdesc_feature (tdesc);
 
-/* Complete RISCV_CSR_FEATURE, building the CSR alias names and adding them
-   to the name list for each register.  */
+    /* It's fine if this feature is missing.  */
+    if (feature_csr == nullptr)
+      return true;
 
-static void
-riscv_create_csr_aliases ()
-{
-  for (auto &reg : riscv_csr_feature.registers)
-    {
-      int csr_num = reg.regnum - RISCV_FIRST_CSR_REGNUM;
-      const char *alias = xstrprintf ("csr%d", csr_num);
-      reg.names.push_back (alias);
-
-      /* Setup the other csr aliases.  We don't use a switch table here in
-	 case there are multiple aliases with the same value.  Also filter
-	 based on ABRT_VER in order to avoid a very old alias for misa that
-	 duplicates the name "misa" but at a different CSR address.  */
-#define DECLARE_CSR_ALIAS(NAME,VALUE,CLASS,DEF_VER,ABRT_VER)	 \
-      if (csr_num == VALUE && ABRT_VER >= PRIV_SPEC_CLASS_1P11)  \
-	reg.names.push_back ( # NAME );
+    /* We don't check the return value from the call to check here, all the
+       registers in this feature are optional.  */
+    for (const auto &reg : m_registers)
+      reg.check (tdesc_data, feature_csr, true, aliases);
+
+    return true;
+  }
+
+private:
+
+  /* Complete RISCV_CSR_FEATURE, building the CSR alias names and adding them
+     to the name list for each register.  */
+
+  void
+  riscv_create_csr_aliases ()
+  {
+    for (auto &reg : m_registers)
+      {
+	int csr_num = reg.regnum - RISCV_FIRST_CSR_REGNUM;
+	const char *alias = xstrprintf ("csr%d", csr_num);
+	reg.names.push_back (alias);
+
+	/* Setup the other csr aliases.  We don't use a switch table here in
+	   case there are multiple aliases with the same value.  Also filter
+	   based on ABRT_VER in order to avoid a very old alias for misa that
+	   duplicates the name "misa" but at a different CSR address.  */
+#define DECLARE_CSR_ALIAS(NAME,VALUE,CLASS,DEF_VER,ABRT_VER)		\
+	if (csr_num == VALUE && ABRT_VER >= PRIV_SPEC_CLASS_1P11)	\
+	  reg.names.push_back ( # NAME );
 #include "opcode/riscv-opc.h"
 #undef DECLARE_CSR_ALIAS
-    }
-}
+      }
+  }
+};
+
+/* An instance of the csr register feature.  */
+
+static const struct riscv_csr_feature riscv_csr_feature;
 
 /* Controls whether we place compressed breakpoints or not.  When in auto
    mode GDB tries to determine if the target supports compressed
@@ -429,26 +619,6 @@ show_riscv_debug_variable (struct ui_file *file, int from_tty,
 		    c->name, value);
 }
 
-/* When this is set to non-zero debugging information about breakpoint
-   kinds will be printed.  */
-
-static unsigned int riscv_debug_breakpoints = 0;
-
-/* When this is set to non-zero debugging information about inferior calls
-   will be printed.  */
-
-static unsigned int riscv_debug_infcall = 0;
-
-/* When this is set to non-zero debugging information about stack unwinding
-   will be printed.  */
-
-static unsigned int riscv_debug_unwinder = 0;
-
-/* When this is set to non-zero debugging information about gdbarch
-   initialisation will be printed.  */
-
-static unsigned int riscv_debug_gdbarch = 0;
-
 /* See riscv-tdep.h.  */
 
 int
@@ -481,6 +651,14 @@ riscv_abi_flen (struct gdbarch *gdbarch)
   return gdbarch_tdep (gdbarch)->abi_features.flen;
 }
 
+/* See riscv-tdep.h.  */
+
+bool
+riscv_abi_embedded (struct gdbarch *gdbarch)
+{
+  return gdbarch_tdep (gdbarch)->abi_features.embedded;
+}
+
 /* Return true if the target for GDBARCH has floating point hardware.  */
 
 static bool
@@ -598,21 +776,14 @@ riscv_register_name (struct gdbarch *gdbarch, int regnum)
      example we want to see 'ra' instead of 'x1' whatever the target
      description called it.  */
   if (regnum >= RISCV_ZERO_REGNUM && regnum < RISCV_FIRST_FP_REGNUM)
-    {
-      gdb_assert (regnum < riscv_xreg_feature.registers.size ());
-      return riscv_xreg_feature.registers[regnum].names[0];
-    }
+    return riscv_xreg_feature.register_name (regnum);
 
   /* Like with the x-regs we prefer the abi names for the floating point
      registers.  */
   if (regnum >= RISCV_FIRST_FP_REGNUM && regnum <= RISCV_LAST_FP_REGNUM)
     {
       if (riscv_has_fp_regs (gdbarch))
-	{
-	  regnum -= RISCV_FIRST_FP_REGNUM;
-	  gdb_assert (regnum < riscv_freg_feature.registers.size ());
-	  return riscv_freg_feature.registers[regnum].names[0];
-	}
+	return riscv_freg_feature.register_name (regnum);
       else
 	return NULL;
     }
@@ -1915,6 +2086,11 @@ struct riscv_call_info
     xlen = riscv_abi_xlen (gdbarch);
     flen = riscv_abi_flen (gdbarch);
 
+    /* Reduce the number of integer argument registers when using the
+       embedded abi (i.e. rv32e).  */
+    if (riscv_abi_embedded (gdbarch))
+      int_regs.last_regnum = RISCV_A0_REGNUM + 5;
+
     /* Disable use of floating point registers if needed.  */
     if (!riscv_has_fp_abi (gdbarch))
       float_regs.next_regnum = float_regs.last_regnum + 1;
@@ -3063,6 +3239,16 @@ riscv_features_from_gdbarch_info (const struct gdbarch_info info)
 	features.flen = 8;
       else if (e_flags & EF_RISCV_FLOAT_ABI_SINGLE)
 	features.flen = 4;
+
+      if (e_flags & EF_RISCV_RVE)
+	{
+	  if (features.xlen == 8)
+	    {
+	      warning (_("64-bit ELF with RV32E flag set!  Assuming 32-bit"));
+	      features.xlen = 4;
+	    }
+	  features.embedded = true;
+	}
     }
 
   return features;
@@ -3089,38 +3275,6 @@ riscv_find_default_target_description (const struct gdbarch_info info)
   return riscv_lookup_target_description (features);
 }
 
-/* All of the registers in REG_SET are checked for in FEATURE, TDESC_DATA
-   is updated with the register numbers for each register as listed in
-   REG_SET.  If any register marked as required in REG_SET is not found in
-   FEATURE then this function returns false, otherwise, it returns true.  */
-
-static bool
-riscv_check_tdesc_feature (struct tdesc_arch_data *tdesc_data,
-			   const struct tdesc_feature *main_feature,
-			   const struct tdesc_feature *csr_feature,
-			   const struct riscv_register_feature *reg_set,
-			   std::vector<riscv_pending_register_alias> *aliases)
-{
-  for (const auto &reg : reg_set->registers)
-    {
-      bool found = reg.check (tdesc_data, main_feature, reg_set, aliases);
-
-      if (!found && reg.required != RISCV_REG_OPTIONAL)
-	{
-	  if (reg.required == RISCV_REG_REQUIRED_MAYBE_CSR
-	      && csr_feature != nullptr)
-	    {
-	      gdb_assert (main_feature != csr_feature);
-	      found = reg.check (tdesc_data, csr_feature,  reg_set, aliases);
-	    }
-	  if (!found)
-	    return false;
-	}
-    }
-
-  return true;
-}
-
 /* Add all the expected register sets into GDBARCH.  */
 
 static void
@@ -3229,7 +3383,7 @@ riscv_tdesc_unknown_reg (struct gdbarch *gdbarch, tdesc_feature *feature,
 
      To prevent these duplicates showing up in any of the register list,
      record their register numbers here.  */
-  if (strcmp (tdesc_feature_name (feature), riscv_freg_feature.name) == 0)
+  if (strcmp (tdesc_feature_name (feature), riscv_freg_feature.name ()) == 0)
     {
       struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
       int *regnum_ptr = nullptr;
@@ -3260,7 +3414,7 @@ riscv_tdesc_unknown_reg (struct gdbarch *gdbarch, tdesc_feature *feature,
   /* Any unknown registers in the CSR feature are recorded within a single
      block so we can easily identify these registers when making choices
      about register groups in riscv_register_reggroup_p.  */
-  if (strcmp (tdesc_feature_name (feature), riscv_csr_feature.name) == 0)
+  if (strcmp (tdesc_feature_name (feature), riscv_csr_feature.name ()) == 0)
     {
       struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
       if (tdep->unknown_csrs_first_regnum == -1)
@@ -3306,95 +3460,22 @@ riscv_gdbarch_init (struct gdbarch_info info,
   /* Ensure we always have a target description.  */
   if (!tdesc_has_registers (tdesc))
     tdesc = riscv_find_default_target_description (info);
-  gdb_assert (tdesc);
+  gdb_assert (tdesc != nullptr);
 
   if (riscv_debug_gdbarch)
     fprintf_unfiltered (gdb_stdlog, "Have got a target description\n");
 
-  const struct tdesc_feature *feature_cpu
-    = tdesc_find_feature (tdesc, riscv_xreg_feature.name);
-  const struct tdesc_feature *feature_fpu
-    = tdesc_find_feature (tdesc, riscv_freg_feature.name);
-  const struct tdesc_feature *feature_virtual
-    = tdesc_find_feature (tdesc, riscv_virtual_feature.name);
-  const struct tdesc_feature *feature_csr
-    = tdesc_find_feature (tdesc, riscv_csr_feature.name);
-
-  if (feature_cpu == NULL)
-    return NULL;
-
   tdesc_arch_data_up tdesc_data = tdesc_data_alloc ();
   std::vector<riscv_pending_register_alias> pending_aliases;
 
-  bool valid_p = riscv_check_tdesc_feature (tdesc_data.get (),
-					    feature_cpu, feature_csr,
-					    &riscv_xreg_feature,
-					    &pending_aliases);
-  if (valid_p)
-    {
-      /* Check that all of the core cpu registers have the same bitsize.  */
-      int xlen_bitsize = tdesc_register_bitsize (feature_cpu, "pc");
-
-      for (auto &tdesc_reg : feature_cpu->registers)
-	valid_p &= (tdesc_reg->bitsize == xlen_bitsize);
-
-      if (riscv_debug_gdbarch)
-	fprintf_filtered
-	  (gdb_stdlog,
-	   "From target-description, xlen = %d\n", xlen_bitsize);
-
-      features.xlen = (xlen_bitsize / 8);
-    }
-
-  if (feature_fpu != NULL)
-    {
-      valid_p &= riscv_check_tdesc_feature (tdesc_data.get (), feature_fpu,
-					    feature_csr,
-					    &riscv_freg_feature,
-					    &pending_aliases);
-
-      /* Search for the first floating point register (by any alias), to
-	 determine the bitsize.  */
-      int bitsize = -1;
-      const auto &fp0 = riscv_freg_feature.registers[0];
-
-      for (const char *name : fp0.names)
-	{
-	  if (tdesc_unnumbered_register (feature_fpu, name))
-	    {
-	      bitsize = tdesc_register_bitsize (feature_fpu, name);
-	      break;
-	    }
-	}
-
-      gdb_assert (bitsize != -1);
-      features.flen = (bitsize / 8);
-
-      if (riscv_debug_gdbarch)
-	fprintf_filtered
-	  (gdb_stdlog,
-	   "From target-description, flen = %d\n", bitsize);
-    }
-  else
-    {
-      features.flen = 0;
-
-      if (riscv_debug_gdbarch)
-	fprintf_filtered
-	  (gdb_stdlog,
-	   "No FPU in target-description, assume soft-float ABI\n");
-    }
-
-  if (feature_virtual)
-    riscv_check_tdesc_feature (tdesc_data.get (), feature_virtual, feature_csr,
-			       &riscv_virtual_feature,
-			       &pending_aliases);
-
-  if (feature_csr)
-    riscv_check_tdesc_feature (tdesc_data.get (), feature_csr, nullptr,
-			       &riscv_csr_feature,
-			       &pending_aliases);
-
+  bool valid_p = (riscv_xreg_feature.check (tdesc, tdesc_data.get (),
+					    &pending_aliases, &features)
+		  && riscv_freg_feature.check (tdesc, tdesc_data.get (),
+					       &pending_aliases, &features)
+		  && riscv_virtual_feature.check (tdesc, tdesc_data.get (),
+						  &pending_aliases, &features)
+		  && riscv_csr_feature.check (tdesc, tdesc_data.get (),
+					      &pending_aliases, &features));
   if (!valid_p)
     {
       if (riscv_debug_gdbarch)
@@ -3407,10 +3488,17 @@ riscv_gdbarch_init (struct gdbarch_info info,
      providing.  */
   struct riscv_gdbarch_features abi_features
     = riscv_features_from_gdbarch_info (info);
+
+  /* If the ABI_FEATURES xlen is 0 then this indicates we got no useful abi
+     features from the INFO object.  In this case we just treat the
+     hardware features as defining the abi.  */
+  if (abi_features.xlen == 0)
+    abi_features = features;
+
   /* 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)
+  if (abi_features.xlen != features.xlen)
     error (_("bfd requires xlen %d, but target has xlen %d"),
 	    abi_features.xlen, features.xlen);
   /* We do support running binaries compiled for 32-bit float on targets
@@ -3420,12 +3508,6 @@ riscv_gdbarch_init (struct gdbarch_info info,
     error (_("bfd requires flen %d, but target has flen %d"),
 	    abi_features.flen, features.flen);
 
-  /* 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);
        arches != NULL;
@@ -3697,7 +3779,6 @@ void _initialize_riscv_tdep ();
 void
 _initialize_riscv_tdep ()
 {
-  riscv_create_csr_aliases ();
   riscv_init_reggroups ();
 
   gdbarch_register (bfd_arch_riscv, riscv_gdbarch_init, NULL);
diff --git a/gdb/riscv-tdep.h b/gdb/riscv-tdep.h
index 5bd3314d450..42939105b08 100644
--- a/gdb/riscv-tdep.h
+++ b/gdb/riscv-tdep.h
@@ -128,6 +128,11 @@ extern int riscv_abi_xlen (struct gdbarch *gdbarch);
    with RISCV_ISA_FLEN.  */
 extern int riscv_abi_flen (struct gdbarch *gdbarch);
 
+/* Return true if GDBARCH is using the embedded x-regs abi, that is the
+   target only has 16 x-registers, which includes a reduced number of
+   argument registers.  */
+extern bool riscv_abi_embedded (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.25.4

Re: [PATCH] gdb/riscv: rewrite target description validation, add rv32e support

[Jim Wilson]

On Thu, Nov 26, 2020 at 5:29 AM Andrew Burgess <andrew.burgess@embecosm.com>
wrote:

> This commit started as adding rv32e support to gdb.  The rv32e
> architecture is a cut-down rv32i, it only has 16 x-registers compared
> to the usual 32, and an rv32e target should not have any floating
> point registers.
>

The original definition of rv32e said no FP.  But this was changed in a
later ISA version, and rv32e is now allowed to have FP.  I don't see
anything obvious in the patch that prevents rv32e from having FP regs
though, so it seems OK.

However, the ilp32e ABI was defined with 32-bit alignment, plus the fact
that arguments get passed in regs without alignment padding, and the fact
that 64-bit FP still requires 64-bit alignment, means that there are
multiple ways that we can end up with unaligned data on the stack if we
allow rv32ed.  So at present, neither gcc nor llvm have support for rv32e
plus FP.  In theory, rv32ef should work fine, but no one has asked for it
yet, so we haven't bothered to implement it yet.  There is an EABI group
working on a new embedded ABI, and when that is done, the stack alignment
problem will be fixed and we will probably have to add rv32e f and d
support to the GNU toolchain.  That could be a year or two away though.

Jim

Re: [PATCH] gdb/riscv: rewrite target description validation, add rv32e support

[Andrew Burgess]

* Jim Wilson <jimw@sifive.com> [2020-11-27 15:09:51 -0800]:

> On Thu, Nov 26, 2020 at 5:29 AM Andrew Burgess <andrew.burgess@embecosm.com>
> wrote:
> 
> > This commit started as adding rv32e support to gdb.  The rv32e
> > architecture is a cut-down rv32i, it only has 16 x-registers compared
> > to the usual 32, and an rv32e target should not have any floating
> > point registers.
> >
> 
> The original definition of rv32e said no FP.  But this was changed in a
> later ISA version, and rv32e is now allowed to have FP.  I don't see
> anything obvious in the patch that prevents rv32e from having FP regs
> though, so it seems OK.
> 
> However, the ilp32e ABI was defined with 32-bit alignment, plus the fact
> that arguments get passed in regs without alignment padding, and the fact
> that 64-bit FP still requires 64-bit alignment, means that there are
> multiple ways that we can end up with unaligned data on the stack if we
> allow rv32ed.  So at present, neither gcc nor llvm have support for rv32e
> plus FP.  In theory, rv32ef should work fine, but no one has asked for it
> yet, so we haven't bothered to implement it yet.  There is an EABI group
> working on a new embedded ABI, and when that is done, the stack alignment
> problem will be fixed and we will probably have to add rv32e f and d
> support to the GNU toolchain.  That could be a year or two away
> though.

Thanks for the feedback.

You are correct, I have not done anything to prevent use of F/D
extensions with rv32e.

I only have an rv32e toolchain and simulator so I've not done any
testing of rv32ef (never mind rv32ed).  I guess as the new ABI is
standardised and toolchain support starts to appear we can expose this
stuff to more testing.  I'm sure the inferior calling code will have
issues in that case.

I'm going to go ahead any merge this, if you have any follow up
thoughts, or see any issues do just let me know and I'll get them
addressed.

Thanks,
Andrew