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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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.