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

[Tsukasa OI <research_trasio@irq.a4lg.com>]

On 2022/08/11 16:31, Jan Beulich wrote:
> 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?)

Adding a local helper function would be the best.

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

I'm not sure but both mine and Nelson's generate "$x${CANONICAL}".

> 
>> --- 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.

I'll fix this in the next version of the patch (I'll also fix PATCH 4/5).

Thanks,
Tsukasa

> 
> Jan
>