Re: [PATCH] MdePkg/BaseSynchronizationLib XCODE: fix InternalSync[De|In]crement

["Ni, Ruiyu" <ruiyu.ni@intel.com>]

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: Wednesday, November 7, 2018 10:42 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: Re: [edk2] [PATCH] MdePkg/BaseSynchronizationLib XCODE: fix
> InternalSync[De|In]crement
> 
> On 11/07/18 15:31, Laszlo Ersek wrote:
> > (+Andrew)
> >
> > Hi Ray,
> >
> > On 11/07/18 05:03, Ruiyu Ni wrote:
> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1303
> >>
> >> XCODE disassembly code of InternalSyncDecrement with today's code is:
> >>
> >>   __asm__ __volatile__ (
> >>     "movl    $1, %%eax  \n\t"
> >>     "lock               \n\t"
> >>     "xadd    %%eax, %1  \n\t"
> >>     "inc     %%eax      \n\t"
> >>     : "=a" (Result),          // %0
> >>       "+m" (*Value)           // %1
> >>     :                         // no inputs that aren't also outputs
> >>     : "memory",
> >>       "cc"
> >>     );
> >>
> >>  0:       55      pushl   %ebp
> >>  1:       89 e5   movl    %esp, %ebp
> >>  3:       8b 45 08        movl    8(%ebp), %eax
> >>  6:       b8 01 00 00 00  movl    $1, %eax
> >>  b:       f0      lock
> >>  c:       0f c1 00        xaddl   %eax, _InternalSyncIncrement(%eax)
> >>  f:       40      incl    %eax
> >> 10:       5d      popl    %ebp
> >> 11:       c3      retl
> >>
> >> %EAX value retrieved in line #3 is overwritten in line #6.
> >
> > (a) This looks like an XCODE bug to me. The "=a" constraint on operand
> > %0 means that Result should be set from eax/rax, and that this operand
> > is "write only". Here's the gcc documentation:
> >
> > "The ordinary output operands must be write-only; GCC assumes that the
> > values in these operands before the instruction are dead and need not
> > be generated."
> >
> > Furthermore, if a register is named in any operand constraint (such as
> > input, output, input-output), then it cannot, by definition, be listed
> > in the "clobber list" -- it is *obvious* that the inline assembly will
> > work with that register. In case of an output-only operand, it's
> > obvious that the inline assembly will *overwrite* that register, and
> > the compiler cannot assume *when* that overwrite will happen. Here's
> > the gcc
> > documentation:
> >
> > "You may not write a clobber description in a way that overlaps with
> > an input or output operand.  For example, you may not have an operand
> > describing a register class with one member if you mention that
> > register in the clobber list.  Variables declared to live in specific
> > registers [...] and used as asm input or output operands must have no
> > part mentioned in the clobber description." [1]
> >
> > However, XCODE generates such code that depends on EAX as an *input*
> > operand. That's clearly wrong and violates the constraints in the
> > operand list. This is an XCODE bug.
> >
> > In fact: the situation is worse than that. In the commit message you
> > spell out that the MOV instruction at offset 6 overwrites the value in
> > EAX that was just loaded at offset 3. But this is just the small
> > problem; the *large* problem is the generated XADD instruction itself,
> > at offset 0xb and 0xc. The binary encoding is, from your commit message:
> >
> >   f0 0f c1 00
> >
> > and this is *exactly* the problem that my commit 8a94eb9283fa fixed
> > for gcc! From my commit message:
> >
> >     >     439c:       f0 0f c1 00             lock xadd %eax,(%rax)
> >
> > Because, it makes *no sense* for XADD to use the AX register for
> > *both* pointer-to-memory (i.e. address of the destination location
> > that receives the sum) *and* as the other addend!
> >
> > In other words, regardless of how we fill the AX register up-front, an
> > XADD instruction generated like this *cannot* be right.
> >
> >> The patch uses the clobber list to tell GCC that EAX is used in ASM.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> >> Cc: Liming Gao <liming.gao@intel.com>
> >> Cc: Michael Kinney <michael.d.kinney@intel.com>
> >> Cc: Laszlo Ersek <lersek@redhat.com>
> >> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >>  MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c | 12
> >> ++++++++----
> >>  1 file changed, 8 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
> >> b/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
> >> index af39bdeb51..0a985529fd 100644
> >> --- a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
> >> +++ b/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
> >> @@ -40,11 +40,13 @@ InternalSyncIncrement (
> >>      "lock               \n\t"
> >>      "xadd    %%eax, %1  \n\t"
> >>      "inc     %%eax      \n\t"
> >> -    : "=a" (Result),          // %0
> >> +    "mov     %%eax, %0  \n\t"
> >> +    : "=r" (Result),          // %0
> >>        "+m" (*Value)           // %1
> >>      :                         // no inputs that aren't also outputs
> >>      : "memory",
> >> -      "cc"
> >> +      "cc",
> >> +      "eax"
> >>      );
> >>
> >>    return Result;
> >
> > (b) This change is invalid, for two separate reasons.
> >
> > Reason #1: on the clobber list, the AX register should be listed as
> > "a", not as "eax".
> >
> > Reason #2:
> >
> > - For operand %0, we say "use any register in the 'r' class as
> > write-only". (The class "r" means "general register".) And, at the end
> > of the inline assembly, we store EAX manually to that 'r' class
> > register. And we rely on the compiler to store that other general
> > register into Result.
> >
> > - We append the AX register (which should be spelled as "a") to the
> > clobber list. However, the "a" register is itself in the "general
> > register" class, and therefore this clobber list breaks the passage
> > that I quoted above, marked as [1]!
> >
> >
> >> @@ -76,11 +78,13 @@ InternalSyncDecrement (
> >>      "lock                \n\t"
> >>      "xadd    %%eax, %1   \n\t"
> >>      "dec     %%eax       \n\t"
> >> -    : "=a" (Result),           // %0
> >> +    "mov     %%eax, %0  \n\t"
> >> +    : "=r" (Result),           // %0
> >>        "+m" (*Value)            // %1
> >>      :                          // no inputs that aren't also outputs
> >>      : "memory",
> >> -      "cc"
> >> +      "cc",
> >> +      "eax"
> >>      );
> >>
> >>    return Result;
> >>
> >
> > (c) The patch doesn't update the X64 variant.
> >
> > I think we should be clear here that we are working around an XCODE bug.
> > Commit 8a94eb9283fa was different, because the code before that
> > violated the gcc documentation, and so the issue was in the code.
> >
> >
> > I'll comment more on your second email.
> 
> Actually I'd like to continue here.
> 
> In my view:
> 
> - Commit 17634d026f96 ("MdePkg/SynchronizationLib: fix
> Interlocked[De|In]crement return value", 2018-09-25) was the right idea,
> because it improved internal edk2 API conformance (regarding the return values).
> However, the assembly template was not correct, according to the GCC
> documentation.
> 
> - Commit 8a94eb9283fa ("MdePkg/BaseSynchronizationLib: fix XADD operands
> in GCC IA32/X64 assembly", 2018-09-26) fixed the assembly templates, so that
> they would conform to the GCC documentation.
> 
> - Now the current issue is that XCODE does not implement the GCC
> documentation correctly, from the compiler / assembler side. So the bug is not
> in the edk2 code, but the toolchain.
> 
> 
> My first preference would be if we could split the code between XCODE and
> genuine GCC. This is now the n'th time when XCODE does not live up to its
> promised GCC compatibility. I think it's time to address that with actual code
> separation in edk2, like we do for MSVC and ICC already. Whatever we do for
> XCODE is basically "shotgun debugging" at this point, and I wouldn't like that to
> affect source code that genuine GCC compiles.
> 
> My second preference would be to roll back both commits 8a94eb9283fa and
> 17634d026f96.

I agree. But need Jiewen's comments on rolling back the 2 commits.
Because he raised the InterlockedIncrement() issue internally to me.

> 
> Disclaimer: obviously, if someone can refute my statements about the
> correctness of the current code, that's 100% welcome and appreciated.
> The above is how I understand the status, but if I'm wrong, I *should* be
> corrected.
> 
> Thanks,
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel