[PATCH 0/2] RISCV CSR register groups and aliases

[PATCH 0/2] RISCV CSR register groups and aliases

[Andrew Burgess]

Some changes to how register groups and aliases are managed for RISC-V
CSRs.

---

Andrew Burgess (2):
  gdb/riscv: place unknown csrs into the correct register groups
  gdb/riscv: remove csr aliases created with DECLARE_CSR_ALIAS

 gdb/ChangeLog                               | 12 ++++++
 gdb/riscv-tdep.c                            | 44 +++++++++++++--------
 gdb/testsuite/ChangeLog                     |  9 +++++
 gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp | 41 +++++++++++++------
 4 files changed, 77 insertions(+), 29 deletions(-)

-- 
2.25.4

[PATCH 1/2] gdb/riscv: place unknown csrs into the correct register groups

[Andrew Burgess]

Unknown riscv CSRs should not be in the 'general' group, but should be
in the system and csr register groups.

To see this in action connect to QEMU, this target advertises two
registers dscratch and mucounteren which are unknown to GDB (these are
legacy CSRs).  Before this commit these registers would show up in the
output of:

  (gdb) info registers
  ....
  dscratch       Could not fetch register "dscratch"; remote failure reply 'E14'
  mucounteren    Could not fetch register "mucounteren"; remote failure reply 'E14'

Ignore the errors, this is just a QEMU annoyance, it advertises these
CSRs, but doesn't actually let GDB read them.  These registers don't
show up in the output of either:

  (gdb) info registers csr
  (gdb) info registers system

After this commit this situation is reveresed, which makes more sense
to me.

gdb/ChangeLog:

	* riscv-tdep.c (riscv_is_unknown_csr): New function,
	implementation moved from riscv_register_reggroup_p.
	(riscv_register_reggroup_p): Update group handling for unknown
	CSRs.

gdb/testsuite/ChangeLog:

	* gdb.arch/riscv-tdesc-regs.exp (get_expected_result): New proc,
	update test to use this.
---
 gdb/ChangeLog                               |  7 ++++
 gdb/riscv-tdep.c                            | 34 ++++++++++++++----
 gdb/testsuite/ChangeLog                     |  5 +++
 gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp | 39 ++++++++++++++++-----
 4 files changed, 69 insertions(+), 16 deletions(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 4e255056863..437e8f847ae 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -969,6 +969,18 @@ riscv_is_regnum_a_named_csr (int regnum)
     }
 }
 
+/* Return true if REGNUM is an unknown CSR identified in
+   riscv_tdesc_unknown_reg for GDBARCH.  */
+
+static bool
+riscv_is_unknown_csr (struct gdbarch *gdbarch, int regnum)
+{
+  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
+  return (regnum >= tdep->unknown_csrs_first_regnum
+	  && regnum < (tdep->unknown_csrs_first_regnum
+		       + tdep->unknown_csrs_count));
+}
+
 /* Implement the register_reggroup_p gdbarch method.  Is REGNUM a member
    of REGGROUP?  */
 
@@ -986,13 +998,21 @@ riscv_register_reggroup_p (struct gdbarch  *gdbarch, int 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;
+	 as some targets (QEMU) report CSRs which then can't be read and
+	 having unreadable registers in the save/restore group breaks
+	 things like inferior calls.
+
+	 The unknown CSRs are also removed from the general group, and
+	 added into both the csr and system group.  This is inline with the
+	 known CSRs (see below).  */
+      if (riscv_is_unknown_csr (gdbarch, regnum))
+	{
+	  if (reggroup == restore_reggroup || reggroup == save_reggroup
+	       || reggroup == general_reggroup)
+	    return 0;
+	  else if (reggroup == system_reggroup || reggroup == csr_reggroup)
+	    return 1;
+	}
 
       /* This is some other unknown register from the target description.
 	 In this case we trust whatever the target description says about
diff --git a/gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp b/gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp
index 1be32e0e8a1..e35d6181b7f 100644
--- a/gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp
+++ b/gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp
@@ -80,7 +80,32 @@ gdb_test "info registers \$csr0" "Invalid register `csr0'"
 gdb_test "info registers \$dscratch0" "dscratch0\[ \t\]+.*"
 gdb_test "info registers \$dscratch" "dscratch\[ \t\]+.*"
 
-foreach rgroup {x_all all save restore} {
+# Return the number of times REGISTER should appear in GROUP, this
+# will either be 0 or 1.
+proc get_expected_result { register group } {
+
+    # Everything should appear once in the 'all' group.
+    if { $group == "all" || $group == "x_all" } {
+	return 1
+    }
+
+    if { $group == "save" || $group == "restore" } {
+	# Everything is in the save/restore groups except these two.
+	if { $register == "unknown_csr" || $register == "dscratch" } {
+	    return 0
+	}
+	return 1
+    }
+
+    if { $group == "system" || $group == "csr" } {
+	# All the registers we check should be in these groups.
+	return 1
+    }
+
+    return 0
+}
+
+foreach rgroup {x_all all save restore general system csr} {
     # Now use 'info registers all' to see how many times the floating
     # point status registers show up in the output.
     array set reg_counts {}
@@ -110,14 +135,10 @@ foreach rgroup {x_all all save restore} {
 	} else {
 	    set count 0
 	}
-	if {($reg == "unknown_csr" || $reg == "dscratch") \
-		&& $rgroup != "all" && $rgroup != "x_all"} {
-	    gdb_assert {$count == 0} \
-		"register $reg not seen in reggroup $rgroup"
-	} else {
-	    gdb_assert {$count == 1} \
-		"register $reg seen once in reggroup $rgroup"
-	}
+
+	set expected_count [ get_expected_result $reg $rgroup ]
+	gdb_assert {$count == $expected_count} \
+	    "register $reg seen in reggroup $rgroup $expected_count times"
     }
     array unset reg_counts
 }
-- 
2.25.4

[PATCH 2/2] gdb/riscv: remove csr aliases created with DECLARE_CSR_ALIAS

[Andrew Burgess]

In this commit:

  commit 767a879e31ce31179e6135c2f991f670a35709fa
  Date:   Tue Jun 9 17:38:30 2020 +0100

      gdb/riscv: Improved register alias name creation

RISC-V GDB was changed to make use of the DECLARE_CSR_ALIAS macro to
define register aliases for some CSRs.  Actually, only one alias was
created 'dscratch' as an alias for 'dscratch0'.  All of the other
DECLARE_CSR_ALIAS lines (from include/opcode/riscv-opc.h) were
filtered out.

In this commit:

  commit 08ccfccf0ed825be9be2972594d4be4a2207ef13
  Date:   Mon Jun 8 10:54:53 2020 +0800

      RISC-V: Support debug and float CSR as the unprivileged ones.

Changes were made to include/opcode/riscv-opc.h so that GDB no longer
created even the dscratch alias.

This caused a test failure in gdb.arch/riscv-tdesc-regs.exp.

In looking at how to address this failure I think that the best
strategy is, for now at least, to just remove the code that tries to
create aliases with DECLARE_CSR_ALIAS.

My thoughts are that:

  1. At least some of the aliases are for CSRs where the register now
  has a completely different use.  Being able to reference the CSR
  using a completely inappropriate name just seems confusing.  This
  was solved by the filtering added in the first commit referenced
  above.  But we certainly don't want to blindly add all aliases.

  2. Names presented in a target description are always honoured, so
  if a user has a legacy target then they should just start sending a
  target description with their legacy register names in, this problem
  is then solved.

  3. It's easy enough to figure out which CSRs a target has with the
  info registers command, so missing an alias shouldn't be a big
  issue.

  4.  Allowing users to use names for registers that differ from the
  names the target announces doesn't feel like a critical feature.  If
  in the future targets want multiple names for a register then maybe
  we could/should extend target descriptions to allow the target to
  send aliases as well as the primary name.... but that can wait for
  another day.

So in this commit I remove the use of DECLARE_CSR_ALIAS, and remove
the test that was failing.

gdb/ChangeLog:

	* riscv-tdep.c (riscv_create_csr_aliases): Remove use of
	DECLARE_CSR_ALIAS.

gdb/testsuite/ChangeLog:

	* gdb.arch/riscv-tdesc-regs.exp: Remove unwanted test.
---
 gdb/ChangeLog                               |  5 +++++
 gdb/riscv-tdep.c                            | 10 ----------
 gdb/testsuite/ChangeLog                     |  4 ++++
 gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp |  4 ----
 4 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 437e8f847ae..2f182e8ba8c 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -376,16 +376,6 @@ riscv_create_csr_aliases ()
       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
     }
 }
 
diff --git a/gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp b/gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp
index e35d6181b7f..6ee2bb7cb16 100644
--- a/gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp
+++ b/gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp
@@ -76,10 +76,6 @@ gdb_test_no_output "set tdesc filename $remote_file" \
 # Check that an alias for an unknown CSR will give a suitable error.
 gdb_test "info registers \$csr0" "Invalid register `csr0'"
 
-# Check we can access the dscratch register using either of its names.
-gdb_test "info registers \$dscratch0" "dscratch0\[ \t\]+.*"
-gdb_test "info registers \$dscratch" "dscratch\[ \t\]+.*"
-
 # Return the number of times REGISTER should appear in GROUP, this
 # will either be 0 or 1.
 proc get_expected_result { register group } {
-- 
2.25.4

Re: [PATCH 0/2] RISCV CSR register groups and aliases

[Luis Machado]

On 11/26/20 10:09 AM, Andrew Burgess wrote:
> Some changes to how register groups and aliases are managed for RISC-V
> CSRs.
> 
> ---
> 
> Andrew Burgess (2):
>    gdb/riscv: place unknown csrs into the correct register groups
>    gdb/riscv: remove csr aliases created with DECLARE_CSR_ALIAS
> 
>   gdb/ChangeLog                               | 12 ++++++
>   gdb/riscv-tdep.c                            | 44 +++++++++++++--------
>   gdb/testsuite/ChangeLog                     |  9 +++++
>   gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp | 41 +++++++++++++------
>   4 files changed, 77 insertions(+), 29 deletions(-)
> 

Both look OK to me.

Re: [PATCH 0/2] RISCV CSR register groups and aliases

[Nelson Chu]

Hi Andrew,

Thank you very much for notifying us about these changes.

Nelson

On Thu, Nov 26, 2020 at 9:09 PM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
>
> Some changes to how register groups and aliases are managed for RISC-V
> CSRs.
>
> ---
>
> Andrew Burgess (2):
>   gdb/riscv: place unknown csrs into the correct register groups
>   gdb/riscv: remove csr aliases created with DECLARE_CSR_ALIAS
>
>  gdb/ChangeLog                               | 12 ++++++
>  gdb/riscv-tdep.c                            | 44 +++++++++++++--------
>  gdb/testsuite/ChangeLog                     |  9 +++++
>  gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp | 41 +++++++++++++------
>  4 files changed, 77 insertions(+), 29 deletions(-)
>
> --
> 2.25.4
>

Re: [PATCH 1/2] gdb/riscv: place unknown csrs into the correct register groups

[Jim Wilson]

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

> To see this in action connect to QEMU, this target advertises two
> registers dscratch and mucounteren which are unknown to GDB (these are
> legacy CSRs).  Before this commit these registers would show up in the
> output of:
>

The real problem here is that no one is maintaining the RISC-V qemu gdb
stub support.  I wrote the original version of it, but I can't single
handedly do everything, so I'm still hoping someone else takes up
maintenance of this.  Ideally, the qemu gdb stub should be updated when gdb
is updated.  Last time I looked at qemu, all of the xml files in qemu are
identical to the gdb ones, except for RISC-V, which was identical when I
added it, but no longer is, because the gdb ones are changing but the qemu
ones are not.

Anyways, I don't have any objections to this patch.  This is just a
wishlist for someone to start maintaining qemu properly.

Jim

Re: [PATCH 1/2] gdb/riscv: place unknown csrs into the correct register groups

[Andrew Burgess]

* Jim Wilson <jimw@sifive.com> [2020-11-27 14:38:44 -0800]:

> On Thu, Nov 26, 2020 at 5:09 AM Andrew Burgess <andrew.burgess@embecosm.com>
> wrote:
> 
> > To see this in action connect to QEMU, this target advertises two
> > registers dscratch and mucounteren which are unknown to GDB (these are
> > legacy CSRs).  Before this commit these registers would show up in the
> > output of:
> >
> 
> The real problem here is that no one is maintaining the RISC-V qemu gdb
> stub support.  I wrote the original version of it, but I can't single
> handedly do everything, so I'm still hoping someone else takes up
> maintenance of this.  Ideally, the qemu gdb stub should be updated when gdb
> is updated.

I know we've had discussions on this topic before, but this position
makes no sense to me.

The target description should describe the features that the TARGET
knows about, not be some super-set of registers pulled from GDB (or
anywhere else).

It should be:

  Q: Does QEMU emulate register XXXXX ?
     Yes -> Add register to target description.
     No -> Don't add register to target description.

If QEMU only offers a single configuration of RISC-V then we can get
away with a fixed XML description on the QEMU side.  If QEMU allows us
to change the configuration we're emulating then the target
description will need to be dynamically built on the QEMU side based
on which features are in use.

>                Last time I looked at qemu, all of the xml files in qemu are
> identical to the gdb ones, except for RISC-V, which was identical when I
> added it, but no longer is, because the gdb ones are changing but the qemu
> ones are not.
> 
> Anyways, I don't have any objections to this patch.  This is just a
> wishlist for someone to start maintaining qemu properly.

Indeed.  I do have a task on my list to look at this - it's just
pretty low down on my priorities right now :-/

Anyway, thanks for your feedback.

Andrew