On Fri, Dec 01, 2017 at 05:34:52PM +0000, Ard Biesheuvel wrote:
> On 1 December 2017 at 16:33, Evan Lloyd <[email protected]> wrote:
> > Responses inline:
> >
> >> -----Original Message-----
> >> From: Ard Biesheuvel [mailto:[email protected]]
> >> Sent: 13 October 2017 08:33
> >> To: Evan Lloyd <[email protected]>
> >> Cc: [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 26 September 2017 at 21:15, <[email protected]> wrote:
> >> > From: Girish Pathak <[email protected]>
> >> >
> >> > 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 <[email protected]>
> >> > Signed-off-by: Evan Lloyd <[email protected]>
> >> > ---
> >> >
> >> 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
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
