On 4 January 2018 at 18:55, Girish Pathak <girish.pat...@arm.com> wrote: > Hi Ard, > >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of >> Ard Biesheuvel >> Sent: 23 December 2017 14:12 >> To: Evan Lloyd <evan.ll...@arm.com> >> Cc: "matteo.carl...@arm.com"@arm.com; >> "leif.lindh...@linaro.org"@arm.com; "n...@arm.com"@arm.com; edk2- >> de...@lists.01.org; Thomas Abraham <thomas.abra...@arm.com>; Arvind >> Chauhan <arvind.chau...@arm.com>; >> "ard.biesheu...@linaro.org"@arm.com >> Subject: Re: [edk2] [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg: Add >> and update debug ASSERTS >> >> On 22 December 2017 at 19:08, <evan.ll...@arm.com> wrote: >> > From: Girish Pathak <girish.pathak at arm.com> >> > >> > This change adds some debug assertions e.g to catch NULL pointer >> > errors missing in PL11Lcd and HdLcd platform libraries. >> > >> > Contributed-under: TianoCore Contribution Agreement 1.1 >> > Signed-off-by: Girish Pathak <girish.pat...@arm.com> >> > Signed-off-by: Evan Lloyd <evan.ll...@arm.com> >> > --- >> > >> Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpr >> ess.c | 22 +++++++++++++++++- >> > >> > >> Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArm >> VEx >> > press.c | 24 +++++++++++++++++++- >> > 2 files changed, 44 insertions(+), 2 deletions(-) >> > >> > diff --git >> > >> a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx >> pres >> > s.c >> > >> b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx >> pres >> > s.c index >> > >> 6afd764897f49c64490ce891682f99bb0f5d993b..a8fe8696da0653017ce9fa6e4a >> 86 >> > caf283bc04c9 100644 >> > --- >> > >> a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx >> pres >> > s.c >> > +++ >> b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVEx >> > +++ press.c >> > @@ -153,6 +153,9 @@ LcdPlatformGetVram ( >> > EFI_STATUS Status; >> > EFI_ALLOCATE_TYPE AllocationType; >> > >> > + ASSERT (VramBaseAddress != NULL); >> > + ASSERT (VramSize != NULL); >> > + >> > // Set the vram size >> > *VramSize = LCD_VRAM_SIZE; >> > >> > @@ -171,6 +174,7 @@ LcdPlatformGetVram ( >> > VramBaseAddress >> > ); >> > if (EFI_ERROR (Status)) { >> > + ASSERT (FALSE); >> > return Status; >> > } >> > >> > @@ -181,8 +185,8 @@ LcdPlatformGetVram ( >> > *VramSize, >> > EFI_MEMORY_WC >> > ); >> > - ASSERT_EFI_ERROR (Status); >> > if (EFI_ERROR (Status)) { >> > + ASSERT (FALSE); >> >> As in the sibling patch against EDK2, this patch makes it more difficult to >> figure out what went wrong when you hit the ASSERT. >> ASSERT_EFI_ERROR prints the value of Status, ASSERT(FALSE) only prints >> '0 != 1' >> > > This change(and other similar changes) is in response to review comments on > patch v1 > https://lists.01.org/pipermail/edk2-devel/2017-October/015995.html > > with above reference, Can you please confirm if we should revert to the patch > v1 version ? >
I guess Leif and I are in disagreement here. In particular, I think his comment """ ASSERT (FALSE)? (You already know Status is an EFI_ERROR, and a console message saying ASSERT (Status) is not getting you out of looking at the source code to find out what happened.) """ is misguided, given that ASSERT_EFI_ERROR (Status) will actually print the value of Status to the debug console. However, the objections against putting function calls in ASSERT()s are justified: ASSERT() should not have side effects if its condition is met, and function calls may have side effects. I suppose we should wait for Leif to return on the 22nd before proceeding with the review. Apologies for the confusion, and for the delay. >> > gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize)); >> > return Status; >> > } >> > @@ -221,6 +225,7 @@ LcdPlatformSetMode ( >> > EFI_STATUS Status; >> > >> > if (ModeNumber >= LcdPlatformGetMaxMode ()) { >> > + ASSERT (FALSE); >> >> These are fine: the code itself explains adequately which condition triggered >> the ASSERT to fire. >> >> > return EFI_INVALID_PARAMETER; >> > } >> > >> > @@ -279,7 +284,10 @@ LcdPlatformQueryMode ( >> > OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST Info >> > ) >> > { >> > + ASSERT (Info != NULL); >> > + >> > if (ModeNumber >= LcdPlatformGetMaxMode ()) { >> > + ASSERT (FALSE); >> > return EFI_INVALID_PARAMETER; >> > } >> > >> > @@ -343,7 +351,18 @@ LcdPlatformGetTimings ( >> > OUT UINT32 * CONST VFrontPorch >> > ) >> > { >> > + // One of the pointers is NULL >> > + ASSERT (HRes != NULL); >> > + ASSERT (HSync != NULL); >> > + ASSERT (HBackPorch != NULL); >> > + ASSERT (HFrontPorch != NULL); >> > + ASSERT (VRes != NULL); >> > + ASSERT (VSync != NULL); >> > + ASSERT (VBackPorch != NULL); >> > + ASSERT (VFrontPorch != NULL); >> > + >> > if (ModeNumber >= LcdPlatformGetMaxMode ()) { >> > + ASSERT (FALSE); >> > return EFI_INVALID_PARAMETER; >> > } >> > >> > @@ -376,6 +395,7 @@ LcdPlatformGetBpp ( >> > ) >> > { >> > if (ModeNumber >= LcdPlatformGetMaxMode ()) { >> > + ASSERT (FALSE); >> > return EFI_INVALID_PARAMETER; >> > } >> > >> > diff --git >> > >> a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdAr >> mV >> > Express.c >> > >> b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdAr >> mV >> > Express.c index >> > >> 799fb3fc781ce04bb64cb1fa0b87f262a670ed78..fd4eea8f8e2397bc7d4ddf4cfe >> 3d >> > cc97a5109edb 100644 >> > --- >> > >> a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdAr >> mV >> > Express.c >> > +++ >> b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111Lcd >> > +++ ArmVExpress.c >> > @@ -205,6 +205,9 @@ LcdPlatformGetVram ( >> > >> > Status = EFI_SUCCESS; >> > >> > + ASSERT (VramBaseAddress != NULL); >> > + ASSERT (VramSize != NULL); >> > + >> > // Is it on the motherboard or on the daughterboard? >> > switch (PL111_CLCD_SITE) { >> > >> > @@ -225,6 +228,7 @@ LcdPlatformGetVram ( >> > VramBaseAddress >> > ); >> > if (EFI_ERROR (Status)) { >> > + ASSERT (FALSE); >> > return Status; >> > } >> > >> > @@ -235,8 +239,8 @@ LcdPlatformGetVram ( >> > *VramSize, >> > EFI_MEMORY_WC >> > ); >> > - ASSERT_EFI_ERROR (Status); >> > if (EFI_ERROR (Status)) { >> > + ASSERT (FALSE); >> > gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES >> (*VramSize)); >> > return Status; >> > } >> > @@ -294,6 +298,7 @@ LcdPlatformSetMode ( >> > UINT32 SysId; >> > >> > if (ModeNumber >= LcdPlatformGetMaxMode ()) { >> > + ASSERT (FALSE); >> > return EFI_INVALID_PARAMETER; >> > } >> > >> > @@ -369,7 +374,10 @@ LcdPlatformQueryMode ( >> > OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST Info >> > ) >> > { >> > + ASSERT (Info != NULL); >> > + >> > if (ModeNumber >= LcdPlatformGetMaxMode ()) { >> > + ASSERT (FALSE); >> > return EFI_INVALID_PARAMETER; >> > } >> > >> > @@ -433,7 +441,18 @@ LcdPlatformGetTimings ( >> > OUT UINT32 * CONST VFrontPorch >> > ) >> > { >> > + // One of the pointers is NULL >> > + ASSERT (HRes != NULL); >> > + ASSERT (HSync != NULL); >> > + ASSERT (HBackPorch != NULL); >> > + ASSERT (HFrontPorch != NULL); >> > + ASSERT (VRes != NULL); >> > + ASSERT (VSync != NULL); >> > + ASSERT (VBackPorch != NULL); >> > + ASSERT (VFrontPorch != NULL); >> > + >> > if (ModeNumber >= LcdPlatformGetMaxMode ()) { >> > + ASSERT (FALSE); >> > return EFI_INVALID_PARAMETER; >> > } >> > >> > @@ -465,7 +484,10 @@ LcdPlatformGetBpp ( >> > OUT LCD_BPP * CONST Bpp >> > ) >> > { >> > + ASSERT (Bpp != NULL); >> > + >> > if (ModeNumber >= LcdPlatformGetMaxMode ()) { >> > + ASSERT (FALSE); >> > return EFI_INVALID_PARAMETER; >> > } >> > >> > -- >> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759") >> > >> > _______________________________________________ >> > edk2-devel mailing list >> > edk2-devel@lists.01.org >> > https://lists.01.org/mailman/listinfo/edk2-devel >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel