From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.115; helo=mga14.intel.com; envelope-from=ming.shao@intel.com; receiver=edk2-devel@lists.01.org Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C60CE21148DDE for ; Thu, 27 Sep 2018 02:46:46 -0700 (PDT) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 27 Sep 2018 02:46:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,310,1534834800"; d="jpg'145?scan'145,208,217,145";a="74115948" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga008.fm.intel.com with ESMTP; 27 Sep 2018 02:46:22 -0700 Received: from FMSMSX109.amr.corp.intel.com (10.18.116.9) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 27 Sep 2018 02:46:16 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx109.amr.corp.intel.com (10.18.116.9) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 27 Sep 2018 02:46:13 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.140]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.220]) with mapi id 14.03.0319.002; Thu, 27 Sep 2018 17:46:11 +0800 From: "Shao, Ming" To: "lersek@redhat.com" CC: "edk2-devel@lists.01.org" , "Ni, Ruiyu" , "Shao, Ming" Thread-Topic: [edk2] [PATCH] MdePkg/BaseSynchronizationLib: fix XADD operands in GCC IA32/X64 assembly Thread-Index: AQHUVXw6t0Upgpr8XEm4SK+Va24mXqUD4fMw Date: Thu, 27 Sep 2018 09:46:10 +0000 Message-ID: <0D32B2537B667F42AD320D616D521AF738B921BD@shsmsx102.ccr.corp.intel.com> References: <20180925194857.10514-1-lersek@redhat.com> <8ecbcc60-8e0f-e418-614e-666aa7fb007b@Intel.com> In-Reply-To: <8ecbcc60-8e0f-e418-614e-666aa7fb007b@Intel.com> Accept-Language: en-US X-MS-Has-Attach: yes X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNjRkNGM4MDctZDcwYy00Y2ZiLWE1NWUtYzRhMDllOWU5NzYxIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiTFwvZFUzR2tBTURMdnB0dGdYN2krRVZKUjU1eE9tVG1IOFloWXZ4K3o1ZkZzelE0SFN1d1pCZk1WRGNjQlwvUm1jIn0= x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 X-Content-Filtered-By: Mailman/MimeDel 2.1.29 Subject: Re: [PATCH] MdePkg/BaseSynchronizationLib: fix XADD operands in GCC IA32/X64 assembly X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 27 Sep 2018 09:46:47 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 instru= ction. 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 th= is 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 a= n 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 Subject: Fwd: [edk2] [PATCH] MdePkg/BaseSynchronizationLib: fix XADD operan= ds 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 > To: edk2-devel-01 > CC: Michael Kinney >, Ruiyu Ni >, Jiewen Y= ao >, Liming Gao > Currently, "gcc-4.8.5-28.el7_5.1.x86_64" generates the following code for m= e, from the XADD inline assembly added to "X64/GccInline.c" in commit 17634d026f96: > 0000000000004383 : > 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 su= pports, namely gcc-4.4: .) Fix the operand list accordingly. With the patch applied, I get: > 0000000000004383 : > 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, un= der . Cc: Jiewen Yao > Cc: Liming Gao > Cc: Michael Kinney > Cc: Ruiyu Ni > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1207 Fixes: 17634d026f968c404b039a8d8431b6389dd396ea Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Laszlo Ersek > --- 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 " : "=3Da" (Result), // %0 - "=3Dm" (*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 " : "=3Da" (Result), // %0 - "=3Dm" (*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 " : "=3Da" (Result), // %0 - "=3Dm" (*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 " : "=3Da" (Result), // %0 - "=3Dm" (*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