Re: [PATCH] i386: Only check suffix in instruction mnemonic

[Jan Beulich <jbeulich at suse dot com>]

On 08.11.2019 16:53, H.J. Lu wrote:
> On Thu, Nov 7, 2019 at 11:41 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 07.11.2019 22:52,  H.J. Lu  wrote:
>>> Another problem is Intel syntax sets instruction suffix from operand size:
>>
>> An unrelated one, but yes.
>>
>>> [hjl@gnu-cfl-1 gas]$ cat 2.s
>>> .intel_syntax noprefix
>>> call WORD PTR [rax]
>>> [hjl@gnu-cfl-1 gas]$ as -mintel64 -o 2.o 2.s
>>> 2.s: Assembler messages:
>>> 2.s:2: Error: invalid instruction suffix for `call'
>>> [hjl@gnu-cfl-1 gas]$
>>>
>>> It is operand size mismatch, not invalid instruction suffix.  In case
>>> of CMPSD and
>>> and MOVSD,the 'l' suffix is invalid, not DWORD on memory operand.  But suffix
>>> check treats them the same.  This makes Intel syntax more complicated.   Should
>>> Intel syntax disallow suffix in mnemonic?  It will make Intel syntax
>>> closer to SDM.
>>
>> There are some suffixes that need honoring. The original authors
>> of the code apparently decided that it's better to accept some
>> stray/bogus suffixes than to further complicate the code. I guess
>> once we have the No_*Suf attributes put straight (as mentioned by
>> Michael the other day, matching plans I've been having), then we
>> could see about further cleaning up Intel syntax behavior here.
>>
> 
> I am checking in this.
> 
> We should check suffix in instruction mnemonic when matching instruction.
> In Intel syntax, normally we check for memory operand size.  But the same
> mnemonic with 2 different encodings can have the same memory operand
> size and i.suffix is set to LONG_DOUBLE_MNEM_SUFFIX from memory operand
> size in Intel syntax to distinguish them.  When there is no suffix in
> mnemonic, we check LONG_DOUBLE_MNEM_SUFFIX in i.suffix for mnemonic
> suffix.

So even after quite a bit of checking and thinking my gut feeling
still is that this may have broken some corner cases. But I can't
come up with a specific example, so maybe all is well.

On the positive side this fixes MOVDIRI handling: Previously only
the operand-size less cases below would have been accepted, whereas
now all 6 valid ones remain without diagnostic.

I suspect though that your change should have been accompanied
with further IgnoreSize dropping, as I think quite a few that we
still have in there are no longer needed now.

Btw, would you mind me putting in the testsuite parts of the
alternative patches I had sent for this PR?

Jan

	movdiri [rcx], eax
	movdiri dword ptr [rcx], eax
	movdiri qword ptr [rcx], eax

	movdiri [rcx], rax
	movdiri dword ptr [rcx], rax
	movdiri qword ptr [rcx], rax

	.code32
	movdiri [ecx], eax
	movdiri dword ptr [ecx], eax
	movdiri qword ptr [ecx], eax