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,
________________________________
From: Ard Biesheuvel <[email protected]>
Sent: 07 December 2017 15:10:05
To: Alexei Fedorov
Cc: Evan Lloyd; [email protected]; [email protected]@arm.com;
[email protected]@arm.com; [email protected]@arm.com;
[email protected]@arm.com
Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug
asserts
On 7 December 2017 at 14:55, Alexei Fedorov <[email protected]> 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.
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
