Re: [RFC PATCH 2/2] RISC-V: Add -M isa disassembler option

[Palmer Dabbelt <palmer@rivosinc.com>]

On Sat, 05 Feb 2022 02:24:11 PST (-0800), binutils@sourceware.org wrote:
> This commit adds -M isa=ISA disassembler option to objdump and gdb.
> ISA string given has a higher precedence than ELF header but lower than
> BFD architecture with specific XLEN (riscv:rv32 and riscv:rv64).

IMO this should be called arch, not isa.  We use arch everywhere else, 
and while I know it's kind of odd (IIRC earlier version of the toolchain 
even called it "isa") it's better to be consistent.

> opcodes/ChangeLog:
>
> 	* riscv-dis.c (default_isa): Rename from local `default_arch'.
> 	(xlen_set_by_option) New.
> 	(set_default_riscv_dis_options): Initialize default ISA and
> 	RISC-V subset.
> 	(parse_riscv_dis_option): Parse -M isa=ISA option.
> 	(riscv_disassemble_insn): Update xlen setting rules.
> 	(riscv_get_disassembler): Stop setting default ISA here.
> 	Instead, pass default ISA for later initialization.
> 	(riscv_option_arg_t): Add arguments for -M isa=ISA option.
> 	(riscv_options): Add -M isa=ISA option.
> 	(disassembler_options_riscv): Add handling for -M isa=ISA.
> ---
>  opcodes/riscv-dis.c | 32 +++++++++++++++++++++++++-------
>  1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
> index 13ddff01c52..b67597542b5 100644
> --- a/opcodes/riscv-dis.c
> +++ b/opcodes/riscv-dis.c
> @@ -34,8 +34,10 @@
>
>  static enum riscv_spec_class default_isa_spec = ISA_SPEC_CLASS_DRAFT - 1;
>  static enum riscv_spec_class default_priv_spec = PRIV_SPEC_CLASS_NONE;
> +static const char *default_isa = "rv64gc";

That should probably be set by the target we're built for, but it looks 
like that's how this was before so it doesn't need to be fixed now.

>  unsigned xlen = 0;
> +static unsigned xlen_set_by_option = 0;
>
>  static riscv_subset_list_t riscv_subsets;
>  static riscv_parse_subset_t riscv_rps_dis =
> @@ -71,6 +73,9 @@ set_default_riscv_dis_options (void)
>    riscv_gpr_names = riscv_gpr_names_abi;
>    riscv_fpr_names = riscv_fpr_names_abi;
>    no_aliases = 0;
> +  riscv_release_subset_list (&riscv_subsets);
> +  riscv_parse_subset (&riscv_rps_dis, default_isa);
> +  xlen_set_by_option = 0;
>  }
>
>  static bool
> @@ -134,6 +139,12 @@ parse_riscv_dis_option (const char *option)
>  				 option, value, name);
>  	}
>      }
> +  else if (strcmp (option, "isa") == 0)
> +    {
> +      riscv_release_subset_list (&riscv_subsets);
> +      riscv_parse_subset (&riscv_rps_dis, value);
> +      xlen_set_by_option = xlen;
> +    }
>    else
>      {
>        /* xgettext:c-format */
> @@ -592,11 +603,16 @@ riscv_disassemble_insn (bfd_vma memaddr, insn_t word, disassemble_info *info)
>    op = riscv_hash[OP_HASH_IDX (word)];
>    if (op != NULL)
>      {
> -      /* If XLEN is not known, get its value from the ELF class.  */
> +      /* Set XLEN with following precedence rules:
> +	 1. -m riscv:rv[32|64] option (gdb: set arch riscv:rv[32|64])
> +	 2. -M isa option (gdb: set disassembler-options isa=rv[32|64])
> +	 3. ELF class in the ELF header.  */
>        if (info->mach == bfd_mach_riscv64)
>  	xlen = 64;
>        else if (info->mach == bfd_mach_riscv32)
>  	xlen = 32;
> +      else if (xlen_set_by_option)
> +        xlen = xlen_set_by_option;
>        else if (info->section != NULL)
>  	{
>  	  Elf_Internal_Ehdr *ehdr = elf_elfheader (info->section->owner);
> @@ -947,8 +963,6 @@ print_insn_riscv (bfd_vma memaddr, struct disassemble_info *info)
>  disassembler_ftype
>  riscv_get_disassembler (bfd *abfd)
>  {
> -  const char *default_arch = "rv64gc";
> -
>    if (abfd)
>      {
>        const struct elf_backend_data *ebd = get_elf_backend_data (abfd);
> @@ -965,13 +979,10 @@ riscv_get_disassembler (bfd *abfd)
>  						      attr[Tag_b].i,
>  						      attr[Tag_c].i,
>  						      &default_priv_spec);
> -	      default_arch = attr[Tag_RISCV_arch].s;
> +	      default_isa = attr[Tag_RISCV_arch].s;
>  	    }
>  	}
>      }
> -
> -  riscv_release_subset_list (&riscv_subsets);
> -  riscv_parse_subset (&riscv_rps_dis, default_arch);
>    return print_insn_riscv;
>  }
>
> @@ -1000,6 +1011,7 @@ riscv_symbol_is_valid (asymbol * sym,
>  typedef enum
>  {
>    RISCV_OPTION_ARG_NONE = -1,
> +  RISCV_OPTION_ARG_ISA,
>    RISCV_OPTION_ARG_PRIV_SPEC,
>
>    RISCV_OPTION_ARG_COUNT
> @@ -1020,6 +1032,9 @@ static struct
>    { "no-aliases",
>      N_("Disassemble only into canonical instructions."),
>      RISCV_OPTION_ARG_NONE },
> +  { "isa=",
> +    N_("Disassemble using chosen ISA and extensions."),
> +    RISCV_OPTION_ARG_ISA },
>    { "priv-spec=",
>      N_("Print the CSR according to the chosen privilege spec."),
>      RISCV_OPTION_ARG_PRIV_SPEC }
> @@ -1044,6 +1059,9 @@ disassembler_options_riscv (void)
>
>        args = XNEWVEC (disasm_option_arg_t, num_args + 1);
>
> +      args[RISCV_OPTION_ARG_ISA].name = "ISA";
> +      args[RISCV_OPTION_ARG_ISA].values = NULL;
> +
>        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

Otherwise this looks good.  Thanks!