From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::22f; helo=mail-it0-x22f.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x22f.google.com (mail-it0-x22f.google.com [IPv6:2607:f8b0:4001:c0b::22f]) (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 32B31221EA0A0 for ; Thu, 7 Dec 2017 07:05:33 -0800 (PST) Received: by mail-it0-x22f.google.com with SMTP id p139so15512133itb.1 for ; Thu, 07 Dec 2017 07:10:07 -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; bh=YJtiYTBbtb7XCqZF4eM/22S25LQb8PlOY3MBvyz+cxc=; b=Wa33gLjMG8PKRYwmwupcMUZxCieTtDvkyeoI5xqAcsLNlxyUckDS/qGIrN8xprbF4C ytPehrdQCc6MCRTPVEoLlvG+Y7p3z43ISW7hEHS8zuh/9VcXrNQtFpvVD8K24YhSIrAf PQjzugXjJHzjA/j/ITGYuOoBlgPDWdiDwtFyg= 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; bh=YJtiYTBbtb7XCqZF4eM/22S25LQb8PlOY3MBvyz+cxc=; b=l69MSxux5Hmp8CQu9hfgbT1leN+a+SlHkoTemugjXVbeWwLT2mjPQvxHTBMbgg7Ge2 ugi+H23xATtjFcjM+TjNwQN2gW5MdDa4lzKok1tLMgFuHJHD2vPdldERu5DU7/Lsa3T7 MImcogy5v9VDHkAqc3/PxSC8NwtGEGzvEC4Z9/qPNnHSG7BEFUs2TfdlWI8HcMtpfVCl eVRYrqvY5GqB6tVkBwQrQRqKADznLbAMo+ZGt7CnQ8rHEOq4oVkv+7g0SE9sKu7pENpt +fP+maeXHsbb+Mx4Erjvm3UeDXgvslJEJajJV6pbBKLXHz5mD+FpI21Qbm3r9nvIIcwp 86xg== X-Gm-Message-State: AKGB3mJ15jmnFRW0xuu+9cKKUUYlxb+L4xbXNgzmI+avJKvoCCig7urP btB6/FFLChxFjqdOgZ796VWoHbHWhN654no/+o/H2Q== X-Google-Smtp-Source: AGs4zMaIrMcm+K1d36o3uVZBUBzXEDB+g0ONR8qTZj5KBN53WtVdZLX19xizhkr2TbHSKzBn9pH4DRSMix84edKQl1k= X-Received: by 10.36.145.203 with SMTP id i194mr1700684ite.73.1512659406590; Thu, 07 Dec 2017 07:10:06 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.104.16 with HTTP; Thu, 7 Dec 2017 07:10:05 -0800 (PST) In-Reply-To: References: <20170926201529.11644-1-evan.lloyd@arm.com> <20170926201529.11644-5-evan.lloyd@arm.com> From: Ard Biesheuvel Date: Thu, 7 Dec 2017 15:10:05 +0000 Message-ID: To: Alexei Fedorov Cc: Evan Lloyd , "edk2-devel@lists.01.org" , "leif.lindholm@linaro.org@arm.com" <"leif.lindholm@linaro.org"@arm.com>, "nd@arm.com@arm.com" <"nd@arm.com"@arm.com>, "ard.biesheuvel@linaro.org@arm.com" <"ard.biesheuvel@linaro.org"@arm.com>, "Matteo.Carlini@arm.com@arm.com" <"Matteo.Carlini@arm.com"@arm.com> Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 Dec 2017 15:05:34 -0000 Content-Type: text/plain; charset="UTF-8" On 7 December 2017 at 14:55, Alexei Fedorov wrote: > Hi, > > I've compiled current HdLcd.c code with different optimisation levels for > DEBUG build using GCC 7.2.1, and the assembler code below was generated for: > > ASSERT_EFI_ERROR (Status); > if (EFI_ERROR( Status )) { > return EFI_DEVICE_ERROR; > } > > -O0 (default DEBUG option for AARCH64 before Ard's patch): > > > str x0, [x29, 72] //, Status > > // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79: > ASSERT_EFI_ERROR (Status); > .loc 1 79 0 > bl DebugAssertEnabled // > and w0, w0, 255 // _1, tmp150 > cmp w0, 0 // _1, > beq .L4 //, > ldr x0, [x29, 72] // Status.9_2, Status > cmp x0, 0 // Status.9_2, > bge .L4 //, > > 2. -Os: > > mov x19, x0 // Status, > // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79: > ASSERT_EFI_ERROR (Status); > .loc 1 79 0 > bl DebugAssertEnabled // > .LVL15: > tst w0, 255 // > beq .L4 //, > tbz x19, #63, .L8 // Status, > > 3. -O3: > > mov x19, x0 // Status, > .LVL14: > // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79: > ASSERT_EFI_ERROR (Status); > bl DebugAssertEnabled // > .LVL15: > tst w0, 255 // > beq .L5 //, > tbnz x19, #63, .L26 // Status, > > with DebugAssertEnabled() compiled as: > > DebugAssertEnabled: > .LFB4: > .loc 1 203 0 > .cfi_startproc > // r:\edk2\MdePkg\Library\BaseDebugLibSerialPort\DebugLib.c:204: return > (BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) & > DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED) != 0); > .loc 1 204 0 > adrp x0, _gPcd_FixedAtBuild_PcdDebugPropertyMask // tmp80, > add x0, x0, :lo12:_gPcd_FixedAtBuild_PcdDebugPropertyMask // > tmp79, tmp80, > ldrb w0, [x0] // _gPcd_FixedAtBuild_PcdDebugPropertyMask.4_1, > _gPcd_FixedAtBuild_PcdDebugPropertyMask > and w0, w0, 1 // _3, _2, > cmp w0, 0 // _3, > cset w0, ne // tmp82, > and w0, w0, 255 // _4, tmp81 > // r:\edk2\MdePkg\Library\BaseDebugLibSerialPort\DebugLib.c:205: } > .loc 1 205 0 > ret > > As you can see each "ASSERT_EFI_ERROR (Status)" macro requires > DebugAssertEnabled() call taking 8 instructions by itself + minimum 3 > instructions (for -Os, -O3) for Status check, which will be performed by "if > (EFI_ERROR( Status ))" anyway. > Please correct me if I'm wrong assuming that placing > ASSERT_EFI_ERROR (Status) > inside > if (EFI_ERROR( Status )) { > statement is an optimisation improvement. > Thanks for digging this up. This appears to be an oversight in the definition of the ASSERT_EFI_ERROR () macro, which is currently defined as #if !defined(MDEPKG_NDEBUG) #define ASSERT_EFI_ERROR(StatusParameter) \ do { \ if (DebugAssertEnabled ()) { \ if (EFI_ERROR (StatusParameter)) { \ which is obviously the wrong way around, given that the compiler can never optimize away the function call, due to its potential side effects (which it doesn't have) So I will propose to fix this, by swapping the two if () statements. Please let me know if the issue is still reproducible with that patch applied.