gdb/riscv: Record information about unknown tdesc registers

Making use of the previous commit, record information about unknown
registers in the target description, and use this to resolve two
issues.

1. Some targets (QEMU) are reporting three register fflags, frm, and
   fcsr, twice, once in the FPU feature, and once in the CSR feature.
   GDB does create two registers with identical names, but this
   is (sort of) fine, we only ever use the first one, and as both
   registers access the same target state things basically work OK.
   The only real problem is that the register names show up twice in
   'info registers all' output.

   In this commit we spot the duplicates of these registers and then
   return NULL when asked for the name of these registers.  This
   causes GDB to hide these registers from the user, fixing this
   problem.

2. Some targets (QEMU) advertise CSRs that GDB then can't read.  The
   problem is these targets also say these CSRs are part of the
   save/restore register groups.

   This means that before an inferior call GDB tries to save all of
   these CSRs, and a failure to read one causes the inferior call to
   be abandoned.

   We already work around this issue to some degree, known CSRs are
   removed from the save/restore groups, despite what the target might
   say.  However, any unknown CSRs are (currently) not removed in this
   way.

   After this commit we keep a log of the register numbers for all
   unknown CSRs, then when asked about the register groups, we
   override the group information for unknown CSRs, removing them from
   the save and restore groups.

gdb/ChangeLog:

	* riscv-tdep.c (riscv_register_name): Return NULL for duplicate
	fflags, frm, and fcsr registers.
	(riscv_register_reggroup_p): Remove unknown CSRs from save and
	restore groups.
	(riscv_tdesc_unknown_reg): New function.
	(riscv_gdbarch_init): Pass riscv_tdesc_unknown_reg to
	tdesc_use_registers.
	* riscv-tdep.h (struct gdbarch_tdep): Add
	unknown_csrs_first_regnum, unknown_csrs_count,
	duplicate_fflags_regnum, duplicate_frm_regnum, and
	duplicate_fcsr_regnum fields.

gdb/testsuite/ChangeLog:

	* gdb.arch/riscv-tdesc-regs.exp: Extend test case.

5 files changed, 145 insertions, 3 deletions

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 771382b..f3ede94 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,19 @@
 2020-06-25  Andrew Burgess  <andrew.burgess@embecosm.com>
 
+	* riscv-tdep.c (riscv_register_name): Return NULL for duplicate
+	fflags, frm, and fcsr registers.
+	(riscv_register_reggroup_p): Remove unknown CSRs from save and
+	restore groups.
+	(riscv_tdesc_unknown_reg): New function.
+	(riscv_gdbarch_init): Pass riscv_tdesc_unknown_reg to
+	tdesc_use_registers.
+	* riscv-tdep.h (struct gdbarch_tdep): Add
+	unknown_csrs_first_regnum, unknown_csrs_count,
+	duplicate_fflags_regnum, duplicate_frm_regnum, and
+	duplicate_fcsr_regnum fields.
+
+2020-06-25  Andrew Burgess  <andrew.burgess@embecosm.com>
+
 	* target-descriptions.c (tdesc_use_registers): Add new parameter a
 	callback, use the callback (when not null) to help number unknown
 	registers.
diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index b1dd79f..cfc45d1 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -617,6 +617,22 @@ riscv_register_name (struct gdbarch *gdbarch, int regnum)
         return NULL;
     }
 
+  /* Some targets (QEMU) are reporting these three registers twice, once
+     in the FPU feature, and once in the CSR feature.  Both of these read
+     the same underlying state inside the target, but naming the register
+     twice in the target description results in GDB having two registers
+     with the same name, only one of which can ever be accessed, but both
+     will show up in 'info register all'.  Unless, we identify the
+     duplicate copies of these registers (in riscv_tdesc_unknown_reg) and
+     then hide the registers here by giving them no name.  */
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+  if (tdep->duplicate_fflags_regnum == regnum)
+    return NULL;
+  if (tdep->duplicate_frm_regnum == regnum)
+    return NULL;
+  if (tdep->duplicate_fcsr_regnum == regnum)
+    return NULL;
+
   /* The remaining registers are different.  For all other registers on the
      machine we prefer to see the names that the target description
      provides.  This is particularly important for CSRs which might be
@@ -968,6 +984,19 @@ riscv_register_reggroup_p (struct gdbarch  *gdbarch, int regnum,
 
   if (regnum > RISCV_LAST_REGNUM)
     {
+      /* Any extra registers from the CSR tdesc_feature (identified in
+	 riscv_tdesc_unknown_reg) are removed from the save/restore groups
+	 as some targets (QEMU) report CSRs which then can't be read.  */
+      struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+      if ((reggroup == restore_reggroup || reggroup == save_reggroup)
+	  && regnum >= tdep->unknown_csrs_first_regnum
+	  && regnum < (tdep->unknown_csrs_first_regnum
+		       + tdep->unknown_csrs_count))
+	return 0;
+
+      /* This is some other unknown register from the target description.
+	 In this case we trust whatever the target description says about
+	 which groups this register should be in.  */
       int ret = tdesc_register_in_reggroup_p (gdbarch, regnum, reggroup);
       if (ret != -1)
         return ret;
@@ -3166,6 +3195,85 @@ riscv_gcc_target_options (struct gdbarch *gdbarch)
   return target_options;
 }
 
+/* Call back from tdesc_use_registers, called for each unknown register
+   found in the target description.
+
+   See target-description.h (typedef tdesc_unknown_register_ftype) for a
+   discussion of the arguments and return values.  */
+
+static int
+riscv_tdesc_unknown_reg (struct gdbarch *gdbarch, tdesc_feature *feature,
+			 const char *reg_name, int possible_regnum)
+{
+  /* At one point in time GDB had an incorrect default target description
+     that duplicated the fflags, frm, and fcsr registers in both the FPU
+     and CSR register sets.
+
+     Some targets (QEMU) copied these target descriptions into their source
+     tree, and so we're currently stuck working with some targets that
+     declare the same registers twice.
+
+     There's not much we can do about this any more.  Assuming the target
+     will direct a request for either register number to the correct
+     underlying hardware register then it doesn't matter which one GDB
+     uses, so long as we (GDB) are consistent (so that we don't end up with
+     invalid cache misses).
+
+     As we always scan the FPU registers first, then the CSRs, if the
+     target has included the offending registers in both sets then we will
+     always see the FPU copies here, as the CSR versions will replace them
+     in the register list.
+
+     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)
+    {
+      struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+      int *regnum_ptr = nullptr;
+
+      if (strcmp (reg_name, "fflags") == 0)
+	regnum_ptr = &tdep->duplicate_fflags_regnum;
+      else if (strcmp (reg_name, "frm") == 0)
+	regnum_ptr = &tdep->duplicate_frm_regnum;
+      else if (strcmp (reg_name, "fcsr") == 0)
+	regnum_ptr = &tdep->duplicate_fcsr_regnum;
+
+      if (regnum_ptr != nullptr)
+	{
+	  /* This means the register appears more than twice in the target
+	     description.  Just let GDB add this as another register.
+	     We'll have duplicates in the register name list, but there's
+	     not much more we can do.  */
+	  if (*regnum_ptr != -1)
+	    return -1;
+
+	  /* Record the number assigned to this register, then return the
+	     number (so it actually gets assigned to this register).  */
+	  *regnum_ptr = possible_regnum;
+	  return possible_regnum;
+	}
+    }
+
+  /* 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)
+    {
+      struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+      if (tdep->unknown_csrs_first_regnum == -1)
+	tdep->unknown_csrs_first_regnum = possible_regnum;
+      gdb_assert (tdep->unknown_csrs_first_regnum
+		  + tdep->unknown_csrs_count == possible_regnum);
+      tdep->unknown_csrs_count++;
+      return possible_regnum;
+    }
+
+  /* Some other unknown register.  Don't assign this a number now, it will
+     be assigned a number automatically later by the target description
+     handling code.  */
+  return -1;
+}
+
 /* Implement the gnu_triplet_regexp method.  A single compiler supports both
    32-bit and 64-bit code, and may be named riscv32 or riscv64 or (not
    recommended) riscv.  */
@@ -3403,7 +3511,7 @@ riscv_gdbarch_init (struct gdbarch_info info,
   set_gdbarch_print_registers_info (gdbarch, riscv_print_registers_info);
 
   /* Finalise the target description registers.  */
-  tdesc_use_registers (gdbarch, tdesc, tdesc_data);
+  tdesc_use_registers (gdbarch, tdesc, tdesc_data, riscv_tdesc_unknown_reg);
 
   /* Override the register type callback setup by the target description
      mechanism.  This allows us to provide special type for floating point
diff --git a/gdb/riscv-tdep.h b/gdb/riscv-tdep.h
index e415fb4..0ff555b 100644
--- a/gdb/riscv-tdep.h
+++ b/gdb/riscv-tdep.h
@@ -79,6 +79,21 @@ struct gdbarch_tdep
 
   /* ISA-specific data types.  */
   struct type *riscv_fpreg_d_type = nullptr;
+
+  /* Use for tracking unknown CSRs in the target description.
+     UNKNOWN_CSRS_FIRST_REGNUM is the number assigned to the first unknown
+     CSR.  All other unknown CSRs will be assigned sequential numbers after
+     this, with UNKNOWN_CSRS_COUNT being the total number of unknown CSRs.  */
+  int unknown_csrs_first_regnum = -1;
+  int unknown_csrs_count = 0;
+
+  /* Some targets (QEMU) are reporting three registers twice in the target
+     description they send.  These three register numbers, when not set to
+     -1, are for the duplicate copies of these registers.  */
+  int duplicate_fflags_regnum = -1;
+  int duplicate_frm_regnum = -1;
+  int duplicate_fcsr_regnum = -1;
+
 };
 
 
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index b02db09..0090327 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,5 +1,9 @@
 2020-06-25  Andrew Burgess  <andrew.burgess@embecosm.com>
 
+	* gdb.arch/riscv-tdesc-regs.exp: Extend test case.
+
+2020-06-25  Andrew Burgess  <andrew.burgess@embecosm.com>
+
 	* gdb.arch/riscv-tdesc-loading-01.xml: New file.
 	* gdb.arch/riscv-tdesc-loading-02.xml: New file.
 	* gdb.arch/riscv-tdesc-loading-03.xml: New file.
diff --git a/gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp b/gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp
index 63ac8fb..9feddba 100644
--- a/gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp
+++ b/gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp
@@ -100,13 +100,14 @@ foreach rgroup {all save restore} {
 	}
     }
 
-    foreach reg {dscratch} {
+    foreach reg {fflags frm fcsr unknown_csr dscratch} {
 	if { [info exists reg_counts($reg) ] } {
 	    set count $reg_counts($reg)
 	} else {
 	    set count 0
 	}
-	if {$reg == "dscratch" && $rgroup != "all"} {
+	if {($reg == "unknown_csr" || $reg == "dscratch") \
+		&& $rgroup != "all"} {
 	    gdb_assert {$count == 0} \
 		"register $reg not seen in reggroup $rgroup"
 	} else {