[PATCH] RISC-V: Add support for 'set disassembler-options'
Andrew Burgess
andrew.burgess@embecosm.com
Thu Jan 14 16:00:57 GMT 2021
* Simon Cook <simon.cook@embecosm.com> [2021-01-14 10:52:01 +0000]:
> Implement support for passing options to the disassembler from GDB. This
> supports all options supported by the -M option in objdump, and updates
> the RISC-V disassembler to support resetting options back to the default
> should the user later request no options.
>
> The disassembler now holds a table of supported options and arguments.
> Option printing (for objdump --help) has been updated to use this table;
> this implementation is based on the MIPS disassembler options.
>
> gdb/ChangeLog:
>
> * riscv-tdep.c (riscv_gdbarch_init): Add disassembler options.
Missing an entry for `riscv_disassembler_options` here.
>
> gdb/testsuite/ChangeLog:
>
> * gdb.arch/riscv-disassembler-options.exp: New test.
> * gdb.arch/riscv-disassembler-options.s: New test source.
The gdb/ parts are approved. I can't approve include/ or opcodes/,
however I did have some feedback....
>
> include/Changelog:
>
> * dis-asm.h (disassembler_options_riscv): New prototype.
>
> opcodes/ChangeLog:
>
> * riscv-dis.c (init_csr): New variable.
> (set_default_riscv_dis_options): Reset default_priv_spec and
> init_csr.
> (print_insn_args): Remove init_csr from function.
> (previous_disassembler_options): New variable.
> (print_insn_riscv): Add parsing of updated disassembler
> options.
> (riscv_options): New variable.
> (riscv_options_args_privspec): New variable.
> (riscv_option_args): New variable.
> (disassembler_options_riscv): New function.
> (print_riscv_disassembler_options): Reimplement using
> disassembler_options_riscv.
> ---
> gdb/riscv-tdep.c | 8 +
> .../gdb.arch/riscv-disassembler-options.exp | 57 ++++++
> .../gdb.arch/riscv-disassembler-options.s | 29 +++
> include/dis-asm.h | 1 +
> opcodes/riscv-dis.c | 169 ++++++++++++++++--
> 5 files changed, 251 insertions(+), 13 deletions(-)
> create mode 100644 gdb/testsuite/gdb.arch/riscv-disassembler-options.exp
> create mode 100644 gdb/testsuite/gdb.arch/riscv-disassembler-options.s
>
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index c0e84e585d..74aeaca518 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -94,6 +94,9 @@ static unsigned int riscv_debug_unwinder = 0;
>
> static unsigned int riscv_debug_gdbarch = 0;
>
> +/* Options to pass to disassembler. */
> +static char *riscv_disassembler_options = NULL;
> +
> /* Cached information about a frame. */
>
> struct riscv_unwind_cache
> @@ -3636,6 +3639,11 @@ riscv_gdbarch_init (struct gdbarch_info info,
>
> register_riscv_ravenscar_ops (gdbarch);
>
> + /* Disassembler options. */
> + set_gdbarch_disassembler_options (gdbarch, &riscv_disassembler_options);
> + set_gdbarch_valid_disassembler_options (gdbarch,
> + disassembler_options_riscv ());
> +
> return gdbarch;
> }
>
> diff --git a/gdb/testsuite/gdb.arch/riscv-disassembler-options.exp b/gdb/testsuite/gdb.arch/riscv-disassembler-options.exp
> new file mode 100644
> index 0000000000..c118d96113
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/riscv-disassembler-options.exp
> @@ -0,0 +1,57 @@
> +# Copyright 2021 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +# Test RISC-V disassembler options.
> +
> +if { ![istarget "riscv*-*-*"] } then {
> + verbose "Skipping RISC-V disassembler option tests."
> + return
> +}
> +
> +standard_testfile .s
> +set objfile [standard_output_file ${testfile}.o]
> +
> +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${objfile}" object {}] \
> + != "" } {
> + return
> +}
> +
> +clean_restart ${objfile}
> +
> +proc riscv_disassemble_test { func insn mesg } {
> + gdb_test "disassemble $func" \
> + "Dump of assembler code for function\
> + $func:\r\n\[^:\]+:\t$insn\r\nEnd of assembler dump\." \
> + $mesg
> +}
> +
> +# Verify defaults.
> +riscv_disassemble_test foo "nop" "disassemble implicit default"
> +
> +# Verify option overrides.
> +gdb_test "set disassembler-options no-aliases"
> +riscv_disassemble_test foo "c.addi\tzero,0" "disassemble no-aliases"
> +gdb_test "set disassembler-options no-aliases,numeric"
> +riscv_disassemble_test foo "c.addi\tx0,0" "disassemble no-aliases,numeric"
> +
> +# Verify switching of priviledge spec.
> +gdb_test "set disassembler-options priv-spec=1.9.1"
> +riscv_disassemble_test bar "csrr\tt6,mucounteren" "disassemble priv-spec=1.9.1"
> +gdb_test "set disassembler-options priv-spec=1.11"
> +riscv_disassemble_test bar "csrr\tt6,mcountinhibit" "disassemble priv-spec=1.11"
> +
> +# Verify can roll back to defaults.
> +gdb_test "set disassembler-options"
> +riscv_disassemble_test foo "nop" "disassemble explicit default"
> diff --git a/gdb/testsuite/gdb.arch/riscv-disassembler-options.s b/gdb/testsuite/gdb.arch/riscv-disassembler-options.s
> new file mode 100644
> index 0000000000..bc337556bd
> --- /dev/null
> +++ b/gdb/testsuite/gdb.arch/riscv-disassembler-options.s
> @@ -0,0 +1,29 @@
> +# This test is part of GDB, the GNU debugger.
> +#
> +# Copyright 2021 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program. If not, see <http://www.gnu.org/licenses/>.
> +
> +
> + .globl foo
> + .option rvc
> +foo:
> + c.nop
> + .size foo, .-foo
> +
> +# 0x320 is MUCOUNTEREN in 1.9.1 MCOUNTINHIBIT in 1.11
> + .globl bar
> +bar:
> + csrr x31,0x320
> + .size bar, .-bar
> diff --git a/include/dis-asm.h b/include/dis-asm.h
> index 368eb2734f..4b06e0856e 100644
> --- a/include/dis-asm.h
> +++ b/include/dis-asm.h
> @@ -315,6 +315,7 @@ 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_s390 (void);
> +extern const disasm_options_and_args_t *disassembler_options_riscv (void);
>
> /* Fetch the disassembler for a given architecture ARC, endianess (big
> endian if BIG is true), bfd_mach value MACH, and ABFD, if that support
> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
> index c3d9bb8745..b49f565663 100644
> --- a/opcodes/riscv-dis.c
> +++ b/opcodes/riscv-dis.c
> @@ -31,6 +31,7 @@
>
> #include "bfd_stdint.h"
> #include <ctype.h>
> +#include <limits.h>
>
> static enum riscv_priv_spec_class default_priv_spec = PRIV_SPEC_CLASS_NONE;
>
> @@ -47,9 +48,14 @@ static const char * const *riscv_fpr_names;
> /* Other options. */
> static int no_aliases; /* If set disassemble as most general inst. */
>
> +/* If false csr table needs (re-)initializing. */
> +static bfd_boolean init_csr = FALSE;
> +
> static void
> set_default_riscv_dis_options (void)
> {
> + default_priv_spec = PRIV_SPEC_CLASS_NONE;
> + init_csr = FALSE;
> riscv_gpr_names = riscv_gpr_names_abi;
> riscv_fpr_names = riscv_fpr_names_abi;
> no_aliases = 0;
> @@ -372,7 +378,6 @@ print_insn_args (const char *d, insn_t l, bfd_vma pc, disassemble_info *info)
> case 'E':
> {
> static const char *riscv_csr_hash[4096]; /* Total 2^12 CSR. */
> - static bfd_boolean init_csr = FALSE;
> unsigned int csr = EXTRACT_OPERAND (CSR, l);
>
> if (!init_csr)
> @@ -554,6 +559,9 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info)
> return insnlen;
> }
>
> +/* Disassembler option string that has previously been parsed. */
> +static char *previous_disassembler_options = NULL;
> +
> int
> print_insn_riscv (bfd_vma memaddr, struct disassemble_info *info)
> {
> @@ -562,11 +570,26 @@ print_insn_riscv (bfd_vma memaddr, struct disassemble_info *info)
> bfd_vma n;
> int status;
>
> - if (info->disassembler_options != NULL)
> + /* If the current disassembler options are not the same as those used
> + previously, reset disassembler options and parse the new string.
> + NULL is used as an empty string, so treat NULL and NULL as equal. */
> + if ((info->disassembler_options != previous_disassembler_options &&
> + (!info->disassembler_options || !previous_disassembler_options)) ||
> + (info->disassembler_options && previous_disassembler_options &&
> + strcmp(info->disassembler_options, previous_disassembler_options)))
GNU style places the operators at the start of each line, so this
should be:
if ((info->disassembler_options != previous_disassembler_options
&& (!info->disassembler_options || !previous_disassembler_options))
|| (info->disassembler_options && previous_disassembler_options
&& strcmp(info->disassembler_options,
previous_disassembler_options)))
> {
> - parse_riscv_dis_options (info->disassembler_options);
> - /* Avoid repeatedly parsing the options. */
> - info->disassembler_options = NULL;
> + if (previous_disassembler_options)
In other places in this function the char* pointers are explicitly
compared to NULL, while this use is different. If might be nice to be
consistent?
> + {
> + free (previous_disassembler_options);
> + previous_disassembler_options = NULL;
> + }
> + set_default_riscv_dis_options();
> + if (info->disassembler_options != NULL)
> + {
> + parse_riscv_dis_options (info->disassembler_options);
> + /* Avoid repeatedly parsing the options. */
> + previous_disassembler_options = xstrdup(info->disassembler_options);
Missing a space after xstrdup.
> + }
> }
> else if (riscv_gpr_names == NULL)
> set_default_riscv_dis_options ();
> @@ -630,23 +653,143 @@ riscv_symbol_is_valid (asymbol * sym,
> return (strcmp (name, RISCV_FAKE_LABEL_NAME) != 0);
> }
>
> +/* Valid RISC-V diassembler options. */
> +static struct
> +{
> + const char *name;
> + const char *description;
> + /* This corresponds to the row in RISCV_OPTION_ARGS holding valid arguments
> + for this option. */
> + unsigned arg;
> +} riscv_options[] =
> +{
> + { "no-aliases", "Disassemble only into canonical instructions, rather than \
> +into pseudoinstructions.\n", UINT_MAX },
> + { "numeric", "Print numeric register names, rather than ABI names.\n", UINT_MAX },
> + { "priv-spec=", "Print the CSRs according to the chosen privilege spec \
> +(1.9.1, 1.10, 1.11).", 0}
I believe that you need to wrap the descriptions here in `N_(...)`
like:
{ "no-aliases", N_("Disassemble only into canonical instructions, rather than \
into pseudoinstructions.\n"), UINT_MAX },
So that the strings will be found and marked for translation. The
_(...) call you have below is only responsible for looking up the
translated strings.
> +};
> +
> +/* Valid RISC-V disassembler arguments. Note value lists must be NULL
> + terminated. */
> +static const char *
> +riscv_option_args_privspec[] = {"1.9.1", "1.10", "1.11", NULL};
I think the correct GNU style would be:
static const char *riscv_option_args_privspec[] = {
"1.9.1", "1.10", "1.11", NULL
};
> +static struct
> +{
> + const char *name;
> + const char **values;
> +} riscv_option_args[] =
> +{
> + {"PRIV", (const char**)&riscv_option_args_privspec}
> +};
> +
> +const disasm_options_and_args_t *
> +disassembler_options_riscv (void)
> +{
> + static disasm_options_and_args_t *opts_and_args;
> +
> + if (opts_and_args == NULL)
> + {
> + unsigned num_options = ARRAY_SIZE(riscv_options);
> + unsigned num_args = ARRAY_SIZE(riscv_option_args);
Whitespace after ARRAY_SIZE.
> + disasm_option_arg_t *args;
> + disasm_options_t *opts;
> + unsigned i;
> +
> + args = XNEWVEC (disasm_option_arg_t, num_args + 1);
> +
> + /* Array must be NULL terminated. */
> + args[num_args].name = NULL;
> + args[num_args].values = NULL;
> +
> + for (i = 0; i < num_args; i++)
> + {
> + args[i].name = riscv_option_args[i].name;
> + args[i].values = riscv_option_args[i].values;
> + }
> +
> + 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);
> +
> + /* Last option must be NULL */
> + opts->name[num_options] = NULL;
> + opts->description[num_options] = NULL;
> + opts->arg[num_options] = NULL;
> +
> + 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 != UINT_MAX)
> + opts->arg[i] = &args[riscv_options[i].arg];
> + else
> + 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\
> with the -M switch (multiple options should be separated by commas):\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[i] != NULL)
> + len += strlen (opts->arg[i]->name);
> + fprintf (stream,
> + "%*c %s", (int) (max_len - len), ' ', opts->description[i]);
> + }
> + fprintf (stream, _("\n"));
I'm not sure you need to translate newlines.
Thanks,
Andrew
> + }
> +
> + 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.24.3 (Apple Git-128)
>
More information about the Binutils
mailing list