From bd93abe8d5513ceecc10a91452a85c803cac895b Mon Sep 17 00:00:00 2001 From: Andrew Burgess Date: Sun, 28 Aug 2022 20:40:28 +0100 Subject: [PATCH] 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. --- 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 (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.30.2