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

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

On 5 March 2018 at 15:08, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
> In that case, would you be happy to take Girish's patches with the ASSERTs done your (our) way?
> Leif can fulminate about it when he gets back, if he really feels that strongly.

Why? Because it is almost the end of the quarter? Can't it wait?

> I suspect, though, that Leif is capable of being reasonable if pressed (and offered beer at Plugfest).
>

I am not going to push something I know Leif disapproves of in its
current form in his absence, sorry.

>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: 02 March 2018 19:07
>> To: Evan Lloyd <Evan.Lloyd@arm.com>
>> Cc: leif.lindholm@linaro.org; Girish Pathak <Girish.Pathak@arm.com>;
>> Matteo Carlini <Matteo.Carlini@arm.com>; nd <nd@arm.com>; edk2-
>> devel@lists.01.org
>> Subject: Re: [edk2] [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg:
>> Add and update debug ASSERTS
>>
>> On 28 February 2018 at 20:27, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
>> > Hi Leif, Ard.
>> > Can I get you two argue out the pros and cons of the "ASSERT(FALSE)"
>> debate, please.
>>
>> I can argue the cons if you like. For the pros, you'll have to wait for Leif to
>> return from holiday (in a week or two AFAIK)
>>
>> > (see
>> > https://lists.01.org/pipermail/edk2-devel/2018-January/019788.html)
>> > For what it is worth, our (surprisingly unanimous) opinion is that, since
>> the ASSERT is only there to help spot a problem, then the more information
>> reported the better.  The only benefits of ASSERT(FALSE) would be a smaller
>> debug image and minor efficiency improvement on the path to the crash.
>> >
>> > Regards,
>> > Evan
>> >
>> >> -----Original Message-----
>> >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> >> Sent: 04 January 2018 19:55
>> >> To: Evan Lloyd <Evan.Lloyd@arm.com>
>> >> Cc: Girish Pathak <Girish.Pathak@arm.com>; Matteo Carlini
>> >> <Matteo.Carlini@arm.com>; nd <nd@arm.com>; edk2-
>> devel@lists.01.org;
>> >> Thomas Abraham <thomas.abraham@arm.com>; Arvind Chauhan
>> >> <Arvind.Chauhan@arm.com>; leif.lindholm@linaro.org
>> >> Subject: Re: [edk2] [PATCH edk2-platforms v2 06/18] ARM/VExpressPkg:
>> >> Add and update debug ASSERTS
>> >>
>> >> On 4 January 2018 at 19:51, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
>> >> >
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> >> >> Sent: 04 January 2018 19:24
>> >> >> To: Girish Pathak <Girish.Pathak@arm.com>
>> >> >> Cc: Evan Lloyd <Evan.Lloyd@arm.com>; Matteo Carlini
>> >> >> <Matteo.Carlini@arm.com>; nd <nd@arm.com>; edk2-
>> >> devel@lists.01.org;
>> >> >> Thomas Abraham <thomas.abraham@arm.com>; Arvind Chauhan
>> >> >> <Arvind.Chauhan@arm.com>; leif.lindholm@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.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/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.ht
>> >> >> > ml
>> >> >> >
>> >> >> > 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
>> >> >> >> > 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