Re: [RFC PATCH 3/5] RISC-V: Mapping symbols with ISA string on assembler

[Jan Beulich <jbeulich@suse.com>]

On 11.08.2022 09:00, Tsukasa OI via Binutils wrote:
> @@ -3855,11 +3887,15 @@ s_riscv_option (int x ATTRIBUTE_UNUSED)
>    if (strcmp (name, "rvc") == 0)
>      {
>        riscv_update_subset (&riscv_rps_as, "+c");
> +      updated_riscv_subsets = true;
> +      riscv_opts.arch_is_default = false;
>        riscv_set_rvc (true);
>      }
>    else if (strcmp (name, "norvc") == 0)
>      {
>        riscv_update_subset (&riscv_rps_as, "-c");
> +      updated_riscv_subsets = true;
> +      riscv_opts.arch_is_default = false;
>        riscv_set_rvc (false);
>      }
>    else if (strcmp (name, "pic") == 0)
> @@ -3880,6 +3916,8 @@ s_riscv_option (int x ATTRIBUTE_UNUSED)
>        if (ISSPACE (*name) && *name != '\0')
>  	name++;
>        riscv_update_subset (&riscv_rps_as, name);
> +      updated_riscv_subsets = true;
> +      riscv_opts.arch_is_default = false;

Seeing that all three call sites of riscv_update_subset() gain the
same extra code - wouldn't these assignments better move into that
function? (The function living in bfd may make this difficult, but
it being used by gas only suggests it might better be moved over. Or
otherwise maybe add a local helper function doing all three things?)

As to the resulting "suffix" to $x - is this then intended to be in
strictly canonical form?

> --- a/opcodes/riscv-dis.c
> +++ b/opcodes/riscv-dis.c
> @@ -750,7 +750,7 @@ riscv_get_map_state (int n,
>      return false;
>  
>    name = bfd_asymbol_name(info->symtab[n]);
> -  if (strcmp (name, "$x") == 0)
> +  if (strncmp (name, "$x", 2) == 0)

Nit: The assembler was switched to use startswith() in similar
cases.

Jan