On Thu, Jan 04, 2018 at 07:24:20PM +0000, Ard Biesheuvel wrote:
> 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/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);
}
> 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
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
