> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: 04 January 2018 19:24
> To: Girish Pathak <girish.pat...@arm.com>
> Cc: Evan Lloyd <evan.ll...@arm.com>; Matteo Carlini
> <matteo.carl...@arm.com>; nd <n...@arm.com>; edk2-devel@lists.01.org;
> Thomas Abraham <thomas.abra...@arm.com>; Arvind Chauhan
> <arvind.chau...@arm.com>; leif.lindh...@linaro.org
> 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 <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/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...
>
>
>
> >> > 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
> >> > 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
