Hi Leif, Ard. Can I get you two argue out the pros and cons of the "ASSERT(FALSE)" debate, please. (see https://lists.01.org/pipermail/edk2-devel/2018-January/019788.html) For what it is worth, our (surprisingly unanimous) opinion is that, since the ASSERT is only there to help spot a problem, then the more information reported the better. The only benefits of ASSERT(FALSE) would be a smaller debug image and minor efficiency improvement on the path to the crash.
Regards, Evan > -----Original Message----- > From: Ard Biesheuvel [mailto:[email protected]] > Sent: 04 January 2018 19:55 > To: Evan Lloyd <[email protected]> > Cc: Girish Pathak <[email protected]>; Matteo Carlini > <[email protected]>; nd <[email protected]>; [email protected]; > Thomas Abraham <[email protected]>; Arvind Chauhan > <[email protected]>; [email protected] > Subject: Re: [edk2] [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg: > Add and update debug ASSERTS > > On 4 January 2018 at 19:51, Evan Lloyd <[email protected]> wrote: > > > > > >> -----Original Message----- > >> From: Ard Biesheuvel [mailto:[email protected]] > >> Sent: 04 January 2018 19:24 > >> To: Girish Pathak <[email protected]> > >> Cc: Evan Lloyd <[email protected]>; Matteo Carlini > >> <[email protected]>; nd <[email protected]>; edk2- > [email protected]; > >> Thomas Abraham <[email protected]>; Arvind Chauhan > >> <[email protected]>; [email protected] > >> Subject: Re: [edk2] [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg: > >> Add and update debug ASSERTS > >> > >> On 4 January 2018 at 18:55, Girish Pathak <[email protected]> > >> wrote: > >> > Hi Ard, > >> > > >> >> -----Original Message----- > >> >> From: edk2-devel [mailto:[email protected]] On > >> >> Behalf Of Ard Biesheuvel > >> >> Sent: 23 December 2017 14:12 > >> >> To: Evan Lloyd <[email protected]> > >> >> Cc: "[email protected]"@arm.com; > >> >> "[email protected]"@arm.com; "[email protected]"@arm.com; > edk2- > >> >> [email protected]; Thomas Abraham <[email protected]>; > >> Arvind > >> >> Chauhan <[email protected]>; > >> "[email protected]"@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, <[email protected]> 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 <[email protected]> > >> >> > Signed-off-by: Evan Lloyd <[email protected]> > >> >> > --- > >> >> > > >> >> > >> > Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExp > >> r > >> >> ess.c | 22 +++++++++++++++++- > >> >> > > >> >> > > >> >> > >> > Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdAr > >> m > >> >> VEx > >> >> > press.c | 24 +++++++++++++++++++- > >> >> > 2 files changed, 44 insertions(+), 2 deletions(-) > >> >> > > >> >> > diff --git > >> >> > > >> >> > >> > a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVE > >> x > >> >> pres > >> >> > s.c > >> >> > > >> >> > >> > b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVE > >> x > >> >> pres > >> >> > s.c index > >> >> > > >> >> > >> > 6afd764897f49c64490ce891682f99bb0f5d993b..a8fe8696da0653017ce9fa > >> 6e4a > >> >> 86 > >> >> > caf283bc04c9 100644 > >> >> > --- > >> >> > > >> >> > >> > a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVE > >> x > >> >> pres > >> >> > s.c > >> >> > +++ > >> >> > >> > b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVE > >> x > >> >> > +++ 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. > > > > [[Evan Lloyd]] An alternative might be for Girish to take the other route > Leif suggested, and cache the condition in a variable. > > That might be a slight overhead, and the (presumably BOOLEAN) variable > may need careful naming, but... > > > > If we are going to use a boolean to record the result of the comparison, and > ASSERT() on it in the if () block if the comparison is false, I don't see what > the difference is with doing ASSERT (FALSE) directly. > > > >> > >> > >> > >> >> > 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/PL111Lcd > >> Ar > >> >> mV > >> >> > Express.c > >> >> > > >> >> > >> > b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111Lcd > >> Ar > >> >> mV > >> >> > Express.c index > >> >> > > >> >> > >> > 799fb3fc781ce04bb64cb1fa0b87f262a670ed78..fd4eea8f8e2397bc7d4ddf > >> 4cfe > >> >> 3d > >> >> > cc97a5109edb 100644 > >> >> > --- > >> >> > > >> >> > >> > a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111Lcd > >> Ar > >> >> 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 > >> >> > [email protected] > >> >> > https://lists.01.org/mailman/listinfo/edk2-devel > >> >> _______________________________________________ > >> >> edk2-devel mailing list > >> >> [email protected] > >> >> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
