gdb/riscv: Take CSR names from target description
authorAndrew Burgess <andrew.burgess@embecosm.com>
Thu, 11 Jun 2020 10:49:05 +0000 (11:49 +0100)
committerAndrew Burgess <andrew.burgess@embecosm.com>
Thu, 25 Jun 2020 17:07:30 +0000 (18:07 +0100)
commited69cbc8ef0a403caff2725072362252ac27bee3
tree9bd31d4cdba95c008fc653c486529643acd8537c
parent4445e8f59ac69e64e64c5f95882aa8172e4788f7
gdb/riscv: Take CSR names from target description

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
gdb/riscv-tdep.c
gdb/testsuite/ChangeLog
gdb/testsuite/gdb.arch/riscv-tdesc-regs.exp