Re: [PATCH] gas/RISC-V: adjust assembler for opcode table re-ordering

[Jan Beulich <jbeulich@suse.com>]

On 08.01.2023 00:29, Aurelien Jarno wrote:
> On 2023-01-06 13:34, Jan Beulich via Binutils wrote:
>> PR gas/29940
>>
>> With the single-operand JAL entry now sitting ahead of the two-operand
>> one, the parsing of a two-operand insn would first try to parse an 'a'-
>> style operand, resulting in the insertion of bogus (and otherwise
>> unused) undefined symbols in the symbol table, having register names.
>> Since 'a' is used as 1st operand only with J and JAL, and since JAL is
>> the only insn _also_ allowing for a register as 1st operand (and then
>> there being a 2nd one), special case this parsing aspect right there.
>> ---
>> This, of course, is fragile, but I guess such workarounds are
>> unavoidable with the chosen approach of (recurring) parsing, and with
>> register names being special only in certain contexts.
>>
>> A more generic approach, then possibly also helping performance, might
>> be to count the number of operands first, and do full parsing only when
>> the count matches that in the operand specifier string (at least when
>> there are multiple insn forms).
>>
>> The similar workaround in my_getSmallExpression() actually looks
>> suspicious to me: I expect that it would get in the way of using equates
>> "shadowing" names of GPRs.
>>
>> --- a/gas/config/tc-riscv.c
>> +++ b/gas/config/tc-riscv.c
>> @@ -3266,6 +3266,17 @@ riscv_ip (char *str, struct riscv_cl_ins
>>  	      continue;
>>  
>>  	    case 'a': /* 20-bit PC-relative offset.  */
>> +	      /* Like in my_getSmallExpression() we need to avoid emitting
>> +		 a stray undefined symbol if the 1st JAL entry doesn't match,
>> +		 but the 2nd (with 2 operands) might.  */
>> +	      if (oparg == insn->args)
>> +		{
>> +		  asargStart = asarg;
>> +		  if (reg_lookup (&asarg, RCLASS_GPR, NULL)
>> +		      && (*asarg == ',' || (ISSPACE (*asarg) && asarg[1] == ',')))
>> +		    break;
>> +		  asarg = asargStart;
>> +		}
>>  	    jump:
>>  	      my_getExpression (imm_expr, asarg);
>>  	      asarg = expr_end;
> 
> Thanks for the patch. I have tested it and confirmed it fix the problem
> I reported.

With 2.40 scheduled to be cut in less than a week, may I ask for an arch
maintainer's view here?

Nick, to save a round trip, could you confirm (or otherwise) that this
fix is then also fine to go on the branch (after having gone into master)?

Thanks, Jan