public inbox for gdb-cvs@sourceware.org
help / color / mirror / Atom feed
* [binutils-gdb] gdb: add a gdbarch_register_name self test, and fix some architectures
@ 2022-10-02 16:27 Andrew Burgess
  0 siblings, 0 replies; only message in thread
From: Andrew Burgess @ 2022-10-02 16:27 UTC (permalink / raw)
  To: gdb-cvs

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

commit 9a972b54040c6791a163b65a27d24d1acad7c397
Author: Andrew Burgess <aburgess@redhat.com>
Date:   Tue Aug 30 15:21:47 2022 +0100

    gdb: add a gdbarch_register_name self test, and fix some architectures
    
    This commit adds a self-test that checks that gdbarch_register_name
    never returns nullptr for any valid register number.
    
    Most architectures already met this requirement, there were just 6
    that failed the new selftest, and are updated in this commit.
    
    Beyond the self-tests I don't have any facilities to test that the
    architectures I've adjusted still work correctly.
    
    If you review all the various gdbarch_register_name implementations
    then you will see that there are far more architectures that seem like
    they might return nullptr in some situations, e.g. alpha, avr, bpf,
    etc.  This commit doesn't attempt to address these cases as non of
    them are hit during the selftest.  Many of these cases can never be
    hit, for example, in alpha_register_name GDB checks for a register
    number less than zero, this case can't happen and could be changed
    into an assert.
    
    A later commit in this series will have a general cleanup of all the
    various register_name methods, and remove all references to NULL from
    their code, however, as that commit will be mostly adjusting code that
    is never hit, I want to keep those changes separate.
    
    The selftest has been tested on x86-64, but I don't have access to
    suitable systems to fully test any of the *-tdep.c code I've changed
    in this commit.

Diff:
---
 gdb/cris-tdep.c         |  4 +--
 gdb/gdbarch-selftests.c | 27 +++++++++++++++
 gdb/m32c-tdep.c         |  8 ++---
 gdb/m68hc11-tdep.c      |  2 +-
 gdb/mep-tdep.c          |  2 +-
 gdb/sh-tdep.c           | 89 +++++++++++++++++--------------------------------
 gdb/v850-tdep.c         |  8 ++---
 7 files changed, 69 insertions(+), 71 deletions(-)

diff --git a/gdb/cris-tdep.c b/gdb/cris-tdep.c
index 549bc066480..93dcc920946 100644
--- a/gdb/cris-tdep.c
+++ b/gdb/cris-tdep.c
@@ -1654,7 +1654,7 @@ cris_special_register_name (struct gdbarch *gdbarch, int regno)
 	return cris_spec_regs[i].name;
     }
   /* Special register not applicable to this CRIS version.  */
-  return NULL;
+  return "";
 }
 
 static const char *
@@ -1678,7 +1678,7 @@ cris_register_name (struct gdbarch *gdbarch, int regno)
   else
     {
       /* Invalid register.  */
-      return NULL;
+      return "";
     }
 }
 
diff --git a/gdb/gdbarch-selftests.c b/gdb/gdbarch-selftests.c
index 68d340f81ea..68254412ed6 100644
--- a/gdb/gdbarch-selftests.c
+++ b/gdb/gdbarch-selftests.c
@@ -122,6 +122,30 @@ register_to_value_test (struct gdbarch *gdbarch)
     }
 }
 
+/* Test function gdbarch_register_name.  */
+
+static void
+register_name_test (struct gdbarch *gdbarch)
+{
+  scoped_mock_context<test_target_ops> mockctx (gdbarch);
+
+  const int num_regs = gdbarch_num_cooked_regs (gdbarch);
+  for (auto regnum = 0; regnum < num_regs; regnum++)
+    {
+      /* If a register is to be hidden from the user then we should get
+	 back an empty string, not nullptr.  Every other register should
+	 return a non-empty string.  */
+      const char *name = gdbarch_register_name (gdbarch, regnum);
+
+      if (run_verbose() && name == nullptr)
+	debug_printf ("arch: %s, register: %d returned nullptr\n",
+		      gdbarch_bfd_arch_info (gdbarch)->printable_name,
+		      regnum);
+
+      SELF_CHECK (name != nullptr);
+    }
+}
+
 } // namespace selftests
 
 void _initialize_gdbarch_selftests ();
@@ -130,4 +154,7 @@ _initialize_gdbarch_selftests ()
 {
   selftests::register_test_foreach_arch ("register_to_value",
 					 selftests::register_to_value_test);
+
+  selftests::register_test_foreach_arch ("register_name",
+					 selftests::register_name_test);
 }
diff --git a/gdb/m32c-tdep.c b/gdb/m32c-tdep.c
index 07659689c99..2a458f85722 100644
--- a/gdb/m32c-tdep.c
+++ b/gdb/m32c-tdep.c
@@ -757,14 +757,14 @@ mark_save_restore (struct m32c_reg *reg)
 /* A raw banked general-purpose data register named NAME.
    NAME should be an identifier, not a string.  */
 #define RBD(name)						\
-  (R(NULL, tdep->int16, SIM (name ## _bank0)),		\
-   R(NULL, tdep->int16, SIM (name ## _bank1)) - 1)
+  (R("", tdep->int16, SIM (name ## _bank0)),		\
+   R("", tdep->int16, SIM (name ## _bank1)) - 1)
 
 /* A raw banked data address register named NAME.
    NAME should be an identifier, not a string.  */
 #define RBA(name)						\
-  (R(NULL, tdep->data_addr_reg_type, SIM (name ## _bank0)),	\
-   R(NULL, tdep->data_addr_reg_type, SIM (name ## _bank1)) - 1)
+  (R("", tdep->data_addr_reg_type, SIM (name ## _bank0)),	\
+   R("", tdep->data_addr_reg_type, SIM (name ## _bank1)) - 1)
 
 /* A cooked register named NAME referring to a raw banked register
    from the bank selected by the current value of FLG.  RAW_PAIR
diff --git a/gdb/m68hc11-tdep.c b/gdb/m68hc11-tdep.c
index f22f818b147..64e82cfaf18 100644
--- a/gdb/m68hc11-tdep.c
+++ b/gdb/m68hc11-tdep.c
@@ -400,7 +400,7 @@ m68hc11_register_name (struct gdbarch *gdbarch, int reg_nr)
   /* If we don't know the address of a soft register, pretend it
      does not exist.  */
   if (reg_nr > M68HC11_LAST_HARD_REG && soft_regs[reg_nr].name == 0)
-    return NULL;
+    return "";
 
   return m68hc11_register_names[reg_nr];
 }
diff --git a/gdb/mep-tdep.c b/gdb/mep-tdep.c
index 555eb5e144f..67fc9f0cb45 100644
--- a/gdb/mep-tdep.c
+++ b/gdb/mep-tdep.c
@@ -1016,7 +1016,7 @@ mep_register_name (struct gdbarch *gdbarch, int regnr)
      would affect the output of 'info all-registers', which would
      disturb the test suites.  So we leave it invisible.  */
   else
-    return NULL;
+    return "";
 }
 
 
diff --git a/gdb/sh-tdep.c b/gdb/sh-tdep.c
index f6b7df5063b..e4c631b73ec 100644
--- a/gdb/sh-tdep.c
+++ b/gdb/sh-tdep.c
@@ -112,19 +112,12 @@ sh_sh_register_name (struct gdbarch *gdbarch, int reg_nr)
   static const char *register_names[] = {
     "r0", "r1", "r2", "r3", "r4", "r5", "r6", "r7",
     "r8", "r9", "r10", "r11", "r12", "r13", "r14", "r15",
-    "pc", "pr", "gbr", "vbr", "mach", "macl", "sr",
-    "", "",
-    "", "", "", "", "", "", "", "",
-    "", "", "", "", "", "", "", "",
-    "", "",
-    "", "", "", "", "", "", "", "",
-    "", "", "", "", "", "", "", "",
-    "", "", "", "", "", "", "", "",
+    "pc", "pr", "gbr", "vbr", "mach", "macl", "sr"
   };
-  if (reg_nr < 0)
-    return NULL;
+
+  gdb_assert (reg_nr >= 0);
   if (reg_nr >= (sizeof (register_names) / sizeof (*register_names)))
-    return NULL;
+    return "";
   return register_names[reg_nr];
 }
 
@@ -140,13 +133,12 @@ sh_sh3_register_name (struct gdbarch *gdbarch, int reg_nr)
     "", "", "", "", "", "", "", "",
     "ssr", "spc",
     "r0b0", "r1b0", "r2b0", "r3b0", "r4b0", "r5b0", "r6b0", "r7b0",
-    "r0b1", "r1b1", "r2b1", "r3b1", "r4b1", "r5b1", "r6b1", "r7b1"
-    "", "", "", "", "", "", "", "",
+    "r0b1", "r1b1", "r2b1", "r3b1", "r4b1", "r5b1", "r6b1", "r7b1",
   };
-  if (reg_nr < 0)
-    return NULL;
+
+  gdb_assert (reg_nr >= 0);
   if (reg_nr >= (sizeof (register_names) / sizeof (*register_names)))
-    return NULL;
+    return "";
   return register_names[reg_nr];
 }
 
@@ -163,12 +155,10 @@ sh_sh3e_register_name (struct gdbarch *gdbarch, int reg_nr)
     "ssr", "spc",
     "r0b0", "r1b0", "r2b0", "r3b0", "r4b0", "r5b0", "r6b0", "r7b0",
     "r0b1", "r1b1", "r2b1", "r3b1", "r4b1", "r5b1", "r6b1", "r7b1",
-    "", "", "", "", "", "", "", "",
   };
-  if (reg_nr < 0)
-    return NULL;
+  gdb_assert (reg_nr >= 0);
   if (reg_nr >= (sizeof (register_names) / sizeof (*register_names)))
-    return NULL;
+    return "";
   return register_names[reg_nr];
 }
 
@@ -182,15 +172,10 @@ sh_sh2e_register_name (struct gdbarch *gdbarch, int reg_nr)
     "fpul", "fpscr",
     "fr0", "fr1", "fr2", "fr3", "fr4", "fr5", "fr6", "fr7",
     "fr8", "fr9", "fr10", "fr11", "fr12", "fr13", "fr14", "fr15",
-    "", "",
-    "", "", "", "", "", "", "", "",
-    "", "", "", "", "", "", "", "",
-    "", "", "", "", "", "", "", "",
   };
-  if (reg_nr < 0)
-    return NULL;
+  gdb_assert (reg_nr >= 0);
   if (reg_nr >= (sizeof (register_names) / sizeof (*register_names)))
-    return NULL;
+    return "";
   return register_names[reg_nr];
 }
 
@@ -227,10 +212,9 @@ sh_sh2a_register_name (struct gdbarch *gdbarch, int reg_nr)
     /* double precision (pseudo) 68 - 75 */
     "dr0", "dr2", "dr4", "dr6", "dr8", "dr10", "dr12", "dr14",
   };
-  if (reg_nr < 0)
-    return NULL;
+  gdb_assert (reg_nr >= 0);
   if (reg_nr >= (sizeof (register_names) / sizeof (*register_names)))
-    return NULL;
+    return "";
   return register_names[reg_nr];
 }
 
@@ -264,13 +248,11 @@ sh_sh2a_nofpu_register_name (struct gdbarch *gdbarch, int reg_nr)
     "ibcr", "ibnr", "tbr",
     /* 67: register bank number, the user visible pseudo register.  */
     "bank",
-    /* double precision (pseudo) 68 - 75 */
-    "", "", "", "", "", "", "", "",
+    /* double precision (pseudo) 68 - 75: report blank, see below.  */
   };
-  if (reg_nr < 0)
-    return NULL;
+  gdb_assert (reg_nr >= 0);
   if (reg_nr >= (sizeof (register_names) / sizeof (*register_names)))
-    return NULL;
+    return "";
   return register_names[reg_nr];
 }
 
@@ -285,14 +267,11 @@ sh_sh_dsp_register_name (struct gdbarch *gdbarch, int reg_nr)
     "a0g", "a0", "a1g", "a1", "m0", "m1", "x0", "x1",
     "y0", "y1", "", "", "", "", "", "mod",
     "", "",
-    "rs", "re", "", "", "", "", "", "",
-    "", "", "", "", "", "", "", "",
-    "", "", "", "", "", "", "", "",
+    "rs", "re",
   };
-  if (reg_nr < 0)
-    return NULL;
+  gdb_assert (reg_nr >= 0);
   if (reg_nr >= (sizeof (register_names) / sizeof (*register_names)))
-    return NULL;
+    return "";
   return register_names[reg_nr];
 }
 
@@ -309,13 +288,10 @@ sh_sh3_dsp_register_name (struct gdbarch *gdbarch, int reg_nr)
     "ssr", "spc",
     "rs", "re", "", "", "", "", "", "",
     "r0b", "r1b", "r2b", "r3b", "r4b", "r5b", "r6b", "r7b",
-    "", "", "", "", "", "", "", "",
-    "", "", "", "", "", "", "", "",
   };
-  if (reg_nr < 0)
-    return NULL;
+  gdb_assert (reg_nr >= 0);
   if (reg_nr >= (sizeof (register_names) / sizeof (*register_names)))
-    return NULL;
+    return "";
   return register_names[reg_nr];
 }
 
@@ -350,10 +326,9 @@ sh_sh4_register_name (struct gdbarch *gdbarch, int reg_nr)
     /* FIXME: missing XF */
     /* FIXME: missing XD */
   };
-  if (reg_nr < 0)
-    return NULL;
+  gdb_assert (reg_nr >= 0);
   if (reg_nr >= (sizeof (register_names) / sizeof (*register_names)))
-    return NULL;
+    return "";
   return register_names[reg_nr];
 }
 
@@ -383,13 +358,12 @@ sh_sh4_nofpu_register_name (struct gdbarch *gdbarch, int reg_nr)
     "",
     /* double precision (pseudo) 68 - 75 -- not for nofpu target */
     "", "", "", "", "", "", "", "",
-    /* vectors (pseudo) 76 - 79 -- not for nofpu target */
-    "", "", "", "",
+    /* vectors (pseudo) 76 - 79 -- not for nofpu target: report blank
+       below.  */
   };
-  if (reg_nr < 0)
-    return NULL;
+  gdb_assert (reg_nr >= 0);
   if (reg_nr >= (sizeof (register_names) / sizeof (*register_names)))
-    return NULL;
+    return "";
   return register_names[reg_nr];
 }
 
@@ -406,13 +380,10 @@ sh_sh4al_dsp_register_name (struct gdbarch *gdbarch, int reg_nr)
     "ssr", "spc",
     "rs", "re", "", "", "", "", "", "",
     "r0b", "r1b", "r2b", "r3b", "r4b", "r5b", "r6b", "r7b",
-    "", "", "", "", "", "", "", "",
-    "", "", "", "", "", "", "", "",
   };
-  if (reg_nr < 0)
-    return NULL;
+  gdb_assert (reg_nr >= 0);
   if (reg_nr >= (sizeof (register_names) / sizeof (*register_names)))
-    return NULL;
+    return "";
   return register_names[reg_nr];
 }
 
diff --git a/gdb/v850-tdep.c b/gdb/v850-tdep.c
index f27c6ab5364..b7f3d8a9a18 100644
--- a/gdb/v850-tdep.c
+++ b/gdb/v850-tdep.c
@@ -313,7 +313,7 @@ v850_register_name (struct gdbarch *gdbarch, int regnum)
     "pc", "fp"
   };
   if (regnum < 0 || regnum > E_NUM_OF_V850_REGS)
-    return NULL;
+    return "";
   return v850_reg_names[regnum];
 }
 
@@ -333,7 +333,7 @@ v850e_register_name (struct gdbarch *gdbarch, int regnum)
     "pc", "fp"
   };
   if (regnum < 0 || regnum > E_NUM_OF_V850E_REGS)
-    return NULL;
+    return "";
   return v850e_reg_names[regnum];
 }
 
@@ -377,7 +377,7 @@ v850e2_register_name (struct gdbarch *gdbarch, int regnum)
     "", "", "", "fpspc"
   };
   if (regnum < 0 || regnum >= E_NUM_OF_V850E2_REGS)
-    return NULL;
+    return "";
   return v850e2_reg_names[regnum];
 }
 
@@ -480,7 +480,7 @@ v850e3v5_register_name (struct gdbarch *gdbarch, int regnum)
   };
 
   if (regnum < 0 || regnum >= E_NUM_OF_V850E3V5_REGS)
-    return NULL;
+    return "";
   return v850e3v5_reg_names[regnum];
 }

^ permalink raw reply	[flat|nested] only message in thread

only message in thread, other threads:[~2022-10-02 16:27 UTC | newest]

Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-02 16:27 [binutils-gdb] gdb: add a gdbarch_register_name self test, and fix some architectures Andrew Burgess

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).