From: Andrew Burgess Date: Thu, 11 Jun 2020 10:49:05 +0000 (+0100) Subject: gdb/riscv: Take CSR names from target description X-Git-Url: https://git.libre-soc.org/?a=commitdiff_plain;h=ed69cbc8ef0a403caff2725072362252ac27bee3;p=binutils-gdb.git 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. --- diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 0a8a6b69580..3f0c10f2ce4 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,16 @@ +2020-06-25 Andrew Burgess + + * 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. + 2020-06-25 Andrew Burgess * riscv-tdep.c (struct riscv_register_feature): Fix whitespace diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c index bee30e06465..86ab3c3143e 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 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 *) ®.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 *) ®.regnum); + } break; } } diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog index e333748755e..76641b3c333 100644 --- a/gdb/testsuite/ChangeLog +++ b/gdb/testsuite/ChangeLog @@ -1,3 +1,7 @@ +2020-06-25 Andrew Burgess + + * gdb.arch/riscv-tdesc-regs.exp: Extend test case. + 2020-06-25 Andrew Burgess * gdb.arch/riscv-tdesc-regs-32.xml: New file. 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 . # 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 +}