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

[Laszlo Ersek <lersek@redhat.com>]

(+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."

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