On 14 March 2018 at 12:24, Leif Lindholm <leif.lindh...@linaro.org> wrote: > On Thu, Jan 04, 2018 at 07:24:20PM +0000, Ard Biesheuvel wrote: >> 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. > > I don't have a strong enough opinion on this that it should be > listened to if you both agree. > > It's just the "if error, then assert if error" that breaks up the > parsing flow for me. Could it make sense to use ASSERT_RETURN_ERROR > instead? > > So > if (EFI_ERROR (Status)) { > ASSERT_RETURN_ERROR (Status); > } >
Do you mean using ASSERT_RETURN_ERROR() rather than ASSERT_EFI_ERROR()? That doesn't make sense: the former is for RETURN_STATUS type variables and the latter for EFI_STATUS >> 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. > > Apologies for the even more ridicilous delay. > Err, I need to stop taking holidays or something. > > Don't worry, I won't have another one for a couple of weeks :] > First Linaro Connect, then Plugfest. > > / > Leif _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
