Re: [PATCH] MdePkg/BaseSynchronizationLib: fix XADD operands in GCC IA32/X64 assembly

["Shao, Ming" <ming.shao@intel.com>]

Hi Laszlo,



I build Ruiyu's code with gcc 4.8.5 as X64. And got below disassembled code:



[cid:image001.jpg@01D45689.F8A3F530]



So I didn't see register used as both destination and source of xadd instruction.



Then I build your patch, and got exactly the same disassembled code.



Both Ruiyu's patch and yours can pass my test.



Could you provide more details about your environment so I can reproduce this issue? Thanks.



My environment:

l  Ubuntu 18.04.1 LTS

l  gcc (Ubuntu 4.8.5-4ubuntu8) 4.8.5





-Ming


The content of this message is my personal opinion only and although I am an employee of Intel, the statements I make here in no way represent Intel's position on the issue, nor am I authorized to speak on behalf of Intel on this matter.





-----Original Message-----
From: Ni, Ruiyu
Sent: Wednesday, September 26, 2018 5:35 PM
To: Shao, Ming <ming.shao@intel.com>
Subject: Fwd: [edk2] [PATCH] MdePkg/BaseSynchronizationLib: fix XADD operands in GCC IA32/X64 assembly









-------- Forwarded Message --------

Subject: [edk2] [PATCH] MdePkg/BaseSynchronizationLib: fix XADD operands in GCC IA32/X64 assembly

Date: Tue, 25 Sep 2018 21:48:57 +0200

From: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>

To: edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>>

CC: Michael Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>, Ruiyu Ni <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>, Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>, Liming Gao <liming.gao@intel.com<mailto:liming.gao@intel.com>>



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.yao@intel.com<mailto:jiewen.yao@intel.com>>

Cc: Liming Gao <liming.gao@intel.com<mailto:liming.gao@intel.com>>

Cc: Michael Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>

Cc: Ruiyu Ni <ruiyu.ni@intel.com<mailto:ruiyu.ni@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 <lersek@redhat.com<mailto:lersek@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<mailto:edk2-devel@lists.01.org>

https://lists.01.org/mailman/listinfo/edk2-devel