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

[Palmer Dabbelt <palmer@dabbelt.com>]

On Mon, 09 Jan 2023 09:07:06 PST (-0800), jbeulich@suse.com wrote:
> 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?

Thanks for fixing this.  I don't have any issues with what's there, but 
looks like I'm also getting some failures (glibc/multilib errno related 
stuff).  I'm trying to bisect those so I can't really get a proper test 
up now, I'll try to do so ASAP as it's really late to have stuff broken.

> 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