[binutils-gdb] gdb/riscv: fix failure in gdb.base/completion.exp

[binutils-gdb] gdb/riscv: fix failure in gdb.base/completion.exp

[Andrew Burgess]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=bd93abe8d5513ceecc10a91452a85c803cac895b

commit bd93abe8d5513ceecc10a91452a85c803cac895b
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Sun Aug 28 20:40:28 2022 +0100

    gdb/riscv: fix failure in gdb.base/completion.exp
    
    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.

Diff:
---
 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 e419b97693d..7bddf535804 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