diff mbox series

[1/1] gdb/riscv: Add command to switch between numeric & abi register names

Message ID 20250102192019.10584-1-ciaranwoodward@xmos.com
State Superseded, archived
Headers
Series [1/1] gdb/riscv: Add command to switch between numeric & abi register names |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed

Commit Message

Ciaran Woodward Jan. 2, 2025, 7:20 p.m. UTC 
  In RISC-V, the general registers can be shown in their abi
form (e.g. sp, a0) or their numeric form (e.g. x2, x10).
Depending on context/preference, someone may prefer to
see one form over the other. The disassembler already
supports this configuration, which can be changed using
the 'set disassembler-options numeric' command.

This commit adds a new set/show command to change gdb's
preference: 'set riscv numeric-registers-names on/off'.

If on, 'info registers' and other situations will print
the numeric register names, rather than the abi versions.
The alias generation has been modified so that the abi
versions are still available for access if specifically
requested such as 'print $ra'. This was done by changing
the behaviour of prefer_first_name_p somewhat - so if
prefer_first_name_p is false, all register names will
be added as aliases.

I will note that the check that was removed appears to
have already been broken, since the equality check was
(const char * == const char *), not a string comparison.
There is also no functional downside to adding aliases
which are surplus-to-requirement, since they will be
ignored if there is a 'true' register with the same
name.
---
 gdb/riscv-tdep.c | 42 ++++++++++++++++++++++++++++++++++++++----
 1 file changed, 38 insertions(+), 4 deletions(-)

Comments

Tom Tromey Jan. 14, 2025, 5:06 p.m. UTC | #1 
>>>>> "Ciaran" == Ciaran Woodward <ciaranwoodward@xmos.com> writes:

Thank you for the patch.

I think a RISC-V maintainer should take a look at this as well (I know
nothing about the arch) but I had a couple of notes.

Ciaran> I will note that the check that was removed appears to
Ciaran> have already been broken, since the equality check was
Ciaran> (const char * == const char *), not a string comparison.

Looking at the code it seems like these both come from the same array,
so perhaps == is fine:

  for (const char *name : this->names)
...
	      const char *alias = this->names[i];
	      if (alias == name && !prefer_first_name_p)

Ciaran> +    if(numeric_register_names && (regnum <= RISCV_ZERO_REGNUM + 31))

Space before paren.
There's a few instances of this.

Ciaran> +  add_setshow_boolean_cmd("numeric-register-names", no_class,
Ciaran> +			  &numeric_register_names,

New commands should be documented and mentioned in gdb/NEWS.

thanks,
Tom
Andrew Burgess Jan. 14, 2025, 5:56 p.m. UTC | #2 
Thanks for working on this.  I like this change, but I do have a couple
of thoughts.

Ciaran Woodward <ciaranwoodward@xmos.com> writes:

> In RISC-V, the general registers can be shown in their abi
> form (e.g. sp, a0) or their numeric form (e.g. x2, x10).
> Depending on context/preference, someone may prefer to
> see one form over the other. The disassembler already
> supports this configuration, which can be changed using
> the 'set disassembler-options numeric' command.
>
> This commit adds a new set/show command to change gdb's
> preference: 'set riscv numeric-registers-names on/off'.
>
> If on, 'info registers' and other situations will print
> the numeric register names, rather than the abi versions.
> The alias generation has been modified so that the abi
> versions are still available for access if specifically
> requested such as 'print $ra'. This was done by changing
> the behaviour of prefer_first_name_p somewhat - so if
> prefer_first_name_p is false, all register names will
> be added as aliases.

Given what you say below about unnecessary aliases being harmless, it
seems like we should just remove the prefer_first_name_p completely.

With that done, then, to take $ra as an example, we'd create the aliases
'ra' and 'x1'.  Then riscv_register_name will return which ever name is
currently preferred based on numeric_register_names.

>
> I will note that the check that was removed appears to
> have already been broken, since the equality check was
> (const char * == const char *), not a string comparison.

As Tom pointed out, both pointers came from the same array, so the
comparison was fine.

> There is also no functional downside to adding aliases
> which are surplus-to-requirement, since they will be
> ignored if there is a 'true' register with the same
> name.
> ---
>  gdb/riscv-tdep.c | 42 ++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 932708ca4e9..5fe470e044b 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -134,6 +134,11 @@ static const char *riscv_feature_name_vector = "org.gnu.gdb.riscv.vector";
>  /* The current set of options to be passed to the disassembler.  */
>  static std::string riscv_disassembler_options;
>  
> +/* Controls whether we show register names in their numeric form (eg. x28), or
> + * their abi form (eg. t3).  */

No '*' on the second line of the comment please.  Also, could you make
the comment clearer, as in:

  /* When true show numeric form of register names (e.g. x28).  When
     false use the abi form (e.g. t3).  */
   
> +
> +static bool numeric_register_names = false;
> +
>  /* Cached information about a frame.  */
>  
>  struct riscv_unwind_cache
> @@ -282,8 +287,6 @@ riscv_register_feature::register_info::check
>  	  for (int i = start_index; i < this->names.size (); ++i)
>  	    {
>  	      const char *alias = this->names[i];
> -	      if (alias == name && !prefer_first_name_p)
> -		continue;
>  	      aliases->emplace_back (alias, (void *) &this->regnum);
>  	    }
>  	  return true;
> @@ -342,6 +345,8 @@ struct riscv_xreg_feature : public riscv_register_feature
>    const char *register_name (int regnum) const
>    {
>      gdb_assert (regnum >= RISCV_ZERO_REGNUM && regnum <= m_registers.size ());
> +    if(numeric_register_names && (regnum <= RISCV_ZERO_REGNUM + 31))
> +      return m_registers[regnum].names[1];
>      return m_registers[regnum].names[0];

If you checkout riscv_register_name you'll find two comments that need
to be updated, they talk about the ABI register names being preferred
for x-reg and f-regs, but that is no longer true.

>    }
>  
> @@ -360,7 +365,7 @@ struct riscv_xreg_feature : public riscv_register_feature
>      bool seen_an_optional_reg_p = false;
>      for (const auto &reg : m_registers)
>        {
> -	bool found = reg.check (tdesc_data, feature_cpu, true, aliases);
> +	bool found = reg.check (tdesc_data, feature_cpu, false, aliases);
>  
>  	bool is_optional_reg_p = (reg.regnum >= RISCV_ZERO_REGNUM + 16
>  				  && reg.regnum < RISCV_ZERO_REGNUM + 32);
> @@ -444,6 +449,8 @@ struct riscv_freg_feature : public riscv_register_feature
>      gdb_assert (regnum >= RISCV_FIRST_FP_REGNUM
>  		&& regnum <= RISCV_LAST_FP_REGNUM);
>      regnum -= RISCV_FIRST_FP_REGNUM;
> +    if(numeric_register_names && (regnum <= 31))
> +      return m_registers[regnum].names[1];
>      return m_registers[regnum].names[0];
>    }
>  
> @@ -469,7 +476,7 @@ struct riscv_freg_feature : public riscv_register_feature
>         are missing this is not fatal.  */
>      for (const auto &reg : m_registers)
>        {
> -	bool found = reg.check (tdesc_data, feature_fpu, true, aliases);
> +	bool found = reg.check (tdesc_data, feature_fpu, false, aliases);
>  
>  	bool is_ctrl_reg_p = reg.regnum > RISCV_LAST_FP_REGNUM;
>  
> @@ -736,6 +743,18 @@ show_use_compressed_breakpoints (struct ui_file *file, int from_tty,
>  		"to %s.\n"), value);
>  }
>  
> +/* The show callback for 'show riscv numeric-register-names'.  */
> +
> +static void
> +show_numeric_register_names (struct ui_file *file, int from_tty,
> +			     struct cmd_list_element *c,
> +			     const char *value)
> +{
> +  gdb_printf (file,
> +	      _("Displaying registers with their numeric names is %s.\n"),
> +	      value);
> +}
> +
>  /* The set and show lists for 'set riscv' and 'show riscv' prefixes.  */
>  
>  static struct cmd_list_element *setriscvcmdlist = NULL;
> @@ -4832,4 +4851,19 @@ this option can be used."),
>  				show_use_compressed_breakpoints,
>  				&setriscvcmdlist,
>  				&showriscvcmdlist);
> +
> +  add_setshow_boolean_cmd("numeric-register-names", no_class,

Space before '(' here please.

> +			  &numeric_register_names,
> +			  _("\
> +Set displaying registers with numeric names instead of abi names."), _("\
> +Show whether registers are displayed with numeric names instead of abi names."),
> +			  _("\
> +When enabled, registers will be shown with their numeric names (such as x28)\n\
> +instead of their abi names (such as t0).\n\
> +Also consider using the 'set disassembler-options numeric' command for the\n\
> +equivalent change in the disassembler output."),
> +			  NULL,
> +			  show_numeric_register_names,
> +			  &setriscvcmdlist,
> +			  &showriscvcmdlist);

As Tom has said, you'll needs to add a NEWS entry, and a doc/ entry.
For the docs entry, you should add a new RISC-V entry under '@node
Architectures', checkout how the existing architectures are handled,
then document your new setting.

Thanks,
Andrew
diff mbox series

Patch

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 932708ca4e9..5fe470e044b 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -134,6 +134,11 @@  static const char *riscv_feature_name_vector = "org.gnu.gdb.riscv.vector";
 /* The current set of options to be passed to the disassembler.  */
 static std::string riscv_disassembler_options;
 
+/* Controls whether we show register names in their numeric form (eg. x28), or
+ * their abi form (eg. t3).  */
+
+static bool numeric_register_names = false;
+
 /* Cached information about a frame.  */
 
 struct riscv_unwind_cache
@@ -282,8 +287,6 @@  riscv_register_feature::register_info::check
 	  for (int i = start_index; i < this->names.size (); ++i)
 	    {
 	      const char *alias = this->names[i];
-	      if (alias == name && !prefer_first_name_p)
-		continue;
 	      aliases->emplace_back (alias, (void *) &this->regnum);
 	    }
 	  return true;
@@ -342,6 +345,8 @@  struct riscv_xreg_feature : public riscv_register_feature
   const char *register_name (int regnum) const
   {
     gdb_assert (regnum >= RISCV_ZERO_REGNUM && regnum <= m_registers.size ());
+    if(numeric_register_names && (regnum <= RISCV_ZERO_REGNUM + 31))
+      return m_registers[regnum].names[1];
     return m_registers[regnum].names[0];
   }
 
@@ -360,7 +365,7 @@  struct riscv_xreg_feature : public riscv_register_feature
     bool seen_an_optional_reg_p = false;
     for (const auto &reg : m_registers)
       {
-	bool found = reg.check (tdesc_data, feature_cpu, true, aliases);
+	bool found = reg.check (tdesc_data, feature_cpu, false, aliases);
 
 	bool is_optional_reg_p = (reg.regnum >= RISCV_ZERO_REGNUM + 16
 				  && reg.regnum < RISCV_ZERO_REGNUM + 32);
@@ -444,6 +449,8 @@  struct riscv_freg_feature : public riscv_register_feature
     gdb_assert (regnum >= RISCV_FIRST_FP_REGNUM
 		&& regnum <= RISCV_LAST_FP_REGNUM);
     regnum -= RISCV_FIRST_FP_REGNUM;
+    if(numeric_register_names && (regnum <= 31))
+      return m_registers[regnum].names[1];
     return m_registers[regnum].names[0];
   }
 
@@ -469,7 +476,7 @@  struct riscv_freg_feature : public riscv_register_feature
        are missing this is not fatal.  */
     for (const auto &reg : m_registers)
       {
-	bool found = reg.check (tdesc_data, feature_fpu, true, aliases);
+	bool found = reg.check (tdesc_data, feature_fpu, false, aliases);
 
 	bool is_ctrl_reg_p = reg.regnum > RISCV_LAST_FP_REGNUM;
 
@@ -736,6 +743,18 @@  show_use_compressed_breakpoints (struct ui_file *file, int from_tty,
 		"to %s.\n"), value);
 }
 
+/* The show callback for 'show riscv numeric-register-names'.  */
+
+static void
+show_numeric_register_names (struct ui_file *file, int from_tty,
+			     struct cmd_list_element *c,
+			     const char *value)
+{
+  gdb_printf (file,
+	      _("Displaying registers with their numeric names is %s.\n"),
+	      value);
+}
+
 /* The set and show lists for 'set riscv' and 'show riscv' prefixes.  */
 
 static struct cmd_list_element *setriscvcmdlist = NULL;
@@ -4832,4 +4851,19 @@  this option can be used."),
 				show_use_compressed_breakpoints,
 				&setriscvcmdlist,
 				&showriscvcmdlist);
+
+  add_setshow_boolean_cmd("numeric-register-names", no_class,
+			  &numeric_register_names,
+			  _("\
+Set displaying registers with numeric names instead of abi names."), _("\
+Show whether registers are displayed with numeric names instead of abi names."),
+			  _("\
+When enabled, registers will be shown with their numeric names (such as x28)\n\
+instead of their abi names (such as t0).\n\
+Also consider using the 'set disassembler-options numeric' command for the\n\
+equivalent change in the disassembler output."),
+			  NULL,
+			  show_numeric_register_names,
+			  &setriscvcmdlist,
+			  &showriscvcmdlist);
 }