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

[Leif Lindholm <leif.lindholm@linaro.org>]

On Thu, Jan 04, 2018 at 07:24:20PM +0000, Ard Biesheuvel wrote:
> 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.

I don't have a strong enough opinion on this that it should be
listened to if you both agree.

It's just the "if error, then assert if error" that breaks up the
parsing flow for me. Could it make sense to use ASSERT_RETURN_ERROR
instead?

So
      if (EFI_ERROR (Status)) {
        ASSERT_RETURN_ERROR (Status);
      }

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

Apologies for the even more ridicilous delay.
Err, I need to stop taking holidays or something.

Don't worry, I won't have another one for a couple of weeks :]
First Linaro Connect, then Plugfest.

/
    Leif