Re: [PATCH 0/2] [PATCHv2] gdb/riscv: Improved register alias name creation

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Tom Tromey <tom@tromey.com> [2020-06-11 08:06:09 -0600]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> 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 <andrew.burgess@embecosm.com>
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 &reg : 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 &reg : 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