> -----Original Message----- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo > Ersek > Sent: Wednesday, November 7, 2018 10:42 PM > To: Ni, Ruiyu <ruiyu...@intel.com>; edk2-devel@lists.01.org > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Gao, Liming > <liming....@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...@intel.com> > >> Cc: Liming Gao <liming....@intel.com> > >> Cc: Michael Kinney <michael.d.kin...@intel.com> > >> Cc: Laszlo Ersek <ler...@redhat.com> > >> Cc: Philippe Mathieu-Daudé <phi...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
