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]>; [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
