[PATCH 0/2] RISC-V Disassembler Options Support

[PATCH 0/2] RISC-V Disassembler Options Support

[Andrew Burgess]

This series is a refresh of this patch that was never merged:

  https://sourceware.org/pipermail/binutils/2021-January/114944.html

I actually created this series from scratch, copying code from the
MIPS implementation, and it was only when I was reviewing the change
prior to posting that it all seemed very familiar, and I remembered
Simon's earlier work.

I believe I've addressed the review feedback from the earlier thread,
except that I have retained the help text that lists the valid
priv-spec values - not printing the valid values seems like a really
bad idea to me, so I'd prefer we kept that in.

All feedback welcome.

Thanks,
Andrew

---

Andrew Burgess (2):
  opcodes/riscv: add disassembler options support to libopcodes
  gdb: add risc-v disassembler options support

 gdb/riscv-tdep.c    |   8 +++
 include/ChangeLog   |   5 ++
 include/dis-asm.h   |   1 +
 opcodes/ChangeLog   |   9 +++
 opcodes/riscv-dis.c | 147 +++++++++++++++++++++++++++++++++++++++++---
 5 files changed, 161 insertions(+), 9 deletions(-)

-- 
2.25.4

[PATCH 1/2] opcodes/riscv: add disassembler options support to libopcodes

[Andrew Burgess]

From: Andrew Burgess <andrew.burgess@embecosm.com>

In preparation for the next commit, which will add GDB support for
RISC-V disassembler options, this commit restructures how the
disassembler options are managed within libopcodes.

The implementation provided here is based on this mailing list patch
which was never committed:

  https://sourceware.org/pipermail/binutils/2021-January/114944.html

which in turn took inspiration from the MIPS implementation of the
same feature.

The biggest changes from the original mailing list post are:

  1. The GDB changes have been split into a separate patch, and

  2. The `riscv_option_args_privspec` variable, which held the valid
  priv-spec values is now gone, instead we use the `riscv_priv_specs`
  array from bfd/cpu-riscv.c instead.

Co-authored-by: Simon Cook <simon.cook@embecosm.com>

include/ChangeLog:

	* dis-asm.h (disassembler_options_riscv): Declare.

opcodes/ChangeLog:

	* riscv-dis.c (enum riscv_option_arg_t): New enum typedef.
	(riscv_options): New static global.
	(disassembler_options_riscv): New function.
	(print_riscv_disassembler_options): Rewrite to use
	disassembler_options_riscv.
---
 include/ChangeLog   |   5 ++
 include/dis-asm.h   |   1 +
 opcodes/ChangeLog   |   9 +++
 opcodes/riscv-dis.c | 147 +++++++++++++++++++++++++++++++++++++++++---
 4 files changed, 153 insertions(+), 9 deletions(-)

diff --git a/include/ChangeLog b/include/ChangeLog
index 52483238ffb..c0486e9acf9 100644
--- a/include/ChangeLog
+++ b/include/ChangeLog
@@ -1,3 +1,8 @@
+2021-11-25  Andrew Burgess  <aburgess@redhat.com>
+	    Simon Cook  <simon.cook@embecosm.com>
+
+	* dis-asm.h (disassembler_options_riscv): Declare.
+
 2021-11-16  Fangrui Song  <maskray@google.com>
 
 	* elf/common.h (DT_ENCODING): Bump to 38.
diff --git a/include/dis-asm.h b/include/dis-asm.h
index c0bc1d542cf..81cefb94b2b 100644
--- a/include/dis-asm.h
+++ b/include/dis-asm.h
@@ -320,6 +320,7 @@ extern const disasm_options_and_args_t *disassembler_options_arc (void);
 extern const disasm_options_and_args_t *disassembler_options_arm (void);
 extern const disasm_options_and_args_t *disassembler_options_mips (void);
 extern const disasm_options_and_args_t *disassembler_options_powerpc (void);
+extern const disasm_options_and_args_t *disassembler_options_riscv (void);
 extern const disasm_options_and_args_t *disassembler_options_s390 (void);
 
 /* Fetch the disassembler for a given architecture ARC, endianess (big
diff --git a/opcodes/ChangeLog b/opcodes/ChangeLog
index 1e605183bc4..7bf0094cf0d 100644
--- a/opcodes/ChangeLog
+++ b/opcodes/ChangeLog
@@ -1,3 +1,12 @@
+2021-11-25  Andrew Burgess  <aburgess@redhat.com>
+	    Simon Cook  <simon.cook@embecosm.com>
+
+	* riscv-dis.c (enum riscv_option_arg_t): New enum typedef.
+	(riscv_options): New static global.
+	(disassembler_options_riscv): New function.
+	(print_riscv_disassembler_options): Rewrite to use
+	disassembler_options_riscv.
+
 2021-11-25  Nick Clifton  <nickc@redhat.com>
 
 	PR 28614
diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index acb84712a7e..18e498a0f22 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -990,24 +990,153 @@ riscv_symbol_is_valid (asymbol * sym,
   return (strcmp (name, RISCV_FAKE_LABEL_NAME) != 0
 	  && !riscv_elf_is_mapping_symbols (name));
 }
+\f
+
+/* Indices into option argument vector for options accepting an argument.
+   Use RISCV_OPTION_ARG_NONE for options accepting no argument.  */
+
+typedef enum
+{
+  RISCV_OPTION_ARG_NONE = -1,
+  RISCV_OPTION_ARG_PRIV_SPEC,
+
+  RISCV_OPTION_ARG_COUNT
+} riscv_option_arg_t;
+
+/* Valid RISCV disassembler options.  */
+
+static struct
+{
+  const char *name;
+  const char *description;
+  riscv_option_arg_t arg;
+} riscv_options[] =
+{
+  { "numeric",
+    N_("Print numeric register names, rather than ABI names."),
+    RISCV_OPTION_ARG_NONE },
+  { "no-aliases",
+    N_("Disassemble only into canonical instructions."),
+    RISCV_OPTION_ARG_NONE },
+  { "priv-spec=",
+    N_("Print the CSR according to the chosen privilege spec."),
+    RISCV_OPTION_ARG_PRIV_SPEC }
+};
+
+/* Build the structure representing valid RISCV disassembler options.
+   This is done dynamically for maintenance ease purpose; a static
+   initializer would be unreadable.  */
+
+const disasm_options_and_args_t *
+disassembler_options_riscv (void)
+{
+  static disasm_options_and_args_t *opts_and_args;
+
+  if (opts_and_args == NULL)
+    {
+      size_t num_options = ARRAY_SIZE (riscv_options);
+      size_t num_args = RISCV_OPTION_ARG_COUNT;
+      disasm_option_arg_t *args;
+      disasm_options_t *opts;
+      size_t i, priv_spec_count;
+
+      args = XNEWVEC (disasm_option_arg_t, num_args + 1);
+
+      args[RISCV_OPTION_ARG_PRIV_SPEC].name = "SPEC";
+      priv_spec_count = PRIV_SPEC_CLASS_DRAFT - PRIV_SPEC_CLASS_NONE - 1;
+      args[RISCV_OPTION_ARG_PRIV_SPEC].values
+        = XNEWVEC (const char *, priv_spec_count + 1);
+      for (i = 0; i < priv_spec_count; i++)
+	args[RISCV_OPTION_ARG_PRIV_SPEC].values[i]
+          = riscv_priv_specs[i].name;
+      /* The array we return must be NULL terminated.  */
+      args[RISCV_OPTION_ARG_PRIV_SPEC].values[i] = NULL;
+
+      /* The array we return must be NULL terminated.  */
+      args[num_args].name = NULL;
+      args[num_args].values = NULL;
+
+      opts_and_args = XNEW (disasm_options_and_args_t);
+      opts_and_args->args = args;
+
+      opts = &opts_and_args->options;
+      opts->name = XNEWVEC (const char *, num_options + 1);
+      opts->description = XNEWVEC (const char *, num_options + 1);
+      opts->arg = XNEWVEC (const disasm_option_arg_t *, num_options + 1);
+      for (i = 0; i < num_options; i++)
+	{
+	  opts->name[i] = riscv_options[i].name;
+	  opts->description[i] = _(riscv_options[i].description);
+	  if (riscv_options[i].arg != RISCV_OPTION_ARG_NONE)
+	    opts->arg[i] = &args[riscv_options[i].arg];
+	  else
+	    opts->arg[i] = NULL;
+	}
+      /* The array we return must be NULL terminated.  */
+      opts->name[i] = NULL;
+      opts->description[i] = NULL;
+      opts->arg[i] = NULL;
+    }
+
+  return opts_and_args;
+}
 
 void
 print_riscv_disassembler_options (FILE *stream)
 {
+  const disasm_options_and_args_t *opts_and_args;
+  const disasm_option_arg_t *args;
+  const disasm_options_t *opts;
+  size_t max_len = 0;
+  size_t i;
+  size_t j;
+
+  opts_and_args = disassembler_options_riscv ();
+  opts = &opts_and_args->options;
+  args = opts_and_args->args;
+
   fprintf (stream, _("\n\
-The following RISC-V-specific disassembler options are supported for use\n\
+The following RISC-V specific disassembler options are supported for use\n\
 with the -M switch (multiple options should be separated by commas):\n"));
+  fprintf (stream, "\n");
 
-  fprintf (stream, _("\n\
-  numeric         Print numeric register names, rather than ABI names.\n"));
+  /* Compute the length of the longest option name.  */
+  for (i = 0; opts->name[i] != NULL; i++)
+    {
+      size_t len = strlen (opts->name[i]);
 
-  fprintf (stream, _("\n\
-  no-aliases      Disassemble only into canonical instructions, rather\n\
-                  than into pseudoinstructions.\n"));
+      if (opts->arg[i] != NULL)
+	len += strlen (opts->arg[i]->name);
+      if (max_len < len)
+	max_len = len;
+    }
 
-  fprintf (stream, _("\n\
-  priv-spec=PRIV  Print the CSR according to the chosen privilege spec\n\
-                  (1.9, 1.9.1, 1.10, 1.11).\n"));
+  for (i = 0, max_len++; opts->name[i] != NULL; i++)
+    {
+      fprintf (stream, "  %s", opts->name[i]);
+      if (opts->arg[i] != NULL)
+	fprintf (stream, "%s", opts->arg[i]->name);
+      if (opts->description[i] != NULL)
+	{
+	  size_t len = strlen (opts->name[i]);
+
+	  if (opts->arg != NULL && opts->arg[i] != NULL)
+	    len += strlen (opts->arg[i]->name);
+	  fprintf (stream, "%*c %s", (int) (max_len - len), ' ',
+                   opts->description[i]);
+	}
+      fprintf (stream, "\n");
+    }
+
+  for (i = 0; args[i].name != NULL; i++)
+    {
+      fprintf (stream, _("\n\
+  For the options above, the following values are supported for \"%s\":\n   "),
+	       args[i].name);
+      for (j = 0; args[i].values[j] != NULL; j++)
+	fprintf (stream, " %s", args[i].values[j]);
+      fprintf (stream, _("\n"));
+    }
 
   fprintf (stream, _("\n"));
 }
-- 
2.25.4

[PATCH 2/2] gdb: add risc-v disassembler options support

[Andrew Burgess]

From: Andrew Burgess <andrew.burgess@embecosm.com>

This commit adds support for RISC-V disassembler options to GDB.  This
commit is based on this patch which was never committed:

  https://sourceware.org/pipermail/binutils/2021-January/114944.html

All of the binutils refactoring has been moved to a separate, earlier,
commit, so this commit is pretty straight forward, just registering
the required gdbarch hooks.

Co-authored-by: Simon Cook <simon.cook@embecosm.com>
---
 gdb/riscv-tdep.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index 94dad41a9fd..4d87a899063 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -101,6 +101,9 @@ static const char *riscv_feature_name_fpu = "org.gnu.gdb.riscv.fpu";
 static const char *riscv_feature_name_virtual = "org.gnu.gdb.riscv.virtual";
 static const char *riscv_feature_name_vector = "org.gnu.gdb.riscv.vector";
 
+/* The current set of options to be passed to the disassembler.  */
+static char *riscv_disassembler_options;
+
 /* Cached information about a frame.  */
 
 struct riscv_unwind_cache
@@ -3803,6 +3806,11 @@ riscv_gdbarch_init (struct gdbarch_info info,
   set_gdbarch_gcc_target_options (gdbarch, riscv_gcc_target_options);
   set_gdbarch_gnu_triplet_regexp (gdbarch, riscv_gnu_triplet_regexp);
 
+  /* Disassembler options support.  */
+  set_gdbarch_valid_disassembler_options (gdbarch,
+					  disassembler_options_riscv ());
+  set_gdbarch_disassembler_options (gdbarch, &riscv_disassembler_options);
+
   /* Hook in OS ABI-specific overrides, if they have been registered.  */
   gdbarch_init_osabi (info, gdbarch);
 
-- 
2.25.4

Re: [PATCH 0/2] RISC-V Disassembler Options Support

[Nelson Chu]

Hi Andrew,

I remember my previous comment was that - we should get the privileged spec
names or classes from the bfd/cpu-riscv.h.  And the rewrite patches do it,
so LGTM.  Please commit them when you think it is time.

Thanks
Nelson

Andrew Burgess <aburgess@redhat.com>：

> This series is a refresh of this patch that was never merged:
>
>   https://sourceware.org/pipermail/binutils/2021-January/114944.html
>
> I actually created this series from scratch, copying code from the
> MIPS implementation, and it was only when I was reviewing the change
> prior to posting that it all seemed very familiar, and I remembered
> Simon's earlier work.
>
> I believe I've addressed the review feedback from the earlier thread,
> except that I have retained the help text that lists the valid
> priv-spec values - not printing the valid values seems like a really
> bad idea to me, so I'd prefer we kept that in.
>
> All feedback welcome.
>
> Thanks,
> Andrew
>
> ---
>
> Andrew Burgess (2):
>   opcodes/riscv: add disassembler options support to libopcodes
>   gdb: add risc-v disassembler options support
>
>  gdb/riscv-tdep.c    |   8 +++
>  include/ChangeLog   |   5 ++
>  include/dis-asm.h   |   1 +
>  opcodes/ChangeLog   |   9 +++
>  opcodes/riscv-dis.c | 147 +++++++++++++++++++++++++++++++++++++++++---
>  5 files changed, 161 insertions(+), 9 deletions(-)
>
> --
> 2.25.4
>
>

Re: [PATCH 0/2] RISC-V Disassembler Options Support

[Andrew Burgess]

* Nelson Chu <nelson.chu@sifive.com> [2021-11-26 15:48:56 +0800]:

> Hi Andrew,
> 
> I remember my previous comment was that - we should get the privileged spec
> names or classes from the bfd/cpu-riscv.h.  And the rewrite patches do it,
> so LGTM.  Please commit them when you think it is time.

Thanks, I've now pushed both these patches.

Andrew



> 
> Thanks
> Nelson
> 
> Andrew Burgess <aburgess@redhat.com>：
> 
> > This series is a refresh of this patch that was never merged:
> >
> >   https://sourceware.org/pipermail/binutils/2021-January/114944.html
> >
> > I actually created this series from scratch, copying code from the
> > MIPS implementation, and it was only when I was reviewing the change
> > prior to posting that it all seemed very familiar, and I remembered
> > Simon's earlier work.
> >
> > I believe I've addressed the review feedback from the earlier thread,
> > except that I have retained the help text that lists the valid
> > priv-spec values - not printing the valid values seems like a really
> > bad idea to me, so I'd prefer we kept that in.
> >
> > All feedback welcome.
> >
> > Thanks,
> > Andrew
> >
> > ---
> >
> > Andrew Burgess (2):
> >   opcodes/riscv: add disassembler options support to libopcodes
> >   gdb: add risc-v disassembler options support
> >
> >  gdb/riscv-tdep.c    |   8 +++
> >  include/ChangeLog   |   5 ++
> >  include/dis-asm.h   |   1 +
> >  opcodes/ChangeLog   |   9 +++
> >  opcodes/riscv-dis.c | 147 +++++++++++++++++++++++++++++++++++++++++---
> >  5 files changed, 161 insertions(+), 9 deletions(-)
> >
> > --
> > 2.25.4
> >
> >