ARM: Update GOP

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

Posted by evan.lloyd@arm.com 7 years, 4 months ago

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/HdLcdArmVExpress.c       | 22 +++++++++++++++++-
 Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c | 24 +++++++++++++++++++-
 2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
index 6afd764897f49c64490ce891682f99bb0f5d993b..a8fe8696da0653017ce9fa6e4a86caf283bc04c9 100644
--- a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
+++ b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.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);
     gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize));
     return Status;
   }
@@ -221,6 +225,7 @@ LcdPlatformSetMode (
   EFI_STATUS            Status;
 
   if (ModeNumber >= LcdPlatformGetMaxMode ()) {
+    ASSERT (FALSE);
     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/PL111LcdArmVExpress.c b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
index 799fb3fc781ce04bb64cb1fa0b87f262a670ed78..fd4eea8f8e2397bc7d4ddf4cfe3dcc97a5109edb 100644
--- a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
+++ b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.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

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

Posted by Ard Biesheuvel 7 years, 4 months ago

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/HdLcdArmVExpress.c       | 22 +++++++++++++++++-
>  Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c | 24 +++++++++++++++++++-
>  2 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> index 6afd764897f49c64490ce891682f99bb0f5d993b..a8fe8696da0653017ce9fa6e4a86caf283bc04c9 100644
> --- a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> +++ b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.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'

>      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/PL111LcdArmVExpress.c b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> index 799fb3fc781ce04bb64cb1fa0b87f262a670ed78..fd4eea8f8e2397bc7d4ddf4cfe3dcc97a5109edb 100644
> --- a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> +++ b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.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

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

Posted by Girish Pathak 7 years, 4 months ago

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 ?

> >      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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Ard Biesheuvel 7 years, 4 months ago

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Evan Lloyd 7 years, 4 months ago


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

> 
> 
> 
> >> >      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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Ard Biesheuvel 7 years, 4 months ago

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.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
>> >> > 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Evan Lloyd 7 years, 2 months ago

Hi Leif, Ard.
Can I get you two argue out the pros and cons of the "ASSERT(FALSE)" debate, please.
(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.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
> >> >> > 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Ard Biesheuvel 7 years, 2 months ago

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.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
>> >> >> > 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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Evan Lloyd 7 years, 2 months ago

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Ard Biesheuvel 7 years, 2 months ago

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Leif Lindholm 7 years, 1 month ago

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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Ard Biesheuvel 7 years, 1 month ago

On 14 March 2018 at 12:24, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> 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);
>       }
>

Do you mean using ASSERT_RETURN_ERROR() rather than ASSERT_EFI_ERROR()?

That doesn't make sense: the former is for RETURN_STATUS type
variables and the latter for EFI_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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

Posted by Leif Lindholm 7 years, 1 month ago

On Wed, Mar 14, 2018 at 12:35:03PM +0000, Ard Biesheuvel wrote:
> >> 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);
> >       }
> >
> 
> Do you mean using ASSERT_RETURN_ERROR() rather than ASSERT_EFI_ERROR()?

Yes.

> That doesn't make sense: the former is for RETURN_STATUS type
> variables and the latter for EFI_STATUS

Right, of course.
Makes the text look nicer though :)

Anyway, I'm OK with the original version.
Just wish we had a less redundant-looking way to describe this.

/
    Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel