[PATCH 3/8] gdb/riscv: Take CSR names from target description

[Andrew Burgess <andrew.burgess@embecosm.com>]

First, consider the RISC-V register $x1.  This register has an alias
$ra.  When GDB processes an incoming target description we allow the
target to use either register name to describe the target.

However, within GDB's UI we want to use the $ra alias in preference to
the $x1 architecture name.

To achieve this GDB overrides the tdesc_register_name callback with
riscv_register_name.  In riscv_register_name we ensure that we always
return the preferred name, so in this case "ra".

To ensure the user can still access the register as $x1 if they want
to, when in riscv_check_tdesc_feature we spot that the target has
supplied the register, we add aliases for every name except the
preferred one, so in this case we add the alias "x1".

This scheme seems to work quite well, the targets have the flexibility
to be architecture focused if they wish (using x0 - x31) while GDB is
still using the ABI names ra, sp, gp, etc.

When this code was originally added there was an attempt made to
include the CSRs in the same scheme.  At the time the CSRs only had
two names, one pulled from riscv-opc.h, and one generated in GDB that
had the pattern csr%d.

The idea was that if the remote targets description described the CSRs
as csr%d then GDB would rename these back to the real CSR name.  This
code was only included because if followed the same pattern as the
x-regs and f-regs, not because I was actually aware of any target that
did this.

However, recent changes to add additional CSR aliases has made me
rethink the position here.

Lets consider the CSR $dscratch0.  This register has an alias
'csr1970' (1970 is 0x7b2, which is the offset of the CSR register into
the CSR address space).  However, this register was originally called
just 'dscratch', and so, after recent commits, this register also has
the alias 'dscratch'.

As the riscv-opc.h file calls this register 'dscratch0' GDB's
preferred name for this register is 'dscratch0'.

So, if the remote target description includes the register
'dscratch0', then GDB will add the aliases 'dscratch', and 'csr1970'.
In the UI GDB will describe the register as 'dscratch0', and all it
good.

The problem I see in this case is where the target describes the
register as 'dscratch'.  In this case GDB will still spot the register
and add the aliases 'dscratch', and 'csr1970', GDB will then give the
register the preferred name 'dscratch0'.

I don't like this.  For the CSRs I think that we should stick with the
naming scheme offered by the remote target description.  As the RISC-V
specification evolves and CSR register names evolve, insisting on
referring to registers by the most up to date name makes it harder for
a target to provide a consistent target description for an older
version of the RISC-V architecture spec.

In this precise case the target offers 'dscratch', which is from an
older version of the RISC-V specification, the newer version of the
spec has two registers 'dscratch0' and 'dscratch1'.  If we insist on
using 'dscratch0' it is then a little "weird" (or seems so to me) when
'dscratch1' is missing.

This patch makes a distinction between the x and f registers and the
other register sets.  For x and f we still make use of the renaming
scheme, forcing GDB to prefer the ABI name.  But after this patch the
CSR register group, and also the virtual register group, will always
prefer to use the name given in the target description, adding other
names as aliases, but not making any other name the preferred name.

gdb/ChangeLog:

	* riscv-tdep.c (struct riscv_register_feature::register_info): Fix
	whitespace error for declaration of names member variable.
	(struct riscv_register_feature): Add new prefer_first_name member
	variable, and fix whitespace error in declaration of registers.
	(riscv_xreg_feature): Initialize prefer_first_name field.
	(riscv_freg_feature): Likewise.
	(riscv_virtual_feature): Likewise.
	(riscv_csr_feature): Likewise.
	(riscv_register_name): Expand on comments.  Remove register name
	modifications for CSR and virtual registers.

gdb/testsuite/ChangeLog:

	* gdb.arch/riscv-tdesc-regs.exp: Extend test case.
---
 gdb/ChangeLog                               | 13 ++++
 gdb/riscv-tdep.c                            | 75 +++++++++++++--------
 gdb/testsuite/ChangeLog                     |  4 ++
 gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp | 39 ++++++++++-
 4 files changed, 101 insertions(+), 30 deletions(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index e394263a591..b136cbdfbf4 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -125,6 +125,20 @@ struct riscv_register_feature
      within the target description.  */
   const char *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.
+
+     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;
+
   /* List of all the registers that we expect that we might find in this
      register set.  */
   std::vector<struct register_info> registers;
@@ -134,7 +148,7 @@ struct riscv_register_feature
 
 static const struct riscv_register_feature riscv_xreg_feature =
 {
- "org.gnu.gdb.riscv.cpu",
+ "org.gnu.gdb.riscv.cpu", true,
  {
    { RISCV_ZERO_REGNUM + 0, { "zero", "x0" }, true },
    { RISCV_ZERO_REGNUM + 1, { "ra", "x1" }, true },
@@ -176,7 +190,7 @@ static const struct riscv_register_feature riscv_xreg_feature =
 
 static const struct riscv_register_feature riscv_freg_feature =
 {
- "org.gnu.gdb.riscv.fpu",
+ "org.gnu.gdb.riscv.fpu", true,
  {
    { RISCV_FIRST_FP_REGNUM + 0, { "ft0", "f0" }, true },
    { RISCV_FIRST_FP_REGNUM + 1, { "ft1", "f1" }, true },
@@ -226,7 +240,7 @@ static const struct riscv_register_feature riscv_freg_feature =
 
 static const struct riscv_register_feature riscv_virtual_feature =
 {
- "org.gnu.gdb.riscv.virtual",
+ "org.gnu.gdb.riscv.virtual", false,
  {
    { RISCV_PRIV_REGNUM, { "priv" }, false }
  }
@@ -238,7 +252,7 @@ static const struct riscv_register_feature riscv_virtual_feature =
 
 static struct riscv_register_feature riscv_csr_feature =
 {
- "org.gnu.gdb.riscv.csr",
+ "org.gnu.gdb.riscv.csr", false,
  {
 #define DECLARE_CSR(NAME,VALUE,CLASS,DEFINE_VER,ABORT_VER) \
   { RISCV_ ## VALUE ## _REGNUM, { # NAME }, false },
@@ -472,7 +486,7 @@ value_of_riscv_user_reg (struct frame_info *frame, const void *baton)
 
 /* Implement the register_name gdbarch method.  This is used instead of
    the function supplied by calling TDESC_USE_REGISTERS so that we can
-   ensure the preferred names are offered.  */
+   ensure the preferred names are offered for x-regs and f-regs.  */
 
 static const char *
 riscv_register_name (struct gdbarch *gdbarch, int regnum)
@@ -484,12 +498,18 @@ riscv_register_name (struct gdbarch *gdbarch, int regnum)
   if (name == NULL || name[0] == '\0')
     return NULL;
 
+  /* We want GDB to use the ABI names for registers even if the target
+     gives us a target description with the architectural name.  For
+     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];
     }
 
+  /* 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))
@@ -502,28 +522,18 @@ riscv_register_name (struct gdbarch *gdbarch, int regnum)
         return NULL;
     }
 
-  /* Check that there's no gap between the set of registers handled above,
-     and the set of registers handled next.  */
-  gdb_assert ((RISCV_LAST_FP_REGNUM + 1) == RISCV_FIRST_CSR_REGNUM);
-
-  if (regnum >= RISCV_FIRST_CSR_REGNUM && regnum <= RISCV_LAST_CSR_REGNUM)
-    {
-#define DECLARE_CSR(NAME,VALUE,CLASS,DEFINE_VER,ABORT_VER) \
-      case RISCV_ ## VALUE ## _REGNUM: return # NAME;
-
-      switch (regnum)
-	{
-#include "opcode/riscv-opc.h"
-	}
-#undef DECLARE_CSR
-    }
+  /* 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
+     renamed over time.  If GDB keeps track of the "latest" name, but a
+     particular target provides an older name then we don't want to force
+     users to see the newer name in register output.
 
-  if (regnum == RISCV_PRIV_REGNUM)
-    return "priv";
+     The other case that reaches here are any registers that the target
+     provided that GDB is completely unaware of.  For these we have no
+     choice but to accept the target description name.
 
-  /* It is possible that that the target provides some registers that GDB
-     is unaware of, in that case just return the NAME from the target
-     description.  */
+     Just accept whatever name TDESC_REGISTER_NAME returned.  */
   return name;
 }
 
@@ -3003,8 +3013,8 @@ riscv_check_tdesc_feature (struct tdesc_arch_data *tdesc_data,
 
       for (const char *name : reg.names)
 	{
-	  found =
-	    tdesc_numbered_register (feature, tdesc_data, reg.regnum, name);
+	  found = tdesc_numbered_register (feature, tdesc_data, reg.regnum,
+					   name);
 
 	  if (found)
             {
@@ -3012,8 +3022,15 @@ riscv_check_tdesc_feature (struct tdesc_arch_data *tdesc_data,
                  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.  */
-              for (int i = 0; i < reg.names.size (); ++i)
-		aliases->emplace_back (reg.names[i], (void *)&reg.regnum);
+	      bool prefer_first_name = reg_set->prefer_first_name;
+	      int start_index = prefer_first_name ? 1 : 0;
+	      for (int i = start_index; i < reg.names.size (); ++i)
+                {
+		  const char *alias = reg.names[i];
+                  if (alias == name && !prefer_first_name)
+		    continue;
+		  aliases->emplace_back (alias, (void *)&reg.regnum);
+                }
               break;
             }
 	}
diff --git a/gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp b/gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp
index 46e64d62a2b..63ac8fb7abc 100644
--- a/gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp
+++ b/gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp
@@ -14,7 +14,8 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 # Various tests to check which register names are available after
-# loading a new target description file.
+# loading a new target description file, and which registers show up
+# in the output of the 'info registers' command.
 
 if {![istarget "riscv*-*-*"]} {
     verbose "Skipping ${gdb_test_file_name}."
@@ -79,3 +80,39 @@ gdb_test "info registers \$csr0" "Invalid register `csr0'"
 gdb_test "info registers \$dscratch0" "dscratch0\[ \t\]+.*"
 gdb_test "info registers \$dscratch" "dscratch\[ \t\]+.*"
 
+foreach rgroup {all save restore} {
+    # Now use 'info registers all' to see how many times the floating
+    # point status registers show up in the output.
+    array set reg_counts {}
+    set test "info registers $rgroup"
+    gdb_test_multiple $test $test {
+	-re ".*info registers all\r\n" {
+	    verbose -log "Skip to first register"
+	    exp_continue
+	}
+	-re "^(\[^ \t\]+)\[ \t\]+\[^\r\n\]+\r\n" {
+	    set reg $expect_out(1,string)
+	    incr reg_counts($reg)
+	exp_continue
+	}
+	-re "^$gdb_prompt $" {
+	    # Done.
+	}
+    }
+
+    foreach reg {dscratch} {
+	if { [info exists reg_counts($reg) ] } {
+	    set count $reg_counts($reg)
+	} else {
+	    set count 0
+	}
+	if {$reg == "dscratch" && $rgroup != "all"} {
+	    gdb_assert {$count == 0} \
+		"register $reg not seen in reggroup $rgroup"
+	} else {
+	    gdb_assert {$count == 1} \
+		"register $reg seen once in reggroup $rgroup"
+	}
+    }
+    array unset reg_counts
+}
-- 
2.25.4