From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x341.google.com (mail-wm1-x341.google.com [IPv6:2a00:1450:4864:20::341]) by sourceware.org (Postfix) with ESMTPS id 736A43851C2B for ; Fri, 12 Jun 2020 22:34:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 736A43851C2B 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-wm1-x341.google.com with SMTP id y20so9412227wmi.2 for ; Fri, 12 Jun 2020 15:34:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=embecosm.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=gXZb07JcsHBHh2SZdFp9DShcpHcVi4UhaJ3S4RGbRlg=; b=OSy5DyoAPHrwbSywIIcTGnRZOZ2wyhJph+OaQxlkN7siB6UIogQHz+ty+GGnQ+TK4u 7QRgW3c2xmuRqdCEJO6K/q3XwzqpB+mBLBEb3QXJ7+gPuSkuNSXA5/pJ5hSVupEhiNde h0GTWHSYwOrIjd/M/v96KkDefbxGmLElhHHAEavISXWpGv/m1NHdSAnufL3CM7DxigHD IAkSASCr/kwq/I2oWYdg4NqA1bY7yhCK8PoJIDm6zsslQ9PTilCe1J6a47KyTW2AEvP4 oFiYd06oAyWcUFdLeveNyPyroG29tp0RC6PUvaaj3VPLiXVQcAeGkGCh6n7A2XFuWdev 7F8A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=gXZb07JcsHBHh2SZdFp9DShcpHcVi4UhaJ3S4RGbRlg=; b=bHK7xV9fMOWgEZDn7qi/zRr/CGOkuNZ1M3J1aEmkKabqBOlYlVtZ8jT8Rt1t+4Oo7d +5kjKPqieyOV74niPKQNaAhy83k1EKPtLL2C0fag8N9aGjEUsazKuSOXjkeWSY4Zq8KA X2WRmba0eUlAMlKUgyDEzpRGAVxJj9mSDC1JycEvQj7ihWsAxv6uHYWPR+cz4PhWTS3/ ONdeMuDmykVAJdrjGJI0kA9giT+kHcor9ZUlm8ZFUbEm7O52+pAFljgDSHLax1b96G95 D2+jmm4UPCs2E2sHzivYiVYpcopaE7RYXSaAVF+fW4u4lPaXSHK+eTC5kmTY1066jdLw nZsg== X-Gm-Message-State: AOAM531HegVBrysvqu+79/ueF+BflFjej7U75+00NDQkfLx43ekPahEP D3MO3Fs517nqzPqcXOY1MXoX1K9amAE= X-Google-Smtp-Source: ABdhPJwLsnaqnj+HHjcs1/weBU0FsdkY/IU+4bT4euUOvEHTJAqRyhT3/fpkbwauTsgfwwMCV68evg== X-Received: by 2002:a1c:4009:: with SMTP id n9mr1075608wma.104.1592001249227; Fri, 12 Jun 2020 15:34:09 -0700 (PDT) Received: from localhost (host86-128-12-16.range86-128.btcentralplus.com. [86.128.12.16]) by smtp.gmail.com with ESMTPSA id c16sm10551598wml.45.2020.06.12.15.34.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 12 Jun 2020 15:34:08 -0700 (PDT) Date: Fri, 12 Jun 2020 23:34:07 +0100 From: Andrew Burgess To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 0/2] [PATCHv2] gdb/riscv: Improved register alias name creation Message-ID: <20200612223407.GM2737@embecosm.com> References: <87d0673rmx.fsf@tromey.com> <87d065vgry.fsf@tromey.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87d065vgry.fsf@tromey.com> X-Operating-System: Linux/5.6.15-200.fc31.x86_64 (x86_64) X-Uptime: 23:16:13 up 4 days, 12:23, X-Editor: GNU Emacs [ http://www.gnu.org/software/emacs ] X-Spam-Status: No, score=-10.1 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, URIBL_CSS, URIBL_CSS_A 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: Fri, 12 Jun 2020 22:34:12 -0000 * Tom Tromey [2020-06-11 08:06:09 -0600]: > >>>>> "Andrew" == Andrew Burgess writes: > > Andrew> I now have two patches. The first alone should be enough to get > Andrew> things working for you now; it's basically the same patch as before > Andrew> but with a small bug fix. > > [...] > > Andrew> This leaves just one question from me. I thought that your original > Andrew> thread started by saying that once upon a time you could successfully > Andrew> read the dscratch and/or the dscratch0 registers. The some patches > Andrew> landed and you could no longer read the registers. > > Andrew> From what I'm seeing though, the _actual_ failure to read the register > Andrew> is all QEMU. So my question, did I missunderstand the original > Andrew> problem? Is there a build of GDB where you are able to read actual > Andrew> values from these registers? > > We discussed this on irc a little. The tests where I would connect and > print various registers don't seem to reflect the failures I was > actually seeing. So, for example "print $dscratch" might say "= void" > ... meaning this was an uninitialized convenience variable rather than a > register. > > The actual failure is that some operation in gdb fails, saying a > register isn't available. For example, one test case fails when it > tries to do an inferior call. > > I applied your patches to git master (no local patches at all) and it > fails like so: > > (gdb) call debug.trace (me, "You") > Could not fetch register "mucounteren"; remote failure reply 'E14' > > However, if I check out commit d8af906814b (the one before the first > round of RISC-V changes we're talking about), it works: > > (gdb) call debug.trace (me, "You") > (gdb) So I finally figured out what's happening, and I feel like I opened a (smallish) can of worms... What happens is this. When we make an inferior call, any register that is in the 'save' register group is, well, saved before the call, and restored afterwards. The default behaviour for a register in the target description is to be in the save set. The problem with this is that QEMU announces many registers, for example, mucounteren, but then doesn't actually let GDB read it. If any register fails to read before an inferior call then it can't be backed up, so GDB aborts the inferior call. The _ideal_ solution here would be either (a) QEMU doesn't announce registers that GDB can't read, or (b) QEMU marks registers that can't _always_ be read as no-save. Still, we work with what we've got, right? In order to work around this issue in the past the RISC-V target provides riscv_register_reggroup_p, which overrides the usual target description method for figuring out if a register is in a particular group. This RISC-V method specifically says that the CSRs (except for the floating point status CSRs) should not be included in the save register set. Problem solved. Except, in commit d8af906814b we changed the set of CSRs that GDB knows about, removing mucounteren. Now this shouldn't be a problem. QEMU still announces the register, GDB can still access it. Everything is still good. But, internally, we no longer see this as part of the CSRs, it's just "some other register that the target offers", and as we don't know what to do with this, we just do what the target tells us, which is the default, please save this registers. Only we can't save that register, as we can't read it. So, when I started working on this problem, I ran into another issue, which isn't a show stopper, but is an annoyance. GDB's builtin XML files, include the fflags, fcsr, and frm registers twice, once in the FPU feature, and once in the CSR feature, and as QEMU copied its XML files from GDB, QEMU also announces these regiters twice. If you connect to QEMU and then do: (gdb) info registers save You'll see these three regiters appear twice. Which is pretty annoying. Anyway. The patch below attempts to tackle both of these issues, with this your inferior call issue should be resolved. This is totally a WIP right now, I'm just sharing it so you can test this. I don't know if the final solution I go with should actually be this complex, or if I should take a simpler approach. I'm going to think this through for a couple of days, and play with a few ideas, but I was hoping you might confirm this patch resolves the issue you're seeing in the mean time. Thanks, Andrew --- commit 3581151a580b5fb561ff088e6ac9e963cdb2ddd0 Author: Andrew Burgess Date: Fri Jun 12 23:14:23 2020 +0100 WIP: Handle unknown target description registers diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c index b136cbdfbf4..6c70a1526f3 100644 --- a/gdb/riscv-tdep.c +++ b/gdb/riscv-tdep.c @@ -522,6 +522,15 @@ riscv_register_name (struct gdbarch *gdbarch, int regnum) return NULL; } + /* Hide these registers. */ + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); + if (tdep->duplicate_fflags == regnum) + return NULL; + if (tdep->duplicate_frm == regnum) + return NULL; + if (tdep->duplicate_fcsr == regnum) + return NULL; + /* 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 @@ -873,6 +882,13 @@ riscv_register_reggroup_p (struct gdbarch *gdbarch, int regnum, if (regnum > RISCV_LAST_REGNUM) { + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); + + if ((reggroup == restore_reggroup || reggroup == save_reggroup) + && regnum >= tdep->first_extra_csr_regnum + && regnum < (tdep->first_extra_csr_regnum + tdep->extra_csr_count)) + return 0; + int ret = tdesc_register_in_reggroup_p (gdbarch, regnum, reggroup); if (ret != -1) return ret; @@ -3015,7 +3031,6 @@ riscv_check_tdesc_feature (struct tdesc_arch_data *tdesc_data, { found = tdesc_numbered_register (feature, tdesc_data, reg.regnum, name); - if (found) { /* We know that the target description mentions this @@ -3116,6 +3131,72 @@ riscv_gcc_target_options (struct gdbarch *gdbarch) return target_options; } +/* ... */ + +static int +riscv_unknown_tdesc_reg (struct gdbarch *gdbarch, tdesc_feature *feature, + const char *reg_name, int possible_regnum) +{ + /* At one point in time GDB had an incorrect default target description + that duplicated the fflags, frm, and fcsr registers in both the FPU + and CSR register sets. + + Some targets (QEMU) copied these target descriptions into their source + tree, and so we're currently stuck working with some targets that + declare the same registers twice. + + There's not much we can do about this any more. Assuming the target + will direct a request for either register number to the correct + underlying hardware register then it doesn't matter which one GDB + uses, so long as we (GDB) are consistent (so that we don't end up with + invalid cache misses). + + As we always scan the FPU registers first, then the CSRs, if the + target has included the offending registers in both sets then we will + always see the FPU copies here, as the CSR versions will replace them + in the register list. + + To prevent these duplicates showing up in any of the register list, + record their register numbers here. */ + if (strcmp (tdesc_feature_name (feature), riscv_freg_feature.name) == 0) + { + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); + + if (strcmp (reg_name, "fflags") == 0) + { + gdb_assert (tdep->duplicate_fflags == -1); + tdep->duplicate_fflags = possible_regnum; + return possible_regnum; + } + + if (strcmp (reg_name, "frm") == 0) + { + gdb_assert (tdep->duplicate_frm == -1); + tdep->duplicate_frm = possible_regnum; + return possible_regnum; + } + + if (strcmp (reg_name, "fcsr") == 0) + { + gdb_assert (tdep->duplicate_fcsr == -1); + tdep->duplicate_fcsr = possible_regnum; + return possible_regnum; + } + } + + if (strcmp (tdesc_feature_name (feature), riscv_csr_feature.name) == 0) + { + struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch); + if (tdep->first_extra_csr_regnum == -1) + tdep->first_extra_csr_regnum = possible_regnum; + gdb_assert (tdep->first_extra_csr_regnum + + tdep->extra_csr_count == possible_regnum); + tdep->extra_csr_count++; + return possible_regnum; + } + return -1; +} + /* Implement the gnu_triplet_regexp method. A single compiler supports both 32-bit and 64-bit code, and may be named riscv32 or riscv64 or (not recommended) riscv. */ @@ -3352,6 +3433,7 @@ riscv_gdbarch_init (struct gdbarch_info info, set_gdbarch_print_registers_info (gdbarch, riscv_print_registers_info); /* Finalise the target description registers. */ + set_tdesc_unknown_register_handler (gdbarch, riscv_unknown_tdesc_reg); tdesc_use_registers (gdbarch, tdesc, tdesc_data); /* Override the register type callback setup by the target description diff --git a/gdb/riscv-tdep.h b/gdb/riscv-tdep.h index e415fb4a7a1..69f742f4333 100644 --- a/gdb/riscv-tdep.h +++ b/gdb/riscv-tdep.h @@ -79,6 +79,14 @@ struct gdbarch_tdep /* ISA-specific data types. */ struct type *riscv_fpreg_d_type = nullptr; + + int first_extra_csr_regnum = -1; + int extra_csr_count = 0; + + int duplicate_fflags = -1; + int duplicate_frm = -1; + int duplicate_fcsr = -1; + }; diff --git a/gdb/target-descriptions.c b/gdb/target-descriptions.c index 20a3a640f4f..351cd4ab166 100644 --- a/gdb/target-descriptions.c +++ b/gdb/target-descriptions.c @@ -407,6 +407,10 @@ struct tdesc_arch_data gdbarch_register_name_ftype *pseudo_register_name = NULL; gdbarch_register_type_ftype *pseudo_register_type = NULL; gdbarch_register_reggroup_p_ftype *pseudo_register_reggroup_p = NULL; + + /* Function to (possibly) assign register numbers for unknown + registers. */ + tdesc_unknown_register_ftype *unknown_register_handler = NULL; }; /* Info about an inferior's target description. There's one of these @@ -1051,6 +1055,16 @@ set_tdesc_pseudo_register_reggroup_p data->pseudo_register_reggroup_p = pseudo_reggroup_p; } +void set_tdesc_unknown_register_handler + (struct gdbarch *gdbarch, + tdesc_unknown_register_ftype *handler) +{ + struct tdesc_arch_data *data + = (struct tdesc_arch_data *) gdbarch_data (gdbarch, tdesc_data); + + data->unknown_register_handler = handler; +} + /* Update GDBARCH to use the target description for registers. */ void @@ -1105,6 +1119,36 @@ tdesc_use_registers (struct gdbarch *gdbarch, while (data->arch_regs.size () < num_regs) data->arch_regs.emplace_back (nullptr, nullptr); + /* First we give the target a chance to number previously unknown + registers. This allows targets to record the numbers assigned based + on which feature the register was from. */ + if (data->unknown_register_handler != NULL) + { + for (const tdesc_feature_up &feature : target_desc->features) + for (const tdesc_reg_up ® : feature->registers) + if (htab_find (reg_hash, reg.get ()) != NULL) + { + int regno + = data->unknown_register_handler (gdbarch, feature.get (), + reg->name.c_str (), + num_regs); + gdb_assert (regno == -1 || regno >= num_regs); + if (regno != -1) + { + while (regno >= data->arch_regs.size ()) + data->arch_regs.emplace_back (nullptr, nullptr); + data->arch_regs[regno] = tdesc_arch_reg (reg.get (), NULL); + num_regs = regno + 1; + htab_remove_elt (reg_hash, reg.get ()); + } + } + } + + /* Ensure the array was sized correctly above. */ + gdb_assert (data->arch_regs.size () == num_regs); + + /* Now in a final pass we assign register numbers to any remaining + unnumbered registers. */ for (const tdesc_feature_up &feature : target_desc->features) for (const tdesc_reg_up ® : feature->registers) if (htab_find (reg_hash, reg.get ()) != NULL) @@ -1680,7 +1724,7 @@ maint_print_c_tdesc_cmd (const char *args, int from_tty) error (_("There is no target description to print.")); if (filename == NULL) - error (_("The current target description did not come from an XML file.")); + filename = "fetched from target"; std::string filename_after_features (filename); auto loc = filename_after_features.rfind ("/features/"); diff --git a/gdb/target-descriptions.h b/gdb/target-descriptions.h index 96d283fb379..fb6da4add4c 100644 --- a/gdb/target-descriptions.h +++ b/gdb/target-descriptions.h @@ -80,6 +80,14 @@ void set_tdesc_pseudo_register_reggroup_p (struct gdbarch *gdbarch, gdbarch_register_reggroup_p_ftype *pseudo_reggroup_p); +typedef int (tdesc_unknown_register_ftype) + (struct gdbarch *gdbarch, tdesc_feature *feature, + const char *reg_name, int possible_regnum); + +void set_tdesc_unknown_register_handler + (struct gdbarch *gdbarch, + tdesc_unknown_register_ftype *handler); + /* Update GDBARCH to use the TARGET_DESC for registers. TARGET_DESC may be GDBARCH's target description or (if GDBARCH does not have one which describes registers) another target description