[PATCH] x86: Update register operand check for AddrPrefixOpReg

[H.J. Lu]

On Thu, Oct 1, 2020 at 11:58 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 01.10.2020 17:08, H.J. Lu wrote:
> > On Thu, Oct 1, 2020 at 7:57 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 01.10.2020 15:44, H.J. Lu wrote:
> >>> On Thu, Oct 1, 2020 at 6:23 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 01.10.2020 15:07, H.J. Lu wrote:
> >>>>> On Thu, Oct 1, 2020 at 4:35 AM H.J. Lu <hjl.tools@gmail.com> wrote:
> >>>>>>
> >>>>>> On Thu, Oct 1, 2020 at 1:22 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>>>>
> >>>>>>> On 01.10.2020 01:30, H.J. Lu via Binutils wrote:
> >>>>>>>> I am checking in this patch and backporting it to 2.35 branch.
> >>>>>>>
> >>>>>>> But this is wrong, as can be seen from e.g. ...
> >>>>>>>> + +[a-f0-9]+: 66 0f 38 f8 0d 00 00 00 00      movdir64b 0x0\(%rip\),%rcx        #.*
> >>>>>>>> + +[a-f0-9]+: 67 66 0f 38 f8 0d 00 00 00 00   movdir64b 0x0\(%eip\),%ecx        #.*
> >>>>>>>> + +[a-f0-9]+: 67 66 0f 38 f8 0d 00 00 00 00   movdir64b 0x0\(%eip\),%ecx        #.*
> >>>>>>>
> >>>>>>> ... the middle line here not matching up with
> >>>>>>>
> >>>>>>>> +     movdir64b foo(%rip),%rcx
> >>>>>>>> +     movdir64b foo(%rip),%ecx
> >>>>>>>> +     movdir64b foo(%eip),%ecx
> >>>>>>>
> >>>>>>> ... what was written here. Without your change this
> >>>>>>>
> >>>>>>>         movdir64b (%rbp), %rax
> >>>>>>>         movdir64b (%rbp), %eax
> >>>>>>>         movdir64b (%ebp), %rax
> >>>>>>>         movdir64b (%ebp), %eax
> >>>>>>>
> >>>>>>>         movdir64b (%rip), %rax
> >>>>>>>         movdir64b (%rip), %eax
> >>>>>>>         movdir64b (%eip), %rax
> >>>>>>>         movdir64b (%eip), %eax
> >>>>>>>
> >>>>>>> yields consistent results for both blocks - the middle two entries
> >>>>>>> get an error issued.
> >>>>>>>
> >>>>>>> Please revert, and once again please don't commit (let alone
> >>>>>>> backport) patches without giving people at least _a little bit_ of
> >>>>>>> time to look at them.
> >>>>>>>
> >>>>>>
> >>>>>> The fix is correct.   This specific case came from
> >>>>>>
> >>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97257
> >>>>>>
> >>>>>> The address prefix changes the register operand in these instructions.
> >>>>>> (%rip) is a special case.
> >>>>>>
> >>>>>
> >>>>> Update the fix for DISP and SYMBOL.
> >>>>
> >>>> The description wrongly says:
> >>>>
> >>>> "When the address size prefix applies to both the memory and the register
> >>>>  operand, we need to extract the address size prefix from the register
> >>>>  operand if the memory operand has no real registers, like symbol, DISP
> >>>>  or symbol(%rip)."
> >>>>
> >>>> And then similarly a code comment. To the assembler, %rip _is_ a real
> >>>> register (and %eip could be used in its stead, when 32-bit addressing is
> >>>> desired).
> >>>
> >>> GNU assembler serves GCC.  GCC doesn't use (%eip) and GCC always
> >>> uses (%rip) for RIP-relative addressing.
> >>
> >> This may be its main purpose, but it's by far not its only one.
> >>
> >
> > GNU assembler accommodates other usages.  But it must work with GCC.
>
> We agree on this. What you don't want to accept is that gcc also
> needs to emit correct code for gas to consume.
>
> > In this case, it needs to check the register operand for the address size prefix
> > when the memory operand is (%rip).
>
> And we again agree on the checking aspect. What we disagree on is
> what to do if the check finds a discrepancy. Without your recent
> change, the checking you talk about did already happen. And the
> assembler correctly diagnosed it. The assembler may not _ever_
> guess upon what the programmer meant, when finding an ambiguity
> (exceptions to this rule may be warranted for backwards
> compatibility in very rare cases, but this clearly doesn't apply
> here). We've had this discussion before in different contexts,
> and I have to admit I'm struggling with how you drive things in
> this regard. If there was a way for gcc to indicate "I've done my
> best, but please fix it up for me", I could accept there being
> such a (non-default) mode. I wouldn't like it very much, but I
> could live with it. But for hand-written code (be it entire .s
> files or inline assembly) inconsistencies and ambiguities need to
> be diagnosed by default.
>
> Note in particular that your change isn't even limited to x32
> mode, where one _might_ view the adjustment as benign.

The address size prefix isn't limited to x32.   For RIP-relative
addressing, GCC always uses (%rip).  The assembler will never
guess correctly what the assembly code is trying to do in all
cases.  In this particular case, the assembler needs to look
at the register operand to extract the address size prefix.
I am going to check in the rest of my fix.

> If this change of yours isn't going to get reverted, I will have
> to enter a bug requesting its revert. Given there is already the
> (false) bug report that triggered this fix, having to do so would
> feel pretty odd, though.
>
> Jan



-- 
H.J.