Re: [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg: Add and update debug ASSERTS

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 4 January 2018 at 18:55, Girish Pathak <Girish.Pathak@arm.com> wrote:
> Hi Ard,
>
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Ard Biesheuvel
>> Sent: 23 December 2017 14:12
>> To: Evan Lloyd <Evan.Lloyd@arm.com>
>> Cc: "Matteo.Carlini@arm.com"@arm.com;
>> "leif.lindholm@linaro.org"@arm.com; "nd@arm.com"@arm.com; edk2-
>> devel@lists.01.org; Thomas Abraham <thomas.abraham@arm.com>; Arvind
>> Chauhan <Arvind.Chauhan@arm.com>;
>> "ard.biesheuvel@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.lloyd@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.pathak@arm.com>
>> > Signed-off-by: Evan Lloyd <evan.lloyd@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.

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.



>> >      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/PL111LcdAr
>> mV
>> > Express.c
>> >
>> b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdAr
>> mV
>> > Express.c index
>> >
>> 799fb3fc781ce04bb64cb1fa0b87f262a670ed78..fd4eea8f8e2397bc7d4ddf4cfe
>> 3d
>> > cc97a5109edb 100644
>> > ---
>> >
>> a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdAr
>> 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