[PATCH 2/9] gdb/riscv: fix failure in gdb.base/completion.exp

[Andrew Burgess <aburgess@redhat.com>]

I noticed a test failure in gdb.base/completion.exp for RISC-V on
a native Linux target, this is the failure:

  (gdb) FAIL: gdb.base/completion.exp: complete 'info registers '

The problem is caused by a mismatch in the output of 'maint print
registers' and the completion list for 'info registers'.  The 'info
registers' completion list contains less registers than
expected. Additionally, the list of registers extracted from the
'maint print registers' list was wrong too, in some cases the test was
grabbing the register number, rather than a register name,

Both of these problems have the same root cause, riscv_register_name
returns nullptr for some registers when it should return an empty
string.

The gdbarch_register_name API is not clearly documented anywhere, and
at first glance it would appear that the function can return either
nullptr, or an empty string to indicate that a register is not
available on the current target.  Indeed, there are plenty of places
in GDB where we compare the output of gdbarch_register_name to both
nullptr and '\0' in order to see if a register is supported or not,
and there are plenty of targets that return empty string in some
cases, and nullptr in others.

However, the 'info registers' completion code (reg_or_group_completer)
clearly depends on user_reg_map_regnum_to_name only returning nullptr
when the passed in regnum is greater than the maximum possible
register number (i.e. after all physical registers, pseudo-registers,
and user-registers), this means that gdbarch_register_name should not
be returning nullptr.

I did consider "fixing" user_reg_map_regnum_to_name, if
gdbarch_register_name returns nullptr, I could convert to an empty
string at this point, but that felt like a real hack, so I discarded
that plan.

The next possibility I considered was "fixing" reg_or_group_completer
to not rely on nullptr to indicate the end marker.  Or rather, I could
have reg_or_group_completer use gdbarch_num_cooked_regs, we know that
we should check at least that many register numbers.  Then, once we're
passed that limit, we keep checking until we hit a nullptr.  This
would absolutely work, and didn't actually feel that bad, but, it
still felt a little weird that gdbarch_register_name could return
nullptr OR the empty string to mean the same thing, so I wondered if
the "right" solution was to have gdbarch_register_name not return
nullptr.  With this in mind I tried an experiment:

I added a self-test that, for each architecture, calls
gdbarch_register_name for every register number up to the
gdbarch_num_cooked_regs limit, and checks that the name is not
nullptr.

Only a handful of architectures failed this test, RISC-V being one of
them.

This seems to suggest that most architectures agree that the correct
API for gdbarch_register_name is to return an empty string for
registers that are not supported on the current target, and that
returning nullptr is really a mistake.

In this commit I will update the RISC-V target so that GDB no longer
returns nullptr from riscv_register_name, instead we return the empty
string.

In subsequent commits I will add the selftest that I mention above,
and will fix the targets that fail the selftest.

With this change the gdb.base/completion.exp test now passes.
---
 gdb/riscv-tdep.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 9ec430d8a10..088cf4ac55e 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -882,8 +882,9 @@ riscv_register_name (struct gdbarch *gdbarch, int regnum)
      then this is an unknown register.  If we do get a name back then we
      look up the registers preferred name below.  */
   const char *name = tdesc_register_name (gdbarch, regnum);
-  if (name == NULL || name[0] == '\0')
-    return NULL;
+  gdb_assert (name != nullptr);
+  if (name[0] == '\0')
+    return name;
 
   /* We want GDB to use the ABI names for registers even if the target
      gives us a target description with the architectural name.  For
@@ -893,13 +894,13 @@ riscv_register_name (struct gdbarch *gdbarch, int regnum)
     return riscv_xreg_feature.register_name (regnum);
 
   /* Like with the x-regs we prefer the abi names for the floating point
-     registers.  */
+     registers.  If the target doesn't have floating point registers then
+     the tdesc_register_name call above should have returned an empty
+     string.  */
   if (regnum >= RISCV_FIRST_FP_REGNUM && regnum <= RISCV_LAST_FP_REGNUM)
     {
-      if (riscv_has_fp_regs (gdbarch))
-	return riscv_freg_feature.register_name (regnum);
-      else
-	return NULL;
+      gdb_assert (riscv_has_fp_regs (gdbarch));
+      return riscv_freg_feature.register_name (regnum);
     }
 
   /* Some targets (QEMU) are reporting these three registers twice, once
@@ -911,12 +912,10 @@ riscv_register_name (struct gdbarch *gdbarch, int regnum)
      duplicate copies of these registers (in riscv_tdesc_unknown_reg) and
      then hide the registers here by giving them no name.  */
   riscv_gdbarch_tdep *tdep = gdbarch_tdep<riscv_gdbarch_tdep> (gdbarch);
-  if (tdep->duplicate_fflags_regnum == regnum)
-    return NULL;
-  if (tdep->duplicate_frm_regnum == regnum)
-    return NULL;
-  if (tdep->duplicate_fcsr_regnum == regnum)
-    return NULL;
+  if (tdep->duplicate_fflags_regnum == regnum
+      || tdep->duplicate_frm_regnum == regnum
+      || tdep->duplicate_fcsr_regnum == regnum)
+    return "";
 
   /* The remaining registers are different.  For all other registers on the
      machine we prefer to see the names that the target description
-- 
2.25.4