Currently, "gcc-4.8.5-28.el7_5.1.x86_64" generates the following code for me, from the XADD inline assembly added to "X64/GccInline.c" in commit 17634d026f96:
> 0000000000004383 <InternalSyncIncrement>: > UINT32 > EFIAPI > InternalSyncIncrement ( > IN volatile UINT32 *Value > ) > { > 4383: 55 push %rbp > 4384: 48 89 e5 mov %rsp,%rbp > 4387: 48 83 ec 10 sub $0x10,%rsp > 438b: 48 89 4d 10 mov %rcx,0x10(%rbp) > UINT32 Result; > > __asm__ __volatile__ ( > 438f: 48 8b 55 10 mov 0x10(%rbp),%rdx > 4393: 48 8b 45 10 mov 0x10(%rbp),%rax > 4397: b8 01 00 00 00 mov $0x1,%eax > 439c: f0 0f c1 00 lock xadd %eax,(%rax) > 43a0: ff c0 inc %eax > 43a2: 89 45 fc mov %eax,-0x4(%rbp) > : "m" (*Value) // %2 > : "memory", > "cc" > ); > > return Result; > 43a5: 8b 45 fc mov -0x4(%rbp),%eax > } > 43a8: c9 leaveq > 43a9: c3 retq > The MOV $0X1,%EAX instruction corrupts the address of Value in %RAX before we reach the XADD instruction. In fact, it makes no sense for XADD to use %EAX as source operand and (%RAX) as destination operand at the same time. The XADD instruction's destination operand is a read-write operand. The GCC documentation states: > The ordinary output operands must be write-only; GCC will assume that > the values in these operands before the instruction are dead and need > not be generated. Extended asm supports input-output or read-write > operands. Use the constraint character `+' to indicate such an operand > and list it with the output operands. You should only use read-write > operands when the constraints for the operand (or the operand in which > only some of the bits are to be changed) allow a register. (The above is intentionally quoted from the oldest GCC release that edk2 supports, namely gcc-4.4: <https://gcc.gnu.org/onlinedocs/gcc-4.4.7/gcc/Extended-Asm.html>.) Fix the operand list accordingly. With the patch applied, I get: > 0000000000004383 <InternalSyncIncrement>: > UINT32 > EFIAPI > InternalSyncIncrement ( > IN volatile UINT32 *Value > ) > { > 4383: 55 push %rbp > 4384: 48 89 e5 mov %rsp,%rbp > 4387: 48 83 ec 10 sub $0x10,%rsp > 438b: 48 89 4d 10 mov %rcx,0x10(%rbp) > UINT32 Result; > > __asm__ __volatile__ ( > 438f: 48 8b 55 10 mov 0x10(%rbp),%rdx > 4393: 48 8b 45 10 mov 0x10(%rbp),%rax > 4397: b8 01 00 00 00 mov $0x1,%eax > 439c: f0 0f c1 02 lock xadd %eax,(%rdx) > 43a0: ff c0 inc %eax > 43a2: 89 45 fc mov %eax,-0x4(%rbp) > : // no inputs that aren't also outputs > : "memory", > "cc" > ); > > return Result; > 43a5: 8b 45 fc mov -0x4(%rbp),%eax > } > 43a8: c9 leaveq > 43a9: c3 retq Note that some other bugs remain in "BaseSynchronizationLib/*/GccInline.c"; those should be addressed later, under <https://bugzilla.tianocore.org/show_bug.cgi?id=1208>. Cc: Jiewen Yao <jiewen....@intel.com> Cc: Liming Gao <liming....@intel.com> Cc: Michael Kinney <michael.d.kin...@intel.com> Cc: Ruiyu Ni <ruiyu...@intel.com> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1207 Fixes: 17634d026f968c404b039a8d8431b6389dd396ea Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Laszlo Ersek <ler...@redhat.com> --- Notes: Repo: https://github.com/lersek/edk2.git Branch: xadd_rw MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c | 12 ++++++------ MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c | 12 ++++++------ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c b/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c index d82e0205f553..fa2be7f4b35c 100644 --- a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c +++ b/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c @@ -38,11 +38,11 @@ InternalSyncIncrement ( __asm__ __volatile__ ( "movl $1, %%eax \n\t" "lock \n\t" - "xadd %%eax, %2 \n\t" + "xadd %%eax, %1 \n\t" "inc %%eax " : "=a" (Result), // %0 - "=m" (*Value) // %1 - : "m" (*Value) // %2 + "+m" (*Value) // %1 + : // no inputs that aren't also outputs : "memory", "cc" ); @@ -75,11 +75,11 @@ InternalSyncDecrement ( __asm__ __volatile__ ( "movl $-1, %%eax \n\t" "lock \n\t" - "xadd %%eax, %2 \n\t" + "xadd %%eax, %1 \n\t" "dec %%eax " : "=a" (Result), // %0 - "=m" (*Value) // %1 - : "m" (*Value) // %2 + "+m" (*Value) // %1 + : // no inputs that aren't also outputs : "memory", "cc" ); diff --git a/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c b/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c index 4c4d6e3fc712..ab7efe23c4db 100644 --- a/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c +++ b/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c @@ -38,11 +38,11 @@ InternalSyncIncrement ( __asm__ __volatile__ ( "movl $1, %%eax \n\t" "lock \n\t" - "xadd %%eax, %2 \n\t" + "xadd %%eax, %1 \n\t" "inc %%eax " : "=a" (Result), // %0 - "=m" (*Value) // %1 - : "m" (*Value) // %2 + "+m" (*Value) // %1 + : // no inputs that aren't also outputs : "memory", "cc" ); @@ -74,11 +74,11 @@ InternalSyncDecrement ( __asm__ __volatile__ ( "movl $-1, %%eax \n\t" "lock \n\t" - "xadd %%eax, %2 \n\t" + "xadd %%eax, %1 \n\t" "dec %%eax " : "=a" (Result), // %0 - "=m" (*Value) // %1 - : "m" (*Value) // %2 + "+m" (*Value) // %1 + : // no inputs that aren't also outputs : "memory", "cc" ); -- 2.14.1.3.gb7cf6e02401b _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel