[PATCH v2 15/17] MIPS: relax gas testsuite whitespace expectations

[Jan Beulich]

On 17.07.2024 02:09, Maciej W. Rozycki wrote:
> On Mon, 15 Jul 2024, Jan Beulich wrote:
>>>> ---
>>>> This is adding just the blanks that are going to be needed; imo it would
>>>> generally be better if test case expectations were, from the very
>>>> beginning, written to focus on what is being tested, rather than taking
>>>> verbatim copies of the respective tool's output.
>>>
>>>  I'm sorry, how could the test case writer predict that extra whitespace 
>>> might be added in the future?
>>
>> This is the wrong question to ask, imo. There's no whitespace being
>> "added". The issue is with listing tests which take verbatim the listing
>> output, when really they should further consider the original input to
>> gas. In principle any whitespace there may or may not survive scrubbing.
> 
>  Fair enough, but see my next reply below.
> 
>>>  This statement makes no sense to me, this 
>>> way all tests should be rewritten such as to possibly expect whitespace 
>>> between tokens where there currently isn't any.
>>
>> Listing tests should indeed have been written like this, imo.
> 
>  It is often impractical and such listings are typically obtained from the 
> tool directly, examined to verify that they match expectations, and then 
> possibly tweaked a little by hand.  So there's usually little actual 
> "writing" involved.

I certainly understand why listing tests are done the way they are. It's
much easier and faster to simply take the output of a tool. Yet that's
my whole point: Doing so is deferring the work to someone else, later on.
It's not the first time I'm ending up as the one paying the price.

>>>  Also I think if you retain whitespace, you should be doing this 
>>> consistently, i.e. if the source instruction is say:
>>>
>>> 	dext	$2, $3, 1 << 2, 1 << 4
>>>
>>> then in error reporting this should be used:
>>>
>>> dext $2, $3, 1 << 2, 1 << 4
>>>
>>> rather than this:
>>>
>>> dext $2,$3,1 << 2,1 << 4
>>>
>>> which makes the instruction reported hard to read/parse to a human, as you 
>>> suddenly have whitespace within operands, but not between them.  I guess 
>>> this is something to handle in 17/17.
>>
>> No. I'm very deliberately special casing commas there, or else I'd have
>> to make yet more adjustments to the testsuite. It's already bad enough.
>> Plus who knows how many more target specific parsing adjustments I'd need
>> to make, to deal with the further "surviving" whitespace.
> 
>  Understood, that's a practical argument.  I wonder if it would make sense 
> to do additional scrubbing just for reporting.

That's certainly a possibility. But see below.

>> (As an aside, I'm also having trouble seeing why the whole insn, with all
>> operands, would need quoting in a diagnostic. There may be rare cases
>> whether this is necessary for context, but usually it should be sufficient
>> to mention just the part that's actually wrong.)
> 
>  It would require extra processing.  GAS doesn't actually know which part 
> of the instruction is wrong: it's got rejected by opcodes, because it 
> didn't match any existing instruction+operand set pattern.

That's a MIPS-specific thing then, I suppose? x86 for example parses
operands all in gas, and each one separately. And it goes to some lengths
to provide useful (read: as specific as possible) diagnostics. Which
isn't to say there wouldn't still be a lot of room for improvement.

Yet then, looking at tc-mips.c, I can't help getting the impression that
arguments are parsed individually there, too. For the result to then be
handed to match_insns().

>  It does know 
> if it was the mnemonic or the operands and reports that with the message 
> preceding the instruction quoted, but I think there's little if any value 
> in removing parts of the instruction.  Contrariwise, I think that would 
> actually only obfuscate things.
> 
>  And then even if GAS knew the details of the problematic operands there 
> may be ambiguity, such as where each of the operands might be OK as long 
> as the other one was different.
> 
>  Consider this instruction:
> 
> 	bltc	$2, $2, .
> 
> $ mips-linux-gnu-as -mips32r6 -o bltc.o bltc.s
> bltc.s: Assembler messages:
> bltc.s:1: Error: invalid operands `bltc $2,$2,.'
> $ 
> 
> It gets rejected because both register operands refer to the same 
> register, in which case it would be a different machine instruction if 
> that was actually let through and encoded (i.e. "bltzc $2, .").
> 
>  But there's nothing wrong with either operand, it's the combination of 
> the operands that doesn't work, and opcodes don't share this information. 
> It's buried deep down in the opcode table interpreter, which tries all the 
> entries in turn that match the mnemonic to see if the operand pattern 
> matches and gives up once it's run out of the entries and none of the 
> patterns matched the operands.
> 
>  So from the error message above you know it's operands that are wrong and 
> you need to figure out on your own what is actually wrong with them.
> 
>  I guess opcodes could pass some details up, which could then be processed
> by GAS, but given what I have written above it's not clear to me that it's 
> worth the effort.

Well, I can certainly see that there's a question of balancing effort to
provide better diagnostics against how often people would actually care.

>>>  NAK for this part then.
>>
>> I'm sorry, but no, I'm not going to drop all this work just because of you
>> not liking a tiny aspect. Best I can do then is to leave out this patch,
>> and leave fixing the MIPS testsuite to you. I'm not sure you're going to
>> be happy about this. Please let me know.
> 
>  I'm happy to make such an update myself, that's not a problem, but let me 
> chew it over yet.  If anything I suppose using `*' rather than `?' would 
> be more future-proof, but then again, maybe it doesn't matter?

If * was wanted (needed), that would be an indication of the scrubber doing
almost nothing at all. That's certainly a theoretical possibility, but I
don't think it's one in practice. Unless we were to effectively drop all
scrubbing.

Jan