Re: [PATCH 6/6] x86: fold RegEip/RegRip and RegEiz/RegRiz

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Thu, Aug 2, 2018 at 11:55 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 02.08.18 at 18:33, <hjl.tools@gmail.com> wrote:
>> On Thu, Aug 2, 2018 at 9:06 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 02.08.18 at 14:49, <hjl.tools@gmail.com> wrote:
>>>> On Thu, Aug 2, 2018 at 5:43 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 02.08.18 at 14:28, <hjl.tools@gmail.com> wrote:
>>>>>> On Wed, Aug 1, 2018 at 11:52 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> --- a/gas/testsuite/gas/i386/x86-64-mpx-inval-2.l
>>>>>>> +++ b/gas/testsuite/gas/i386/x86-64-mpx-inval-2.l
>>>>>>> @@ -2,7 +2,7 @@
>>>>>>>  .*:6: Error: 32-bit address isn't allowed in 64-bit MPX instructions.
>>>>>>>  .*:7: Error: 32-bit address isn't allowed in 64-bit MPX instructions.
>>>>>>>  .*:8: Error: `\(%rip\)' cannot be used here
>>>>>>> -.*:9: Error: 32-bit address isn't allowed in 64-bit MPX instructions.
>>>>>>> +.*:9: Error: .*
>>>>>>>
>>>>>>
>>>>>> We should keep "32-bit address isn't allowed in 64-bit MPX instructions"
>>>>>> error.
>>>>>
>>>>> Are you suggesting to special case the place the error message now
>>>>> gets raised, just to get the wording the same as before? What's wrong
>>>>> with it being either the previous one or the analogue to "`(%rip)'
>>>>> cannot be used here", i.e. "`(%eip)' cannot be used here". When a
>>>>> single statement is wrong in multiple possible ways, I don't think there
>>>>> should be a requirement which of the issues the assembler reports, as
>>>>> long as it reports exactly one (I find it odd enough that there are
>>>>> cases where two errors get reported for a single statement, but I'll
>>>>> get to that eventually as well).
>>>>>
>>>>
>>>> We get an error today:
>>>>
>>>> [hjl@gnu-tools-1 tmp]$ cat x.s
>>>> bndmk (%eip), %bnd2
>>>> [hjl@gnu-tools-1 tmp]$ gcc -c x.s
>>>> x.s: Assembler messages:
>>>> x.s:1: Error: 32-bit address isn't allowed in 64-bit MPX instructions.
>>>> [hjl@gnu-tools-1 tmp]$
>>>>
>>>> With your change, what do we get?
>>>
>>> `(%eip)' cannot be used here
>>
>>  .*  Error: 32-bit address isn't allowed in 64-bit MPX instructions\.
>>  [      ]*[1-9][0-9]*[  ]+4C1903
>>  [      ]*[1-9][0-9]*[  ]+bndmk \(%rip\), %bnd3
>> -[      ]*[1-9][0-9]*[  ]+\?\?\?\? 67F30F1B             bndmk \(%eip\), %bnd2
>> -.*  Error: 32-bit address isn't allowed in 64-bit MPX instructions\.
>> -[      ]*[1-9][0-9]*[  ]+15000000
>> -[      ]*[1-9][0-9]*[  ]+00
>> +[      ]*[1-9][0-9]*[  ]+bndmk \(%eip\), %bnd2
>>  [      ]*[1-9][0-9]*[  ]+
>>  [      ]*[1-9][0-9]*[  ]+\#\#\# bndmov
>>  [      ]*[1-9][0-9]*[  ]+\?\?\?\? 6766410F             bndmov \(%r8d\), %bnd1
>>
>> Why is there no error message for bndmk (%eip), %bnd2"?  Also
>
> Well, I've explained this already, but maybe the connection isn't
> as obvious to you as I thought it would be: _Any_ errors appearing
> in the listing are an indication of a problem, because it means the
> assembler didn't simply bail after finding an error. In the case here
> it is
>
>   if (i.tm.cpu_flags.bitfield.cpumpx)
>     {
>       if (flag_code == CODE_64BIT && i.prefix[ADDR_PREFIX])
>         as_bad (_("32-bit address isn't allowed in 64-bit MPX instructions."));
>       else if (flag_code != CODE_16BIT
>                ? i.prefix[ADDR_PREFIX]
>                : i.mem_operands && !i.prefix[ADDR_PREFIX])
>         as_bad (_("16-bit address isn't allowed in MPX instructions"));
>     }
>
> Note how there no "return" here. That's the very inconsistency I

We can add a return here.

> did explain before (see the parenthesized statements in my second
> most recent response, still visible in quoted context above), and
> that I mean to get to fixing eventually.
>
> Bottom line - an error disappearing from the listing should not be
> considered a problem, but an improvement (the only alternative
> view I could accept would be if _all_ errors appeared in the listing).
>
>> `(%eip)' cannot be used here
>>
>> contains less info than
>>
>> 32-bit address isn't allowed in 64-bit MPX instructions
>
> Why that? Everyone ought to know that %eip is 32 bits. And once
> again - it being wrong to use 32-bit _or_ IP-relative addressing,
> there shouldn't be a requirement which of the two gets reported.
> So in the end this change of error message is because the error
> now comes from parse_operands(), in which case md_assemble()
> bails rather than reaching the piece of quoted code above. I view
> it as entirely unreasonable to complicate the code in
> i386_index_check() just to be able to emit that other diagnostic.
>

We should have the same error for both %eip and %eax for MPX
in 64-bit mode.

-- 
H.J.