* [PATCH v2] MdePkg/BaseSynchronizationLib: Fix InternalSync[De|In]crement
@ 2018-11-07 15:54 Ruiyu Ni
2018-11-07 20:56 ` Laszlo Ersek
0 siblings, 1 reply; 2+ messages in thread
From: Ruiyu Ni @ 2018-11-07 15:54 UTC (permalink / raw)
To: edk2-devel
Cc: Liming Gao, Michael Kinney, Laszlo Ersek,
Philippe Mathieu-Daudé, Ard Biesheuvel
Today's code generates assembly code as below for
InternalSyncIncrement:
__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
Line #3 and Line #6 both use EAX as destination register.
Line #c uses EAX and (EAX).
The output operand "=a" tells GCC that EAX is used for output.
But GCC only assumes that EAX will be used in the very last
instruction.
Per GCC document,
"Use the ‘&’ constraint modifier on all output operands that must
not overlap an input. Otherwise, GCC may allocate the output
operand in the same register as an unrelated input operand, on
the assumption that the assembler code consumes its inputs before
producing outputs. This assumption may be false if the assembler
code actually consists of more than one instruction."
"=&a" should be used to tell GCC not use EAX before the assembly.
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>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c | 4 ++--
MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c b/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
index af39bdeb51..760a020a32 100644
--- a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
+++ b/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
@@ -40,7 +40,7 @@ InternalSyncIncrement (
"lock \n\t"
"xadd %%eax, %1 \n\t"
"inc %%eax \n\t"
- : "=a" (Result), // %0
+ : "=&a" (Result), // %0
"+m" (*Value) // %1
: // no inputs that aren't also outputs
: "memory",
@@ -76,7 +76,7 @@ InternalSyncDecrement (
"lock \n\t"
"xadd %%eax, %1 \n\t"
"dec %%eax \n\t"
- : "=a" (Result), // %0
+ : "=&a" (Result), // %0
"+m" (*Value) // %1
: // no inputs that aren't also outputs
: "memory",
diff --git a/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c b/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
index edb904c007..767d4626b8 100644
--- a/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
+++ b/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
@@ -40,7 +40,7 @@ InternalSyncIncrement (
"lock \n\t"
"xadd %%eax, %1 \n\t"
"inc %%eax \n\t"
- : "=a" (Result), // %0
+ : "=&a" (Result), // %0
"+m" (*Value) // %1
: // no inputs that aren't also outputs
: "memory",
@@ -76,7 +76,7 @@ InternalSyncDecrement (
"lock \n\t"
"xadd %%eax, %1 \n\t"
"dec %%eax \n\t"
- : "=a" (Result), // %0
+ : "=&a" (Result), // %0
"+m" (*Value) // %1
: // no inputs that aren't also outputs
: "memory",
--
2.16.1.windows.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2] MdePkg/BaseSynchronizationLib: Fix InternalSync[De|In]crement
2018-11-07 15:54 [PATCH v2] MdePkg/BaseSynchronizationLib: Fix InternalSync[De|In]crement Ruiyu Ni
@ 2018-11-07 20:56 ` Laszlo Ersek
0 siblings, 0 replies; 2+ messages in thread
From: Laszlo Ersek @ 2018-11-07 20:56 UTC (permalink / raw)
To: Ruiyu Ni, edk2-devel
Cc: Liming Gao, Michael Kinney, Philippe Mathieu-Daudé,
Ard Biesheuvel
Hi Ray,
On 11/07/18 16:54, Ruiyu Ni wrote:
> Today's code generates assembly code as below for
> InternalSyncIncrement:
> __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
>
> Line #3 and Line #6 both use EAX as destination register.
> Line #c uses EAX and (EAX).
>
> The output operand "=a" tells GCC that EAX is used for output.
> But GCC only assumes that EAX will be used in the very last
> instruction.
>
> Per GCC document,
> "Use the ‘&’ constraint modifier on all output operands that must
> not overlap an input. Otherwise, GCC may allocate the output
> operand in the same register as an unrelated input operand, on
> the assumption that the assembler code consumes its inputs before
> producing outputs. This assumption may be false if the assembler
> code actually consists of more than one instruction."
>
> "=&a" should be used to tell GCC not use EAX before the assembly.
>
> 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>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c | 4 ++--
> MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c | 4 ++--
> 2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c b/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
> index af39bdeb51..760a020a32 100644
> --- a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
> +++ b/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
> @@ -40,7 +40,7 @@ InternalSyncIncrement (
> "lock \n\t"
> "xadd %%eax, %1 \n\t"
> "inc %%eax \n\t"
> - : "=a" (Result), // %0
> + : "=&a" (Result), // %0
> "+m" (*Value) // %1
> : // no inputs that aren't also outputs
> : "memory",
> @@ -76,7 +76,7 @@ InternalSyncDecrement (
> "lock \n\t"
> "xadd %%eax, %1 \n\t"
> "dec %%eax \n\t"
> - : "=a" (Result), // %0
> + : "=&a" (Result), // %0
> "+m" (*Value) // %1
> : // no inputs that aren't also outputs
> : "memory",
> diff --git a/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c b/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
> index edb904c007..767d4626b8 100644
> --- a/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
> +++ b/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
> @@ -40,7 +40,7 @@ InternalSyncIncrement (
> "lock \n\t"
> "xadd %%eax, %1 \n\t"
> "inc %%eax \n\t"
> - : "=a" (Result), // %0
> + : "=&a" (Result), // %0
> "+m" (*Value) // %1
> : // no inputs that aren't also outputs
> : "memory",
> @@ -76,7 +76,7 @@ InternalSyncDecrement (
> "lock \n\t"
> "xadd %%eax, %1 \n\t"
> "dec %%eax \n\t"
> - : "=a" (Result), // %0
> + : "=&a" (Result), // %0
> "+m" (*Value) // %1
> : // no inputs that aren't also outputs
> : "memory",
>
(1) Before you push the patch, please reinstate the reference to:
https://bugzilla.tianocore.org/show_bug.cgi?id=1303
in the commit message.
(The BZ reference was part of v1.)
(2) Please also append:
Suggested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Fixes: 8a94eb9283fa09a30f5f06f0c12cf0ee4e14fbcf
Fixes: 17634d026f968c404b039a8d8431b6389dd396ea
(3) Can you please replace the ‘ (U+2018) and ’ (U+2019) characters in
the commit message with simple ASCII quotes, such as '&'?
Unless there's a strong reason, I prefer to limit commit messages to
plain ASCII. (This does not apply to names of people, of course.)
Feel free to ignore my request if you disagree.
(4) The patch looks good to me.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
(5) I've also tested the patch, on top of c60d36b4d1ee, both the IA32
and the X64 changes, by performing my usual Linux guest tests described at
<https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt#tests-to-perform-in-the-installed-guest-fedora-26-guest>
with the IA32, IA32X64, and X64 builds of OVMF.
Tested-by: Laszlo Ersek <lersek@redhat.com>
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-11-07 20:56 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-07 15:54 [PATCH v2] MdePkg/BaseSynchronizationLib: Fix InternalSync[De|In]crement Ruiyu Ni
2018-11-07 20:56 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox