Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts

[Alexei Fedorov <Alexei.Fedorov@arm.com>]

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 <ard.biesheuvel@linaro.org>
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 <Alexei.Fedorov@arm.com> 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.