[binutils-gdb] gdb: check for duplicate register names in selftest

[binutils-gdb] gdb: check for duplicate register names in selftest

[Andrew Burgess]

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

commit 9a103324fee16bf4d0bc4fa7b19f5d937d47b881
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Wed Aug 31 11:40:16 2022 +0100

    gdb: check for duplicate register names in selftest
    
    Building on the previous commit, this commit extends the register_name
    selftest to check for duplicate register names.
    
    If two registers in the cooked register set (real + pseudo registers)
    have the same name, then this will show up as duplicate registers in
    the 'info all-registers' output, but the user will only be able to
    interact with one copy of the register.
    
    In this commit I extend the selftest that I added in the previous
    commit to check for duplicate register names, I didn't include this
    functionality in the previous commit because one architecture needed
    fixing, and I wanted to keep those fixes separate from the fixes in
    the previous commit.
    
    The problematic architecture(s) are powerpc:750 and powerpc:604.  In
    both of these cases the 'dabr' register appears twice, there's a
    definition of dabr in power-oea.xml which is included into both
    powerpc-604.xml and powerpc-750.xml.  Both of these later two xml
    files also define the dabr register.
    
    I'm hopeful that this change shouldn't break anything, but I don't
    have the ability to actually test this change, however:
    
    On the gdbserver side, neither powerpc-604.xml nor powerpc-750.xml are
    mentioned in gdbserver/configure.srv, which I think means that
    gdbserver will never use these descriptions, and,
    
    Within GDB the problematic descriptions are held in the variables
    tdesc_powerpc_604 and tdesc_powerpc_750, which are only mentioned in
    the variants array in rs6000-tdep.c, this is used when looking up a
    description based on the architecture.
    
    For a native Linux target however, this will not be used as
    ppc_linux_nat_target::read_description exists, which calls
    ppc_linux_match_description, which I don't believe can return either
    of the problematic descriptions.
    
    This leaves the other native targets, FreeBSD, AIX, etc.  These don't
    appear to override the ::read_description method, so will potentially
    return the problematic descriptions, but, in each case I think the
    ::fetch_registers and ::store_registers methods will ignore the dabr
    register, which will leave the register as <unavailable>.
    
    So, my proposed solution is to just remove the duplicate register from
    each of powerpc-604.xml and powerpc-750.xml, then regenerate the
    corresponding C++ source file.  With this change made, the selftest
    now passes for all architectures.

Diff:
---
 gdb/features/rs6000/powerpc-604.c   | 13 ++++++-------
 gdb/features/rs6000/powerpc-604.xml |  1 -
 gdb/features/rs6000/powerpc-750.c   |  1 -
 gdb/features/rs6000/powerpc-750.xml |  1 -
 gdb/gdbarch-selftests.c             | 21 ++++++++++++++++++++-
 5 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/gdb/features/rs6000/powerpc-604.c b/gdb/features/rs6000/powerpc-604.c
index 09cbb50129f..16e6c12dae9 100644
--- a/gdb/features/rs6000/powerpc-604.c
+++ b/gdb/features/rs6000/powerpc-604.c
@@ -141,13 +141,12 @@ initialize_tdesc_powerpc_604 (void)
   tdesc_create_reg (feature, "hid0", 119, 1, NULL, 32, "int");
   tdesc_create_reg (feature, "hid1", 120, 1, NULL, 32, "int");
   tdesc_create_reg (feature, "iabr", 121, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "dabr", 122, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "pir", 123, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "mmcr0", 124, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "pmc1", 125, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "pmc2", 126, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "sia", 127, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "sda", 128, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "pir", 122, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "mmcr0", 123, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "pmc1", 124, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "pmc2", 125, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "sia", 126, 1, NULL, 32, "int");
+  tdesc_create_reg (feature, "sda", 127, 1, NULL, 32, "int");
 
   tdesc_powerpc_604 = result.release ();
 }
diff --git a/gdb/features/rs6000/powerpc-604.xml b/gdb/features/rs6000/powerpc-604.xml
index fbc9e946d22..052cfbb7e76 100644
--- a/gdb/features/rs6000/powerpc-604.xml
+++ b/gdb/features/rs6000/powerpc-604.xml
@@ -17,7 +17,6 @@
     <reg name="hid0" bitsize="32"/>
     <reg name="hid1" bitsize="32"/>
     <reg name="iabr" bitsize="32"/>
-    <reg name="dabr" bitsize="32"/>
     <reg name="pir" bitsize="32"/>
     <reg name="mmcr0" bitsize="32"/>
     <reg name="pmc1" bitsize="32"/>
diff --git a/gdb/features/rs6000/powerpc-750.c b/gdb/features/rs6000/powerpc-750.c
index 60a7b412070..3c9e7e1d23a 100644
--- a/gdb/features/rs6000/powerpc-750.c
+++ b/gdb/features/rs6000/powerpc-750.c
@@ -141,7 +141,6 @@ initialize_tdesc_powerpc_750 (void)
   tdesc_create_reg (feature, "hid0", 119, 1, NULL, 32, "int");
   tdesc_create_reg (feature, "hid1", 120, 1, NULL, 32, "int");
   tdesc_create_reg (feature, "iabr", 121, 1, NULL, 32, "int");
-  tdesc_create_reg (feature, "dabr", 122, 1, NULL, 32, "int");
   tdesc_create_reg (feature, "ummcr0", 124, 1, NULL, 32, "int");
   tdesc_create_reg (feature, "upmc1", 125, 1, NULL, 32, "int");
   tdesc_create_reg (feature, "upmc2", 126, 1, NULL, 32, "int");
diff --git a/gdb/features/rs6000/powerpc-750.xml b/gdb/features/rs6000/powerpc-750.xml
index 908ecbdfd96..dfc5a87aaa0 100644
--- a/gdb/features/rs6000/powerpc-750.xml
+++ b/gdb/features/rs6000/powerpc-750.xml
@@ -17,7 +17,6 @@
     <reg name="hid0" bitsize="32"/>
     <reg name="hid1" bitsize="32"/>
     <reg name="iabr" bitsize="32"/>
-    <reg name="dabr" bitsize="32"/>
     <reg name="ummcr0" bitsize="32" regnum="124"/>
     <reg name="upmc1" bitsize="32"/>
     <reg name="upmc2" bitsize="32"/>
diff --git a/gdb/gdbarch-selftests.c b/gdb/gdbarch-selftests.c
index 68254412ed6..fa3f4d180cf 100644
--- a/gdb/gdbarch-selftests.c
+++ b/gdb/gdbarch-selftests.c
@@ -27,6 +27,8 @@
 #include "gdbarch.h"
 #include "scoped-mock-context.h"
 
+#include <map>
+
 namespace selftests {
 
 /* Test gdbarch methods register_to_value and value_to_register.  */
@@ -129,6 +131,9 @@ register_name_test (struct gdbarch *gdbarch)
 {
   scoped_mock_context<test_target_ops> mockctx (gdbarch);
 
+  /* Track the number of times each register name appears.  */
+  std::map<const std::string, int> name_counts;
+
   const int num_regs = gdbarch_num_cooked_regs (gdbarch);
   for (auto regnum = 0; regnum < num_regs; regnum++)
     {
@@ -141,8 +146,22 @@ register_name_test (struct gdbarch *gdbarch)
 	debug_printf ("arch: %s, register: %d returned nullptr\n",
 		      gdbarch_bfd_arch_info (gdbarch)->printable_name,
 		      regnum);
-
       SELF_CHECK (name != nullptr);
+
+      /* Every register name, that is not the empty string, should be
+	 unique.  If this is not the case then the user will see duplicate
+	 copies of the register in e.g. 'info registers' output, but will
+	 only be able to interact with one of the copies.  */
+      if (*name != '\0')
+	{
+	  std::string s (name);
+	  name_counts[s]++;
+	  if (run_verbose() && name_counts[s] > 1)
+	    debug_printf ("arch: %s, register: %d (%s) is a duplicate\n",
+			  gdbarch_bfd_arch_info (gdbarch)->printable_name,
+			  regnum, name);
+	  SELF_CHECK (name_counts[s] == 1);
+	}
     }
 }