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

[Evan Lloyd <Evan.Lloyd@arm.com>]

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.
I suspect, though, that Leif is capable of being reasonable if pressed (and offered beer at Plugfest).

Regards,
Evan

> -----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