Re: [PATCH v3] MdePkg/SynchronizationLib: fix Interlocked[De|In]crement return value

[Laszlo Ersek <lersek@redhat.com>]

On 09/25/18 18:18, Laszlo Ersek wrote:
> On 09/25/18 16:22, Laszlo Ersek wrote:
>> On 09/13/18 11:50, Ruiyu Ni wrote:
>>
>>> diff --git a/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c b/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
>>> index 5224dd063f..4c4d6e3fc7 100644
>>> --- a/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
>>> +++ b/MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
>>> @@ -15,14 +15,12 @@
>>>
>>>
>>>
>>> -
>>>  /**
>>>    Performs an atomic increment of an 32-bit unsigned integer.
>>>
>>>    Performs an atomic increment of the 32-bit unsigned integer specified by
>>>    Value and returns the incremented value. The increment operation must be
>>> -  performed using MP safe mechanisms. The state of the return value is not
>>> -  guaranteed to be MP safe.
>>> +  performed using MP safe mechanisms.
>>>
>>>    @param  Value A pointer to the 32-bit value to increment.
>>>
>>> @@ -38,9 +36,10 @@ InternalSyncIncrement (
>>>    UINT32  Result;
>>>
>>>    __asm__ __volatile__ (
>>> +    "movl    $1, %%eax  \n\t"
>>>      "lock               \n\t"
>>> -    "incl    %2         \n\t"
>>> -    "mov     %2, %%eax      "
>>> +    "xadd    %%eax, %2  \n\t"
>>> +    "inc     %%eax          "
>>>      : "=a" (Result),          // %0
>>>        "=m" (*Value)           // %1
>>>      : "m"  (*Value)           // %2
>>
>> The operand list for the inline assembly is incorrect, I suspect. When I
>> build this code with GCC48 as part of OVMF, for the NOOPT target, then
>> disassemble "CpuMpPei.debug", 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 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 generated code is trying to use EAX for both (a) the
>> exchange-and-add, i.e.  as the source operand of the XADD instruction,
>> and (b) as (part of) the address of the (*Value) argument.
>>
>> In effect, before we reach the XADD instruction, the least significant
>> dword of Value's address is overwritten with 0x0000_0001. The exchange
>> then happens against that memory location.
>>
>> It is interesting that GCC first spills the parameter from RCX to the
>> stack, and then it loads the parameter from the stack to both RDX and
>> RAX.
>>
>> I think the operand list used with the inline assembly should be updated
>> to prevent GCC from associating %2 with EAX.
>>
>> I'll try to experiment some more.
> 
> The operand lists for almost all of the functions in
> 
> - MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
> - MdePkg/Library/BaseSynchronizationLib/X64/GccInline.c
> 
> are wrong. The InternalSyncIncrement() and InternalSyncDecrement() issue
> has now bitten us in practice. The InternalSyncCompareExchangeXx()
> functions also have incorrect operand lists, they just haven't blown up yet.
> 
> The core problem is that input-output operands for the inline assembly
> templates must be marked as such. The GCC documentation describes two
> ways for that:
> 
> - List the r/w operand only in the output operand list, and use the "+"
> (not "=") constraint.
> 
> - Alternatively, list the operand in both input and output lists, and
> use the "=" constraint in the output list. However, in the input list,
> the constraing must be "N" (a decimal number), where N stands for the
> operand number in the output list. This is how GCC knows those operands
> are actually the same. Using just the same *expression* to fill in the
> two instances (= input and output) of the operand is not guaranteed to
> work; the gcc documentation explicitly highlights this fact.
> 
> In addition, in the ASM template, operands can be referenced by "[name]"
> instead of %N. This has been supported since at least gcc-3.1. This
> makes the template a lot more readable.
> 
> I'm working on a patch set.

Apologies for conversing myself, but now I've managed to review more
background discussion from the mailing list.

In particular, in the following messages:

http://mid.mail-archive.com/4A89E2EF3DFEDB4C8BFDE51014F606A14E2F666F@SHSMSX104.ccr.corp.intel.com

http://mid.mail-archive.com/E92EE9817A31E24EB0585FDF735412F5B8AD687E@ORSMSX113.amr.corp.intel.com

an agreement was reached that ultimately *two* implementations would
remain: compiler intrinsics for the MSFT toolchain family, and the NASM
implementation for *everything else*, including *both* GCC and INTEL
toolchain families.

So, why was version 3 of the patch then accepted and pushed? Because now
we still have *three* implementations (excerpt):

[Sources.X64]
  InterlockedIncrementMsc.c | MSFT
  InterlockedDecrementMsc.c | MSFT

  X64/InterlockedDecrement.nasm | INTEL
  X64/InterlockedIncrement.nasm | INTEL

  X64/GccInline.c | GCC

This is not in the spirit of the agreement on the list.

Why this is relevant to me: because now I realize I shouldn't *fix* the
GCC inline assembly. Instead, we should *remove* it, and consume the
NASM implementation.

So here's what I'm going to do. I will submit a standalone, "surgical"
patch, for fixing the regression introduced in 17634d026f96.
Additionally, I will file a TianoCore BZ about the *other* bugs in
GccInline.c, and suggest that we fix those issues by eliminating the
GCC-specific variant altogether. This way the regression is going to be
averted while we discuss that BZ.

Thanks
Laszlo