Re: [PATCH] MdePkg/BaseSynchronizationLib: fix XADD operands in GCC IA32/X64 assembly

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

On 9/26/2018 5:05 PM, Laszlo Ersek wrote:
> Hi,
> 
> On 09/25/18 21:48, Laszlo Ersek wrote:
>> Currently, "gcc-4.8.5-28.el7_5.1.x86_64" generates the following code for
>> me, from the XADD inline assembly added to "X64/GccInline.c" in commit
>> 17634d026f96:
>>
>>> 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 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.
> 
> may I get a fast review for this patch, please? The regression from
> commit 17634d026f96 prevents OVMF from booting.

Sure. Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
> 
> Thanks
> Laszlo
> 


-- 
Thanks,
Ray