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:c06::232; helo=mail-io0-x232.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x232.google.com (mail-io0-x232.google.com [IPv6:2607:f8b0:4001:c06::232]) (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 E6EC02217CE53 for ; Fri, 8 Dec 2017 13:35:10 -0800 (PST) Received: by mail-io0-x232.google.com with SMTP id r13so369480iod.2 for ; Fri, 08 Dec 2017 13:39:45 -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=zMPXEOZ2I4UkOMGuxsAQ5xfYzkBtA+RNK6ZOZzyOGNw=; b=SVFsq+D8JSpqjdTiGl5aodPFBM26uYuUURyvJyM08YyDP38Ra87ZP+zb6u/Cjuk+pH JsGuNTkGWXo9vpzGgnBm414PG0qU6PYKU5e/1ix2+1+PeFrHtpR8exXcnrXltN87lmdt gXbMyh3/TOvOB+8iRaGmojHg2jf1+gNdRJ0rw= 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=zMPXEOZ2I4UkOMGuxsAQ5xfYzkBtA+RNK6ZOZzyOGNw=; b=WZGDIuaYpb+G9Es8kLWuPwyPsFLh77FArp6NwbXoEjlsylOeZZOcIYETd9aft8Yhqy avBWEmQFcon0UQcO5NowDxgh9ClrBSZesVxkGAOigeMd0/2B5O3fUwSTJtKdOl/2XwvJ 8srK2do0vGYtEydZnmgR6a69yOZbskXJrNl05rfZgc2cK8Kg4CcHaWTZQhFgAEkkt3u2 VqJDzRVOfKCX7NVOCs10vh5SOlWkAkuVz7bNjqemlyoDypHGHCZ0fGhOIa6RvB8EFdL1 GTJWhj775MthYrx0OPmJhKbe26NZjVaWHQfNEhEjUpLQOQBaL9egrqszWR+yya9MdD7P 4otg== X-Gm-Message-State: AJaThX7VLBgwyxKZJz0CvlKPo2hQjkt7Ul1rQtDa1xgvbN60ceKKAZan Ejw+bwoYK8gHHJQVftovGJpYTPxLzikrC9tSOgrxug== X-Google-Smtp-Source: AGs4zMatvlpjAzwpvF3wPS0/jRmhOtkJrZEdzIUqQydBZFA6Yeq59Rn7KOtQuVotNyHuowlQNz9OUTbczCQb1H1BY24= X-Received: by 10.107.151.142 with SMTP id z136mr44842840iod.248.1512769184545; Fri, 08 Dec 2017 13:39:44 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.104.16 with HTTP; Fri, 8 Dec 2017 13:39:43 -0800 (PST) In-Reply-To: References: <20170926201529.11644-1-evan.lloyd@arm.com> <20170926201529.11644-5-evan.lloyd@arm.com> From: Ard Biesheuvel Date: Fri, 8 Dec 2017 21:39:43 +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: Fri, 08 Dec 2017 21:35:11 -0000 Content-Type: text/plain; charset="UTF-8" On 7 December 2017 at 16:53, Alexei Fedorov wrote: > As expected with new ASSERT_EFI_ERROR () definition compiler generates 1 > conditional branch at the start: > > > // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79: > ASSERT_EFI_ERROR (Status); > .loc 1 79 0 > tbz x0, #63, .L4 // Status, > Thanks for confirming! > > ________________________________ > From: Ard Biesheuvel > Sent: 07 December 2017 15:10:05 > To: Alexei Fedorov > Cc: Evan Lloyd; edk2-devel@lists.01.org; leif.lindholm@linaro.org@arm.com; > nd@arm.com@arm.com; ard.biesheuvel@linaro.org@arm.com; > Matteo.Carlini@arm.com@arm.com > Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug > asserts > > 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. > IMPORTANT NOTICE: The contents of this email and any attachments are > confidential and may also be privileged. If you are not the intended > recipient, please notify the sender immediately and do not disclose the > contents to any other person, use it for any purpose, or store or copy the > information in any medium. Thank you.