From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::143; helo=mail-it1-x143.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x143.google.com (mail-it1-x143.google.com [IPv6:2607:f8b0:4864:20::143]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 83DB82118847D for ; Wed, 7 Nov 2018 06:45:35 -0800 (PST) Received: by mail-it1-x143.google.com with SMTP id b7-v6so23135263itd.5 for ; Wed, 07 Nov 2018 06:45:35 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=14Dcb8ifojYIPDBUC2J3ab/G5S1mLEbeUimr21POja4=; b=ZPTP9MCX3J64ntESKO7PaAAcQ2Q0bLaB4vp4Y4Fh/Elu/inNiACTFc0zkNnWewuAWy 0aZBO91FFvJGRJSMxnmJ7NStmjAW0buwg7/0grMAdWGfLA/r4m885FYJGsRcbdvUeUOF vSOjXPsD1EMsg9tD224vuUQzZ6i5TKHbj9UBg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=14Dcb8ifojYIPDBUC2J3ab/G5S1mLEbeUimr21POja4=; b=InY3CpCw4Jtc1DK7VqRui2w2DGIYTocd98KqF+5uOqULjJq4h91/9k/2/zW3TsXKrt lVrK7naFXhdGLV47/Aa+LbyITrP4ObxMhbmNNrU2JlYrvYgUybNG2ZF5KsmIlem+0p3k pUOlu0QtZPdCpKOPhmZfiGjXjXI7O5kwawOBF+ibDUmJ4Fezu8hFo/z71NfhjjOLggs/ sap+gcM+tkrbz1F5aCntnYITKKqEsogUdcMDG3tJb+g+DIWeDjYwenZW/XponQN6YkQw CCcsxU4d1W+/x9md2fVWCcuNQj8tcC3xa7gcOKOnJX6muS+P05nKOIn4GxFcKtlzG9Jj dPGQ== X-Gm-Message-State: AGRZ1gLmZYkcxdWV6/wsX1CviRXxcizVgN0blbzAhKa1OerT/x7db74I SJ6u3DrXQsFFLGeyUsagsgtJwQi/c6NGuz+4aG9q/Q== X-Google-Smtp-Source: AJdET5ejcfj16G8GAxAW5+ujpWKLcOrCqz/SR7T+yn2wvUCuQw45QOy4ub0WsthyzpcQkNFxqWTUPL13hMJaK3jT9YI= X-Received: by 2002:a02:6f41:: with SMTP id b1-v6mr467942jae.62.1541601934232; Wed, 07 Nov 2018 06:45:34 -0800 (PST) MIME-Version: 1.0 Received: by 2002:a6b:4f16:0:0:0:0:0 with HTTP; Wed, 7 Nov 2018 06:45:33 -0800 (PST) In-Reply-To: <555b0649-a067-472e-2b81-b52457deb67b@redhat.com> References: <20181107040346.153720-1-ruiyu.ni@intel.com> <555b0649-a067-472e-2b81-b52457deb67b@redhat.com> From: Ard Biesheuvel Date: Wed, 7 Nov 2018 15:45:33 +0100 Message-ID: To: Laszlo Ersek Cc: Ruiyu Ni , "edk2-devel@lists.01.org" , Michael Kinney , Liming Gao Subject: Re: [PATCH] MdePkg/BaseSynchronizationLib XCODE: fix InternalSync[De|In]crement X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 07 Nov 2018 14:45:35 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On 7 November 2018 at 15:31, Laszlo Ersek wrote: > (+Andrew) > > Hi Ray, > > On 11/07/18 05:03, Ruiyu Ni wrote: >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1303 >> >> 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" >> : "=3Da" (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 "=3Da" 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? "=3D&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 >> Cc: Liming Gao >> Cc: Michael Kinney >> Cc: Laszlo Ersek >> Cc: Philippe Mathieu-Daud=C3=A9 >> --- >> MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c | 12 ++++++++---= - >> 1 file changed, 8 insertions(+), 4 deletions(-) >> >> diff --git a/MdePkg/Library/BaseSynchronizationLib/Ia32/GccInline.c b/Md= ePkg/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" >> - : "=3Da" (Result), // %0 >> + "mov %%eax, %0 \n\t" >> + : "=3Dr" (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" >> - : "=3Da" (Result), // %0 >> + "mov %%eax, %0 \n\t" >> + : "=3Dr" (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