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

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Dec 01, 2017 at 05:34:52PM +0000, Ard Biesheuvel wrote:
> On 1 December 2017 at 16:33, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
> > Responses inline:
> >
> >> -----Original Message-----
> >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> >> Sent: 13 October 2017 08:33
> >> To: Evan Lloyd <Evan.Lloyd@arm.com>
> >> Cc: edk2-devel@lists.01.org; "ard.biesheuvel@linaro.org"@arm.com;
> >> "leif.lindholm@linaro.org"@arm.com;
> >> "Matteo.Carlini@arm.com"@arm.com; "nd@arm.com"@arm.com
> >> Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add
> >> debug asserts
> >>
> >> On 26 September 2017 at 21:15,  <evan.lloyd@arm.com> wrote:
> >> > From: Girish Pathak <girish.pathak@arm.com>
> >> >
> >> > This change adds some debug assertions e.g to catch NULL pointer
> >> > errors missing in PL11Lcd and HdLcd modules.
> >> >
> >> > This change also improves related error handling code.
> >> >
> >> > Contributed-under: TianoCore Contribution Agreement 1.1
> >> > Signed-off-by: Girish Pathak <girish.pathak@arm.com>
> >> > Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> >> > ---
> >> >
> >> ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdAr
> >> mVExpress.c       | 44 ++++++++++++++++++--
> >> >
> >> ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL11
> >> 1LcdArmVExpress.c | 43 ++++++++++++++++++-
> >> >  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> >> |  8 ++--
> >> >  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> >> |  8 ++--
> >> >  4 files changed, 90 insertions(+), 13 deletions(-)
> >> >
> >> > diff --git
> >> >
> >> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> ArmVE
> >> > xpress.c
> >> >
> >> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> ArmVE
> >> > xpress.c index
> >> >
> >> b9859a56988f7e5be0adbaa49048a683fe586bfe..58dd9f0c77e1bc9af559a
> >> 71d0c7c
> >> > ce72d71c6da5 100644
> >> > ---
> >> >
> >> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> ArmVE
> >> > xpress.c
> >> > +++
> >> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> A
> >> > +++ rmVExpress.c
> >> > @@ -140,6 +140,7 @@ LcdPlatformInitializeDisplay (
> >> >    *                                 buffer in bytes
> >> >    *
> >> >    * @retval EFI_SUCCESS             Frame buffer memory allocation success.
> >> > +  * @retval EFI_INVALID_PARAMETER   VramBaseAddress or VramSize
> >> are NULL.
> >> >    * @retval !(EFI_SUCCESS)          Other errors.
> >> >  **/
> >> >  EFI_STATUS
> >> > @@ -151,6 +152,13 @@ LcdPlatformGetVram (
> >> >    EFI_STATUS              Status;
> >> >    EFI_ALLOCATE_TYPE       AllocationType;
> >> >
> >> > +  // Check VramBaseAddress and VramSize are not NULL.
> >> > +  if (VramBaseAddress == NULL || VramSize == NULL) {
> >> > +    ASSERT (VramBaseAddress != NULL);
> >> > +    ASSERT (VramSize != NULL);
> >> > +    return EFI_INVALID_PARAMETER;
> >> > +  }
> >> > +
> >> >    // Set the vram size
> >> >    *VramSize = LCD_VRAM_SIZE;
> >> >
> >> > @@ -169,6 +177,7 @@ LcdPlatformGetVram (
> >> >                    VramBaseAddress
> >> >                    );
> >> >    if (EFI_ERROR (Status)) {
> >> > +    ASSERT_EFI_ERROR (Status);
> >> >      return Status;
> >> >    }
> >> >
> >> > @@ -179,8 +188,8 @@ LcdPlatformGetVram (
> >> >                    *VramSize,
> >> >                    EFI_MEMORY_WC
> >> >                    );
> >> > -  ASSERT_EFI_ERROR (Status);
> >> >    if (EFI_ERROR (Status)) {
> >> > +    ASSERT_EFI_ERROR (Status);
> >>
> >> What is the point of this change?
> > [[Evan Lloyd]] It is a minor efficiency improvement.  Since the
> > ASSERT can only fire when the if condition is true, it removes a
> > duplicated test from the main (non-error) code flow.  This is
> > irrelevant on hardware, but actually significant when running
> > debug builds on an emulator environment.
> 
> Fair enough. But I'd prefer to finally fix the DEBUG vs NOOPT build
> targets for ARM/AARCH64. DEBUG was never intended to be -O0 (and it is
> not on X86 either).
> 
> Code is complex enough as it is, and how to move trivial tests like
> this around for 'performance' reasons is a dimension I would like to
> avoid.

Echoing from an irc discussion with Ard:
I think NOOPT (and a !-O0 DEBUG) would be good to have for the ARM
architectures as well, both for GCC and CLANG.

Evan, have you done any investigation into the impact of optimization
on emulation performance?

My completely unfounded guess would be -O1 should be a good fit for
that environment, and hence for DEBUG. It would remove some of the
more insane bloat of -O0 without starting heavy inlining.
But as always, real numbers beat guesses.

(If -O2 is better than -O1, is -Os better than either?)

/
    Leif