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

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

On Fri, Aug 3, 2018 at 9:17 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Aug 3, 2018 at 9:01 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 03.08.18 at 17:33, <hjl.tools@gmail.com> wrote:
>>> 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:
>>>>>>> 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.
>>
>> And that's my plan going forward, as said.
>>
>>>> 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.
>>
>> I'm sorry, but I'm not going to cripple the change just so that your
>> unreasonable requirement can be met. Let's take a different example:
>>
>>         add     (%cr1), (%cr0)
>>         add     (,%xmm1), (,%xmm0)
>>
>> Are you telling me it matters whether the assembler complains about
>> the first or second operand being wrong, or there being two memory
>> operands when just one is allowed? Once again - when there are
>> multiple things wrong for a single insn, there should be no expectation
>> whatsoever as to which of the possible error messages actually gets
>> emitted, as that's an implementation detail of the assembler. If you
>> disagree here, then I'm looking forward for a very good explanation
>> of your standpoint.
>
> The 0x67 prefix in MPX instructions is ignored. That is why there is
>
> 32-bit address isn't allowed in 64-bit MPX instructions
>
> We should keep this message for "%eip".

I checked:

[hjl@gnu-tools-1 build-x86_64-linux]$ cat /tmp/y.s
bndmk (%rip), %bnd2
bndmk (%eip), %bnd2
[hjl@gnu-tools-1 build-x86_64-linux]$ gcc -c /tmp/y.s
/tmp/y.s: Assembler messages:
/tmp/y.s:1: Error: `(%rip)' cannot be used here
/tmp/y.s:2: Error: 32-bit address isn't allowed in 64-bit MPX instructions.
[hjl@gnu-tools-1 build-x86_64-linux]$

Your change generates the same error as %rip.   Your change is OK.
Sorry for the confusion.

Thanks.

-- 
H.J.