Re: [PATCH] MdePkg/BaseSynchronizationLib XCODE: fix InternalSync[De|In]crement

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 7 November 2018 at 15:31, Laszlo Ersek <lersek@redhat.com> wrote:
> (+Andrew)
>
> Hi Ray,
>
> On 11/07/18 05:03, Ruiyu Ni wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1303
>>
>> XCODE disassembly code of InternalSyncDecrement with today's code is:
>>
>>   __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
>>
>> %EAX value retrieved in line #3 is overwritten in line #6.
>
> (a) This looks like an XCODE bug to me. The "=a" constraint on operand
> %0 means that Result should be set from eax/rax, and that this operand
> is "write only". Here's the gcc documentation:
>
> "The ordinary output operands must be write-only; GCC assumes that the
> values in these operands before the instruction are dead and need not be
> generated."
>

Do we need to use early clobber here? "=&a"?

It seems to me that without that, the compiler is free to use eax as
input, since it only assumes that eax will be set by the last
instruction.


> Furthermore, if a register is named in any operand constraint (such as
> input, output, input-output), then it cannot, by definition, be listed
> in the "clobber list" -- it is *obvious* that the inline assembly will
> work with that register. In case of an output-only operand, it's obvious
> that the inline assembly will *overwrite* that register, and the
> compiler cannot assume *when* that overwrite will happen. Here's the gcc
> documentation:
>
> "You may not write a clobber description in a way that overlaps with an
> input or output operand.  For example, you may not have an operand
> describing a register class with one member if you mention that register
> in the clobber list.  Variables declared to live in specific registers
> [...] and used as asm input or output operands must have no part
> mentioned in the clobber description." [1]
>
> However, XCODE generates such code that depends on EAX as an *input*
> operand. That's clearly wrong and violates the constraints in the
> operand list. This is an XCODE bug.
>
> In fact: the situation is worse than that. In the commit message you
> spell out that the MOV instruction at offset 6 overwrites the value in
> EAX that was just loaded at offset 3. But this is just the small
> problem; the *large* problem is the generated XADD instruction itself,
> at offset 0xb and 0xc. The binary encoding is, from your commit message:
>
>   f0 0f c1 00
>
> and this is *exactly* the problem that my commit 8a94eb9283fa fixed for
> gcc! From my commit message:
>
>     >     439c:       f0 0f c1 00             lock xadd %eax,(%rax)
>
> Because, it makes *no sense* for XADD to use the AX register for *both*
> pointer-to-memory (i.e. address of the destination location that
> receives the sum) *and* as the other addend!
>
> In other words, regardless of how we fill the AX register up-front, an
> XADD instruction generated like this *cannot* be right.
>
>> The patch uses the clobber list to tell GCC that EAX is used in ASM.
>>
>> 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>
>> ---
>>  MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c b/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
>> index af39bdeb51..0a985529fd 100644
>> --- a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
>> +++ b/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c
>> @@ -40,11 +40,13 @@ InternalSyncIncrement (
>>      "lock               \n\t"
>>      "xadd    %%eax, %1  \n\t"
>>      "inc     %%eax      \n\t"
>> -    : "=a" (Result),          // %0
>> +    "mov     %%eax, %0  \n\t"
>> +    : "=r" (Result),          // %0
>>        "+m" (*Value)           // %1
>>      :                         // no inputs that aren't also outputs
>>      : "memory",
>> -      "cc"
>> +      "cc",
>> +      "eax"
>>      );
>>
>>    return Result;
>
> (b) This change is invalid, for two separate reasons.
>
> Reason #1: on the clobber list, the AX register should be listed as "a",
> not as "eax".
>
> Reason #2:
>
> - For operand %0, we say "use any register in the 'r' class as
> write-only". (The class "r" means "general register".) And, at the end
> of the inline assembly, we store EAX manually to that 'r' class
> register. And we rely on the compiler to store that other general
> register into Result.
>
> - We append the AX register (which should be spelled as "a") to the
> clobber list. However, the "a" register is itself in the "general
> register" class, and therefore this clobber list breaks the passage that
> I quoted above, marked as [1]!
>
>
>> @@ -76,11 +78,13 @@ InternalSyncDecrement (
>>      "lock                \n\t"
>>      "xadd    %%eax, %1   \n\t"
>>      "dec     %%eax       \n\t"
>> -    : "=a" (Result),           // %0
>> +    "mov     %%eax, %0  \n\t"
>> +    : "=r" (Result),           // %0
>>        "+m" (*Value)            // %1
>>      :                          // no inputs that aren't also outputs
>>      : "memory",
>> -      "cc"
>> +      "cc",
>> +      "eax"
>>      );
>>
>>    return Result;
>>
>
> (c) The patch doesn't update the X64 variant.
>
> I think we should be clear here that we are working around an XCODE bug.
> Commit 8a94eb9283fa was different, because the code before that violated
> the gcc documentation, and so the issue was in the code.
>
>
> I'll comment more on your second email.
>
> Thanks,
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel