From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x42d.google.com (mail-wr1-x42d.google.com [IPv6:2a00:1450:4864:20::42d]) by sourceware.org (Postfix) with ESMTPS id 488AC395CCA7 for ; Thu, 11 Jun 2020 13:16:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 488AC395CCA7 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=embecosm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=andrew.burgess@embecosm.com Received: by mail-wr1-x42d.google.com with SMTP id y17so6106405wrn.11 for ; Thu, 11 Jun 2020 06:16:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=6dF8zlnHxtYrEJLL6iMgDyYrTeJH9NnJJ0c2r952aOU=; b=DZDnHzIsR0b3/lSs2rZwKu60O+QC2DC+6NoI8/QYjWLGWKsAKmgEiNO8IL0IUSzobb 2JPhKNefIw3Kyh5RF2C7mJ8wpoYOXVfT7AVyAcN2r0cXXK2QGudKlMloZpufvv2FFr5A sPCCxd3CHCmdVXmWglgZiQtbnU87jEf5SWtis71Dc/3LtcLQKRDPw82XCbayg6K8LxtB HefhemmG1cZ+DvMq9rV3RiG9/IWFWXsYJYkEDtR2VwWWQuegcDDLYgCtA1j8RetsQCtF wpr4dOIVbB2iUAH7GgTIt4WX8xiDsU9gQowm73rGdijQ6b4/b0WVCY6yKXVS+Ad2ZNJP 30UQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=6dF8zlnHxtYrEJLL6iMgDyYrTeJH9NnJJ0c2r952aOU=; b=oN9Xrqx18s4vYlYP088nCsY01fKvaxVNWq8ALdouDpJhbRhBBpbzfg+LBji0VJWRzi vuYM9kr1ijAH9TS28Mqx25O7nK73NBwTbqtRwaO1Abo8ZLU188IkJBTtMV4Ggc8hg6Qk T1L6NgSMG0Qh/VTAFnGxLkMEziKf070w0RdWb8zOoUMDRKEWbf7hmGDIuFH704Kl3ucR zzxPL137NNpI4vIW5KNA+C+t/zmeZEX3/zfaeTARatEzzFN6ECmuHIkxi66EDloo0TEv kOXADX30cd9yma4gkMcgcIdF+1w1VaA8rWsWktgokjLInjnsfuL6+ydeeTvmCd4E+1X+ /wZA== X-Gm-Message-State: AOAM533rMv/NypEXnPCbR/dZ+1JSU9icZv713sLv/uoeNtiwnA3FCmmv tobA61ylCF7ndNu0dSuv11DAIxV0vNg= X-Google-Smtp-Source: ABdhPJwsd5WY8t48eZ2CfJW0vUg6AzysC/fq5Lx9ZRvFlii7YNoQsDn261JcnnkqRkqB9509upsgUg== X-Received: by 2002:a5d:5001:: with SMTP id e1mr10264984wrt.56.1591881382661; Thu, 11 Jun 2020 06:16:22 -0700 (PDT) Received: from localhost (host86-128-12-16.range86-128.btcentralplus.com. [86.128.12.16]) by smtp.gmail.com with ESMTPSA id u3sm4222144wmg.38.2020.06.11.06.16.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 11 Jun 2020 06:16:22 -0700 (PDT) From: Andrew Burgess To: gdb-patches@sourceware.org Subject: [PATCH 2/2] gdb/riscv: Take CSR names from target description Date: Thu, 11 Jun 2020 14:16:15 +0100 Message-Id: <48b40154b43d589400591a380dabea86654ef658.1591880849.git.andrew.burgess@embecosm.com> X-Mailer: git-send-email 2.25.4 In-Reply-To: References: <87d0673rmx.fsf@tromey.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Jun 2020 13:16:26 -0000 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/ChangeLog | 13 ++++++++ gdb/riscv-tdep.c | 79 +++++++++++++++++++++++++++++------------------- 2 files changed, 61 insertions(+), 31 deletions(-) diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c index 64ec9b4a257..b136cbdfbf4 100644 --- a/gdb/riscv-tdep.c +++ b/gdb/riscv-tdep.c @@ -115,7 +115,7 @@ struct riscv_register_feature /* List of names for this register. The first name in this list is the preferred name, the name GDB should use when describing this register. */ - std::vector names; + std::vector names; /* When true this register is required in this feature set. */ bool required_p; @@ -125,16 +125,30 @@ 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; + std::vector registers; }; /* The general x-registers feature set. */ 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; } } -- 2.25.4