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

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

On 7 December 2017 at 16:53, Alexei Fedorov <Alexei.Fedorov@arm.com> 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 <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.