[v1] ArmPlatformPkg: Update GOP

[edk2] [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts

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

From: Girish Pathak <girish.pathak@arm.com>

This change adds some debug assertions e.g to catch NULL pointer errors
missing in PL11Lcd and HdLcd modules.

This change also improves related error handling code.

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>
---
 ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c       | 44 ++++++++++++++++++--
 ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c | 43 ++++++++++++++++++-
 ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c                                |  8 ++--
 ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c                             |  8 ++--
 4 files changed, 90 insertions(+), 13 deletions(-)

diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
index b9859a56988f7e5be0adbaa49048a683fe586bfe..58dd9f0c77e1bc9af559a71d0c7cce72d71c6da5 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
@@ -140,6 +140,7 @@ LcdPlatformInitializeDisplay (
   *                                 buffer in bytes
   *
   * @retval EFI_SUCCESS             Frame buffer memory allocation success.
+  * @retval EFI_INVALID_PARAMETER   VramBaseAddress or VramSize are NULL.
   * @retval !(EFI_SUCCESS)          Other errors.
 **/
 EFI_STATUS
@@ -151,6 +152,13 @@ LcdPlatformGetVram (
   EFI_STATUS              Status;
   EFI_ALLOCATE_TYPE       AllocationType;
 
+  // Check VramBaseAddress and VramSize are not NULL.
+  if (VramBaseAddress == NULL || VramSize == NULL) {
+    ASSERT (VramBaseAddress != NULL);
+    ASSERT (VramSize != NULL);
+    return EFI_INVALID_PARAMETER;
+  }
+
   // Set the vram size
   *VramSize = LCD_VRAM_SIZE;
 
@@ -169,6 +177,7 @@ LcdPlatformGetVram (
                   VramBaseAddress
                   );
   if (EFI_ERROR (Status)) {
+    ASSERT_EFI_ERROR (Status);
     return Status;
   }
 
@@ -179,8 +188,8 @@ LcdPlatformGetVram (
                   *VramSize,
                   EFI_MEMORY_WC
                   );
-  ASSERT_EFI_ERROR (Status);
   if (EFI_ERROR (Status)) {
+    ASSERT_EFI_ERROR (Status);
     gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize));
     return Status;
   }
@@ -215,6 +224,7 @@ LcdPlatformSetMode (
   EFI_STATUS            Status;
 
   if (ModeNumber >= LcdPlatformGetMaxMode ()) {
+    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
     return EFI_INVALID_PARAMETER;
   }
 
@@ -264,6 +274,7 @@ LcdPlatformSetMode (
   *
   * @retval EFI_SUCCESS             Success if the requested mode is found.
   * @retval EFI_INVALID_PARAMETER   Requested mode not found.
+  * @retval EFI_INVALID_PARAMETER   Info is NULL.
 **/
 EFI_STATUS
 LcdPlatformQueryMode (
@@ -271,7 +282,9 @@ LcdPlatformQueryMode (
   OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST  Info
   )
 {
-  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
+  if (ModeNumber >= LcdPlatformGetMaxMode () || Info == NULL) {
+    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
+    ASSERT (Info != NULL);
     return EFI_INVALID_PARAMETER;
   }
 
@@ -334,6 +347,28 @@ LcdPlatformGetTimings (
   )
 {
   if (ModeNumber >= LcdPlatformGetMaxMode ()) {
+    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if (HRes == NULL
+    || HSync == NULL
+    || HBackPorch == NULL
+    || HFrontPorch == NULL
+    || VRes == NULL
+    || VSync == NULL
+    || VBackPorch == NULL
+    || VFrontPorch == NULL)
+  {
+    // 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);
     return EFI_INVALID_PARAMETER;
   }
 
@@ -356,6 +391,7 @@ LcdPlatformGetTimings (
   *
   * @retval EFI_SUCCESS             The requested mode is found.
   * @retval EFI_INVALID_PARAMETER   Requested mode not found.
+  * @retval EFI_INVALID_PARAMETER   Bpp is NULL.
 **/
 EFI_STATUS
 LcdPlatformGetBpp (
@@ -363,7 +399,9 @@ LcdPlatformGetBpp (
   OUT LCD_BPP * CONST                    Bpp
   )
 {
-  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
+  if (ModeNumber >= LcdPlatformGetMaxMode () || Bpp == NULL) {
+    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
+    ASSERT (Bpp != NULL);
     return EFI_INVALID_PARAMETER;
   }
 
diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
index 6ae13f06d8b396ea1c67f0bcd735a9d70f476400..5a4abd4c6f9e84d3d690af7233c1cebfe1ad339b 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
@@ -191,6 +191,7 @@ LcdPlatformInitializeDisplay (
   *                                 buffer in bytes
   *
   * @retval EFI_SUCCESS             Frame buffer memory allocation success.
+  * @retval EFI_INVALID_PARAMETER   VramBaseAddress or VramSize is NULL.
   * @retval !(EFI_SUCCESS)          Other errors.
 **/
 EFI_STATUS
@@ -203,6 +204,13 @@ LcdPlatformGetVram (
 
   Status = EFI_SUCCESS;
 
+  // Check VramBaseAddress and VramSize are not NULL.
+  if (VramBaseAddress == NULL || VramSize == NULL) {
+    ASSERT (VramBaseAddress != NULL);
+    ASSERT (VramSize != NULL);
+    return EFI_INVALID_PARAMETER;
+  }
+
   // Is it on the motherboard or on the daughterboard?
   switch (PL111_CLCD_SITE) {
 
@@ -223,6 +231,7 @@ LcdPlatformGetVram (
                     VramBaseAddress
                     );
     if (EFI_ERROR (Status)) {
+      ASSERT_EFI_ERROR (Status);
       return Status;
     }
 
@@ -233,8 +242,8 @@ LcdPlatformGetVram (
                     *VramSize,
                     EFI_MEMORY_WC
                     );
-    ASSERT_EFI_ERROR (Status);
     if (EFI_ERROR (Status)) {
+      ASSERT_EFI_ERROR (Status);
       gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize));
       return Status;
     }
@@ -293,6 +302,7 @@ LcdPlatformSetMode (
   UINT32                SysId;
 
   if (ModeNumber >= LcdPlatformGetMaxMode ()) {
+    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
     return EFI_INVALID_PARAMETER;
   }
 
@@ -359,6 +369,7 @@ LcdPlatformSetMode (
   *                                 (on success).
   *
   * @retval EFI_SUCCESS             Success if the requested mode is found.
+  * @retval EFI_INVALID_PARAMETER   Info is NULL.
   * @retval EFI_INVALID_PARAMETER   Requested mode not found.
 **/
 EFI_STATUS
@@ -367,7 +378,9 @@ LcdPlatformQueryMode (
   OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST Info
   )
 {
-  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
+  if (ModeNumber >= LcdPlatformGetMaxMode () || Info == NULL) {
+    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
+    ASSERT (Info != NULL);
     return EFI_INVALID_PARAMETER;
   }
 
@@ -415,6 +428,7 @@ LcdPlatformQueryMode (
   *
   * @retval EFI_SUCCESS             Success if the requested mode is found.
   * @retval EFI_INVALID_PARAMETER   Requested mode not found.
+  * @retval EFI_INVALID_PARAMETER   One of the OUT parameters is NULL.
 **/
 EFI_STATUS
 LcdPlatformGetTimings (
@@ -430,6 +444,28 @@ LcdPlatformGetTimings (
   )
 {
   if (ModeNumber >= LcdPlatformGetMaxMode ()) {
+    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if (HRes == NULL
+    || HSync == NULL
+    || HBackPorch == NULL
+    || HFrontPorch == NULL
+    || VRes == NULL
+    || VSync == NULL
+    || VBackPorch == NULL
+    || VFrontPorch == NULL)
+  {
+    // 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);
     return EFI_INVALID_PARAMETER;
   }
 
@@ -452,6 +488,7 @@ LcdPlatformGetTimings (
   *
   * @retval EFI_SUCCESS             The requested mode is found.
   * @retval EFI_INVALID_PARAMETER   Requested mode not found.
+  * @retval EFI_INVALID_PARAMETER   Bpp is NULL.
 **/
 EFI_STATUS
 LcdPlatformGetBpp (
@@ -460,6 +497,8 @@ LcdPlatformGetBpp (
   )
 {
   if (ModeNumber >= LcdPlatformGetMaxMode ()) {
+    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
+    ASSERT (Bpp != NULL);
     return EFI_INVALID_PARAMETER;
   }
 
diff --git a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
index 5f950579720fb69e0a481f697a5cc4038158b409..a266671a26f01d31e8ddb0cf7cbfe59d2f4dc49c 100644
--- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
+++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
@@ -109,15 +109,15 @@ LcdSetMode (
              &VBackPorch,
              &VFrontPorch
              );
-  ASSERT_EFI_ERROR (Status);
   if (EFI_ERROR (Status)) {
-    return EFI_DEVICE_ERROR;
+    ASSERT_EFI_ERROR (Status);
+    return Status;
   }
 
   Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
-  ASSERT_EFI_ERROR (Status);
   if (EFI_ERROR (Status)) {
-    return EFI_DEVICE_ERROR;
+    ASSERT_EFI_ERROR (Status);
+    return Status;
   }
 
   BytesPerPixel = GetBytesPerPixel (LcdBpp);
diff --git a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
index 386e6140a69b045f77ee7fa60c4587d8bf4e7d54..f432c8d802614e8a0e4ddab3898f6e0dbf9a1572 100644
--- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
+++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
@@ -110,15 +110,15 @@ LcdSetMode (
              &VBackPorch,
              &VFrontPorch
              );
-  ASSERT_EFI_ERROR (Status);
   if (EFI_ERROR (Status)) {
-    return EFI_DEVICE_ERROR;
+    ASSERT_EFI_ERROR (Status);
+    return Status;
   }
 
   Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
-  ASSERT_EFI_ERROR (Status);
   if (EFI_ERROR (Status)) {
-    return EFI_DEVICE_ERROR;
+    ASSERT_EFI_ERROR (Status);
+    return Status;
   }
 
   // Disable the CLCD_LcdEn bit
-- 
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 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts

Posted by Ard Biesheuvel 7 years, 2 months ago

On 26 September 2017 at 21:15,  <evan.lloyd@arm.com> wrote:
> From: Girish Pathak <girish.pathak@arm.com>
>
> This change adds some debug assertions e.g to catch NULL pointer errors
> missing in PL11Lcd and HdLcd modules.
>
> This change also improves related error handling code.
>
> 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>
> ---
>  ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c       | 44 ++++++++++++++++++--
>  ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c | 43 ++++++++++++++++++-
>  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c                                |  8 ++--
>  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c                             |  8 ++--
>  4 files changed, 90 insertions(+), 13 deletions(-)
>
> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> index b9859a56988f7e5be0adbaa49048a683fe586bfe..58dd9f0c77e1bc9af559a71d0c7cce72d71c6da5 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> @@ -140,6 +140,7 @@ LcdPlatformInitializeDisplay (
>    *                                 buffer in bytes
>    *
>    * @retval EFI_SUCCESS             Frame buffer memory allocation success.
> +  * @retval EFI_INVALID_PARAMETER   VramBaseAddress or VramSize are NULL.
>    * @retval !(EFI_SUCCESS)          Other errors.
>  **/
>  EFI_STATUS
> @@ -151,6 +152,13 @@ LcdPlatformGetVram (
>    EFI_STATUS              Status;
>    EFI_ALLOCATE_TYPE       AllocationType;
>
> +  // Check VramBaseAddress and VramSize are not NULL.
> +  if (VramBaseAddress == NULL || VramSize == NULL) {
> +    ASSERT (VramBaseAddress != NULL);
> +    ASSERT (VramSize != NULL);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    // Set the vram size
>    *VramSize = LCD_VRAM_SIZE;
>
> @@ -169,6 +177,7 @@ LcdPlatformGetVram (
>                    VramBaseAddress
>                    );
>    if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
>      return Status;
>    }
>
> @@ -179,8 +188,8 @@ LcdPlatformGetVram (
>                    *VramSize,
>                    EFI_MEMORY_WC
>                    );
> -  ASSERT_EFI_ERROR (Status);
>    if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);

What is the point of this change?

>      gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize));
>      return Status;
>    }
> @@ -215,6 +224,7 @@ LcdPlatformSetMode (
>    EFI_STATUS            Status;
>
>    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
>      return EFI_INVALID_PARAMETER;
>    }
>
> @@ -264,6 +274,7 @@ LcdPlatformSetMode (
>    *
>    * @retval EFI_SUCCESS             Success if the requested mode is found.
>    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> +  * @retval EFI_INVALID_PARAMETER   Info is NULL.
>  **/
>  EFI_STATUS
>  LcdPlatformQueryMode (
> @@ -271,7 +282,9 @@ LcdPlatformQueryMode (
>    OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST  Info
>    )
>  {
> -  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +  if (ModeNumber >= LcdPlatformGetMaxMode () || Info == NULL) {
> +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());

Please don't put anything that may have a side effect inside ASSERT
(), since they are dropped from RELEASE builds. You can just use
ASSERT (FALSE) here, or use a temp variable.


> +    ASSERT (Info != NULL);
>      return EFI_INVALID_PARAMETER;
>    }
>
> @@ -334,6 +347,28 @@ LcdPlatformGetTimings (
>    )
>  {
>    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());

same here

> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (HRes == NULL
> +    || HSync == NULL
> +    || HBackPorch == NULL
> +    || HFrontPorch == NULL
> +    || VRes == NULL
> +    || VSync == NULL
> +    || VBackPorch == NULL
> +    || VFrontPorch == NULL)
> +  {
> +    // 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);
>      return EFI_INVALID_PARAMETER;
>    }
>
> @@ -356,6 +391,7 @@ LcdPlatformGetTimings (
>    *
>    * @retval EFI_SUCCESS             The requested mode is found.
>    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> +  * @retval EFI_INVALID_PARAMETER   Bpp is NULL.
>  **/
>  EFI_STATUS
>  LcdPlatformGetBpp (
> @@ -363,7 +399,9 @@ LcdPlatformGetBpp (
>    OUT LCD_BPP * CONST                    Bpp
>    )
>  {
> -  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +  if (ModeNumber >= LcdPlatformGetMaxMode () || Bpp == NULL) {
> +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> +    ASSERT (Bpp != NULL);
>      return EFI_INVALID_PARAMETER;
>    }
>
> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> index 6ae13f06d8b396ea1c67f0bcd735a9d70f476400..5a4abd4c6f9e84d3d690af7233c1cebfe1ad339b 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> @@ -191,6 +191,7 @@ LcdPlatformInitializeDisplay (
>    *                                 buffer in bytes
>    *
>    * @retval EFI_SUCCESS             Frame buffer memory allocation success.
> +  * @retval EFI_INVALID_PARAMETER   VramBaseAddress or VramSize is NULL.
>    * @retval !(EFI_SUCCESS)          Other errors.
>  **/
>  EFI_STATUS
> @@ -203,6 +204,13 @@ LcdPlatformGetVram (
>
>    Status = EFI_SUCCESS;
>
> +  // Check VramBaseAddress and VramSize are not NULL.
> +  if (VramBaseAddress == NULL || VramSize == NULL) {
> +    ASSERT (VramBaseAddress != NULL);
> +    ASSERT (VramSize != NULL);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    // Is it on the motherboard or on the daughterboard?
>    switch (PL111_CLCD_SITE) {
>
> @@ -223,6 +231,7 @@ LcdPlatformGetVram (
>                      VramBaseAddress
>                      );
>      if (EFI_ERROR (Status)) {
> +      ASSERT_EFI_ERROR (Status);
>        return Status;
>      }
>
> @@ -233,8 +242,8 @@ LcdPlatformGetVram (
>                      *VramSize,
>                      EFI_MEMORY_WC
>                      );
> -    ASSERT_EFI_ERROR (Status);
>      if (EFI_ERROR (Status)) {
> +      ASSERT_EFI_ERROR (Status);

Please drop this change

>        gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize));
>        return Status;
>      }
> @@ -293,6 +302,7 @@ LcdPlatformSetMode (
>    UINT32                SysId;
>
>    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());

No function calls inside ASSERT () please

>      return EFI_INVALID_PARAMETER;
>    }
>
> @@ -359,6 +369,7 @@ LcdPlatformSetMode (
>    *                                 (on success).
>    *
>    * @retval EFI_SUCCESS             Success if the requested mode is found.
> +  * @retval EFI_INVALID_PARAMETER   Info is NULL.
>    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
>  **/
>  EFI_STATUS
> @@ -367,7 +378,9 @@ LcdPlatformQueryMode (
>    OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST Info
>    )
>  {
> -  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +  if (ModeNumber >= LcdPlatformGetMaxMode () || Info == NULL) {
> +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> +    ASSERT (Info != NULL);
>      return EFI_INVALID_PARAMETER;
>    }
>
> @@ -415,6 +428,7 @@ LcdPlatformQueryMode (
>    *
>    * @retval EFI_SUCCESS             Success if the requested mode is found.
>    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> +  * @retval EFI_INVALID_PARAMETER   One of the OUT parameters is NULL.
>  **/
>  EFI_STATUS
>  LcdPlatformGetTimings (
> @@ -430,6 +444,28 @@ LcdPlatformGetTimings (
>    )
>  {
>    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (HRes == NULL
> +    || HSync == NULL
> +    || HBackPorch == NULL
> +    || HFrontPorch == NULL
> +    || VRes == NULL
> +    || VSync == NULL
> +    || VBackPorch == NULL
> +    || VFrontPorch == NULL)
> +  {
> +    // 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);
>      return EFI_INVALID_PARAMETER;
>    }
>
> @@ -452,6 +488,7 @@ LcdPlatformGetTimings (
>    *
>    * @retval EFI_SUCCESS             The requested mode is found.
>    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> +  * @retval EFI_INVALID_PARAMETER   Bpp is NULL.
>  **/
>  EFI_STATUS
>  LcdPlatformGetBpp (
> @@ -460,6 +497,8 @@ LcdPlatformGetBpp (
>    )
>  {
>    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> +    ASSERT (Bpp != NULL);
>      return EFI_INVALID_PARAMETER;
>    }
>
> diff --git a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> index 5f950579720fb69e0a481f697a5cc4038158b409..a266671a26f01d31e8ddb0cf7cbfe59d2f4dc49c 100644
> --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> @@ -109,15 +109,15 @@ LcdSetMode (
>               &VBackPorch,
>               &VFrontPorch
>               );
> -  ASSERT_EFI_ERROR (Status);
>    if (EFI_ERROR (Status)) {
> -    return EFI_DEVICE_ERROR;
> +    ASSERT_EFI_ERROR (Status);
> +    return Status;
>    }
>
>    Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
> -  ASSERT_EFI_ERROR (Status);
>    if (EFI_ERROR (Status)) {
> -    return EFI_DEVICE_ERROR;
> +    ASSERT_EFI_ERROR (Status);
> +    return Status;
>    }
>
>    BytesPerPixel = GetBytesPerPixel (LcdBpp);
> diff --git a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> index 386e6140a69b045f77ee7fa60c4587d8bf4e7d54..f432c8d802614e8a0e4ddab3898f6e0dbf9a1572 100644
> --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> @@ -110,15 +110,15 @@ LcdSetMode (
>               &VBackPorch,
>               &VFrontPorch
>               );
> -  ASSERT_EFI_ERROR (Status);
>    if (EFI_ERROR (Status)) {
> -    return EFI_DEVICE_ERROR;
> +    ASSERT_EFI_ERROR (Status);
> +    return Status;
>    }
>
>    Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
> -  ASSERT_EFI_ERROR (Status);
>    if (EFI_ERROR (Status)) {
> -    return EFI_DEVICE_ERROR;
> +    ASSERT_EFI_ERROR (Status);
> +    return Status;
>    }
>
>    // Disable the CLCD_LcdEn bit
> --
> 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 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts

Posted by Evan Lloyd 7 years ago

Responses inline:

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: 13 October 2017 08:33
> To: Evan Lloyd <Evan.Lloyd@arm.com>
> Cc: edk2-devel@lists.01.org; "ard.biesheuvel@linaro.org"@arm.com;
> "leif.lindholm@linaro.org"@arm.com;
> "Matteo.Carlini@arm.com"@arm.com; "nd@arm.com"@arm.com
> Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add
> debug asserts
>
> On 26 September 2017 at 21:15,  <evan.lloyd@arm.com> wrote:
> > From: Girish Pathak <girish.pathak@arm.com>
> >
> > This change adds some debug assertions e.g to catch NULL pointer
> > errors missing in PL11Lcd and HdLcd modules.
> >
> > This change also improves related error handling code.
> >
> > 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>
> > ---
> >
> ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdAr
> mVExpress.c       | 44 ++++++++++++++++++--
> >
> ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL11
> 1LcdArmVExpress.c | 43 ++++++++++++++++++-
> >  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> |  8 ++--
> >  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> |  8 ++--
> >  4 files changed, 90 insertions(+), 13 deletions(-)
> >
> > diff --git
> >
> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> ArmVE
> > xpress.c
> >
> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> ArmVE
> > xpress.c index
> >
> b9859a56988f7e5be0adbaa49048a683fe586bfe..58dd9f0c77e1bc9af559a
> 71d0c7c
> > ce72d71c6da5 100644
> > ---
> >
> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> ArmVE
> > xpress.c
> > +++
> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> A
> > +++ rmVExpress.c
> > @@ -140,6 +140,7 @@ LcdPlatformInitializeDisplay (
> >    *                                 buffer in bytes
> >    *
> >    * @retval EFI_SUCCESS             Frame buffer memory allocation success.
> > +  * @retval EFI_INVALID_PARAMETER   VramBaseAddress or VramSize
> are NULL.
> >    * @retval !(EFI_SUCCESS)          Other errors.
> >  **/
> >  EFI_STATUS
> > @@ -151,6 +152,13 @@ LcdPlatformGetVram (
> >    EFI_STATUS              Status;
> >    EFI_ALLOCATE_TYPE       AllocationType;
> >
> > +  // Check VramBaseAddress and VramSize are not NULL.
> > +  if (VramBaseAddress == NULL || VramSize == NULL) {
> > +    ASSERT (VramBaseAddress != NULL);
> > +    ASSERT (VramSize != NULL);
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> >    // Set the vram size
> >    *VramSize = LCD_VRAM_SIZE;
> >
> > @@ -169,6 +177,7 @@ LcdPlatformGetVram (
> >                    VramBaseAddress
> >                    );
> >    if (EFI_ERROR (Status)) {
> > +    ASSERT_EFI_ERROR (Status);
> >      return Status;
> >    }
> >
> > @@ -179,8 +188,8 @@ LcdPlatformGetVram (
> >                    *VramSize,
> >                    EFI_MEMORY_WC
> >                    );
> > -  ASSERT_EFI_ERROR (Status);
> >    if (EFI_ERROR (Status)) {
> > +    ASSERT_EFI_ERROR (Status);
>
> What is the point of this change?
[[Evan Lloyd]] It is a minor efficiency improvement.  Since the ASSERT can only fire when the if condition is true, it removes a duplicated test from the main (non-error) code flow.  This is irrelevant on hardware, but actually significant when running debug builds on an emulator environment.

>
> >      gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize));
> >      return Status;
> >    }
> > @@ -215,6 +224,7 @@ LcdPlatformSetMode (
> >    EFI_STATUS            Status;
> >
> >    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> >      return EFI_INVALID_PARAMETER;
> >    }
> >
> > @@ -264,6 +274,7 @@ LcdPlatformSetMode (
> >    *
> >    * @retval EFI_SUCCESS             Success if the requested mode is found.
> >    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> > +  * @retval EFI_INVALID_PARAMETER   Info is NULL.
> >  **/
> >  EFI_STATUS
> >  LcdPlatformQueryMode (
> > @@ -271,7 +282,9 @@ LcdPlatformQueryMode (
> >    OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST  Info
> >    )
> >  {
> > -  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> > +  if (ModeNumber >= LcdPlatformGetMaxMode () || Info == NULL) {
> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
>
> Please don't put anything that may have a side effect inside ASSERT (), since
> they are dropped from RELEASE builds. You can just use ASSERT (FALSE)
> here, or use a temp variable.

 [[Evan Lloyd]] Agreed

>
>
> > +    ASSERT (Info != NULL);
> >      return EFI_INVALID_PARAMETER;
> >    }
> >
> > @@ -334,6 +347,28 @@ LcdPlatformGetTimings (
> >    )
> >  {
> >    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
>
> same here
[[Evan Lloyd]] Agreed
>
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  if (HRes == NULL
> > +    || HSync == NULL
> > +    || HBackPorch == NULL
> > +    || HFrontPorch == NULL
> > +    || VRes == NULL
> > +    || VSync == NULL
> > +    || VBackPorch == NULL
> > +    || VFrontPorch == NULL)
> > +  {
> > +    // 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);
> >      return EFI_INVALID_PARAMETER;
> >    }
> >
> > @@ -356,6 +391,7 @@ LcdPlatformGetTimings (
> >    *
> >    * @retval EFI_SUCCESS             The requested mode is found.
> >    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> > +  * @retval EFI_INVALID_PARAMETER   Bpp is NULL.
> >  **/
> >  EFI_STATUS
> >  LcdPlatformGetBpp (
> > @@ -363,7 +399,9 @@ LcdPlatformGetBpp (
> >    OUT LCD_BPP * CONST                    Bpp
> >    )
> >  {
> > -  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> > +  if (ModeNumber >= LcdPlatformGetMaxMode () || Bpp == NULL) {
> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> > +    ASSERT (Bpp != NULL);
> >      return EFI_INVALID_PARAMETER;
> >    }
> >
> > diff --git
> >
> a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
> 11Lc
> > dArmVExpress.c
> >
> b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
> 11Lc
> > dArmVExpress.c index
> >
> 6ae13f06d8b396ea1c67f0bcd735a9d70f476400..5a4abd4c6f9e84d3d690a
> f7233c1
> > cebfe1ad339b 100644
> > ---
> >
> a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
> 11Lc
> > dArmVExpress.c
> > +++
> b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
> > +++ 11LcdArmVExpress.c
> > @@ -191,6 +191,7 @@ LcdPlatformInitializeDisplay (
> >    *                                 buffer in bytes
> >    *
> >    * @retval EFI_SUCCESS             Frame buffer memory allocation success.
> > +  * @retval EFI_INVALID_PARAMETER   VramBaseAddress or VramSize is
> NULL.
> >    * @retval !(EFI_SUCCESS)          Other errors.
> >  **/
> >  EFI_STATUS
> > @@ -203,6 +204,13 @@ LcdPlatformGetVram (
> >
> >    Status = EFI_SUCCESS;
> >
> > +  // Check VramBaseAddress and VramSize are not NULL.
> > +  if (VramBaseAddress == NULL || VramSize == NULL) {
> > +    ASSERT (VramBaseAddress != NULL);
> > +    ASSERT (VramSize != NULL);
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> >    // Is it on the motherboard or on the daughterboard?
> >    switch (PL111_CLCD_SITE) {
> >
> > @@ -223,6 +231,7 @@ LcdPlatformGetVram (
> >                      VramBaseAddress
> >                      );
> >      if (EFI_ERROR (Status)) {
> > +      ASSERT_EFI_ERROR (Status);
> >        return Status;
> >      }
> >
> > @@ -233,8 +242,8 @@ LcdPlatformGetVram (
> >                      *VramSize,
> >                      EFI_MEMORY_WC
> >                      );
> > -    ASSERT_EFI_ERROR (Status);
> >      if (EFI_ERROR (Status)) {
> > +      ASSERT_EFI_ERROR (Status);
>
> Please drop this change

 [[Evan Lloyd]] As stated above, this is not inconsequential for some use cases.  The ASSERT can only fire when the if is satisfied, so moving the ASSERT reduces the number of tests executed in normal execution (depending on optimisation level).

>
> >        gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES
> (*VramSize));
> >        return Status;
> >      }
> > @@ -293,6 +302,7 @@ LcdPlatformSetMode (
> >    UINT32                SysId;
> >
> >    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
>
> No function calls inside ASSERT () please
[[Evan Lloyd]] Agreed

>
> >      return EFI_INVALID_PARAMETER;
> >    }
> >
> > @@ -359,6 +369,7 @@ LcdPlatformSetMode (
> >    *                                 (on success).
> >    *
> >    * @retval EFI_SUCCESS             Success if the requested mode is found.
> > +  * @retval EFI_INVALID_PARAMETER   Info is NULL.
> >    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> >  **/
> >  EFI_STATUS
> > @@ -367,7 +378,9 @@ LcdPlatformQueryMode (
> >    OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST Info
> >    )
> >  {
> > -  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> > +  if (ModeNumber >= LcdPlatformGetMaxMode () || Info == NULL) {
> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> > +    ASSERT (Info != NULL);
> >      return EFI_INVALID_PARAMETER;
> >    }
> >
> > @@ -415,6 +428,7 @@ LcdPlatformQueryMode (
> >    *
> >    * @retval EFI_SUCCESS             Success if the requested mode is found.
> >    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> > +  * @retval EFI_INVALID_PARAMETER   One of the OUT parameters is
> NULL.
> >  **/
> >  EFI_STATUS
> >  LcdPlatformGetTimings (
> > @@ -430,6 +444,28 @@ LcdPlatformGetTimings (
> >    )
> >  {
> >    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  if (HRes == NULL
> > +    || HSync == NULL
> > +    || HBackPorch == NULL
> > +    || HFrontPorch == NULL
> > +    || VRes == NULL
> > +    || VSync == NULL
> > +    || VBackPorch == NULL
> > +    || VFrontPorch == NULL)
> > +  {
> > +    // 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);
> >      return EFI_INVALID_PARAMETER;
> >    }
> >
> > @@ -452,6 +488,7 @@ LcdPlatformGetTimings (
> >    *
> >    * @retval EFI_SUCCESS             The requested mode is found.
> >    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> > +  * @retval EFI_INVALID_PARAMETER   Bpp is NULL.
> >  **/
> >  EFI_STATUS
> >  LcdPlatformGetBpp (
> > @@ -460,6 +497,8 @@ LcdPlatformGetBpp (
> >    )
> >  {
> >    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> > +    ASSERT (Bpp != NULL);
> >      return EFI_INVALID_PARAMETER;
> >    }
> >
> > diff --git a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> > b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> > index
> >
> 5f950579720fb69e0a481f697a5cc4038158b409..a266671a26f01d31e8dd
> b0cf7cbf
> > e59d2f4dc49c 100644
> > --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> > +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> > @@ -109,15 +109,15 @@ LcdSetMode (
> >               &VBackPorch,
> >               &VFrontPorch
> >               );
> > -  ASSERT_EFI_ERROR (Status);
> >    if (EFI_ERROR (Status)) {
> > -    return EFI_DEVICE_ERROR;
> > +    ASSERT_EFI_ERROR (Status);
> > +    return Status;
> >    }
> >
> >    Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
> > -  ASSERT_EFI_ERROR (Status);
> >    if (EFI_ERROR (Status)) {
> > -    return EFI_DEVICE_ERROR;
> > +    ASSERT_EFI_ERROR (Status);
> > +    return Status;
> >    }
> >
> >    BytesPerPixel = GetBytesPerPixel (LcdBpp); diff --git
> > a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> > b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> > index
> >
> 386e6140a69b045f77ee7fa60c4587d8bf4e7d54..f432c8d802614e8a0e4dd
> ab3898f
> > 6e0dbf9a1572 100644
> > --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> > +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> > @@ -110,15 +110,15 @@ LcdSetMode (
> >               &VBackPorch,
> >               &VFrontPorch
> >               );
> > -  ASSERT_EFI_ERROR (Status);
> >    if (EFI_ERROR (Status)) {
> > -    return EFI_DEVICE_ERROR;
> > +    ASSERT_EFI_ERROR (Status);
> > +    return Status;
> >    }
> >
> >    Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
> > -  ASSERT_EFI_ERROR (Status);
> >    if (EFI_ERROR (Status)) {
> > -    return EFI_DEVICE_ERROR;
> > +    ASSERT_EFI_ERROR (Status);
> > +    return Status;
> >    }
> >
> >    // Disable the CLCD_LcdEn bit
> > --
> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> >
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts

Posted by Ard Biesheuvel 7 years ago

On 1 December 2017 at 16:33, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
> Responses inline:
>
>> -----Original Message-----
>> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>> Sent: 13 October 2017 08:33
>> To: Evan Lloyd <Evan.Lloyd@arm.com>
>> Cc: edk2-devel@lists.01.org; "ard.biesheuvel@linaro.org"@arm.com;
>> "leif.lindholm@linaro.org"@arm.com;
>> "Matteo.Carlini@arm.com"@arm.com; "nd@arm.com"@arm.com
>> Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add
>> debug asserts
>>
>> On 26 September 2017 at 21:15,  <evan.lloyd@arm.com> wrote:
>> > From: Girish Pathak <girish.pathak@arm.com>
>> >
>> > This change adds some debug assertions e.g to catch NULL pointer
>> > errors missing in PL11Lcd and HdLcd modules.
>> >
>> > This change also improves related error handling code.
>> >
>> > 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>
>> > ---
>> >
>> ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdAr
>> mVExpress.c       | 44 ++++++++++++++++++--
>> >
>> ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL11
>> 1LcdArmVExpress.c | 43 ++++++++++++++++++-
>> >  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
>> |  8 ++--
>> >  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
>> |  8 ++--
>> >  4 files changed, 90 insertions(+), 13 deletions(-)
>> >
>> > diff --git
>> >
>> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
>> ArmVE
>> > xpress.c
>> >
>> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
>> ArmVE
>> > xpress.c index
>> >
>> b9859a56988f7e5be0adbaa49048a683fe586bfe..58dd9f0c77e1bc9af559a
>> 71d0c7c
>> > ce72d71c6da5 100644
>> > ---
>> >
>> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
>> ArmVE
>> > xpress.c
>> > +++
>> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
>> A
>> > +++ rmVExpress.c
>> > @@ -140,6 +140,7 @@ LcdPlatformInitializeDisplay (
>> >    *                                 buffer in bytes
>> >    *
>> >    * @retval EFI_SUCCESS             Frame buffer memory allocation success.
>> > +  * @retval EFI_INVALID_PARAMETER   VramBaseAddress or VramSize
>> are NULL.
>> >    * @retval !(EFI_SUCCESS)          Other errors.
>> >  **/
>> >  EFI_STATUS
>> > @@ -151,6 +152,13 @@ LcdPlatformGetVram (
>> >    EFI_STATUS              Status;
>> >    EFI_ALLOCATE_TYPE       AllocationType;
>> >
>> > +  // Check VramBaseAddress and VramSize are not NULL.
>> > +  if (VramBaseAddress == NULL || VramSize == NULL) {
>> > +    ASSERT (VramBaseAddress != NULL);
>> > +    ASSERT (VramSize != NULL);
>> > +    return EFI_INVALID_PARAMETER;
>> > +  }
>> > +
>> >    // Set the vram size
>> >    *VramSize = LCD_VRAM_SIZE;
>> >
>> > @@ -169,6 +177,7 @@ LcdPlatformGetVram (
>> >                    VramBaseAddress
>> >                    );
>> >    if (EFI_ERROR (Status)) {
>> > +    ASSERT_EFI_ERROR (Status);
>> >      return Status;
>> >    }
>> >
>> > @@ -179,8 +188,8 @@ LcdPlatformGetVram (
>> >                    *VramSize,
>> >                    EFI_MEMORY_WC
>> >                    );
>> > -  ASSERT_EFI_ERROR (Status);
>> >    if (EFI_ERROR (Status)) {
>> > +    ASSERT_EFI_ERROR (Status);
>>
>> What is the point of this change?
> [[Evan Lloyd]] It is a minor efficiency improvement.  Since the ASSERT can only fire when the if condition is true, it removes a duplicated test from the main (non-error) code flow.  This is irrelevant on hardware, but actually significant when running debug builds on an emulator environment.
>

Fair enough. But I'd prefer to finally fix the DEBUG vs NOOPT build
targets for ARM/AARCH64. DEBUG was never intended to be -O0 (and it is
not on X86 either).

Code is complex enough as it is, and how to move trivial tests like
this around for 'performance' reasons is a dimension I would like to
avoid.

>>
>> >      gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize));
>> >      return Status;
>> >    }
>> > @@ -215,6 +224,7 @@ LcdPlatformSetMode (
>> >    EFI_STATUS            Status;
>> >
>> >    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
>> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
>> >      return EFI_INVALID_PARAMETER;
>> >    }
>> >
>> > @@ -264,6 +274,7 @@ LcdPlatformSetMode (
>> >    *
>> >    * @retval EFI_SUCCESS             Success if the requested mode is found.
>> >    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
>> > +  * @retval EFI_INVALID_PARAMETER   Info is NULL.
>> >  **/
>> >  EFI_STATUS
>> >  LcdPlatformQueryMode (
>> > @@ -271,7 +282,9 @@ LcdPlatformQueryMode (
>> >    OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST  Info
>> >    )
>> >  {
>> > -  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
>> > +  if (ModeNumber >= LcdPlatformGetMaxMode () || Info == NULL) {
>> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
>>
>> Please don't put anything that may have a side effect inside ASSERT (), since
>> they are dropped from RELEASE builds. You can just use ASSERT (FALSE)
>> here, or use a temp variable.
>
>  [[Evan Lloyd]] Agreed
>
>>
>>
>> > +    ASSERT (Info != NULL);
>> >      return EFI_INVALID_PARAMETER;
>> >    }
>> >
>> > @@ -334,6 +347,28 @@ LcdPlatformGetTimings (
>> >    )
>> >  {
>> >    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
>> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
>>
>> same here
> [[Evan Lloyd]] Agreed
>>
>> > +    return EFI_INVALID_PARAMETER;
>> > +  }
>> > +
>> > +  if (HRes == NULL
>> > +    || HSync == NULL
>> > +    || HBackPorch == NULL
>> > +    || HFrontPorch == NULL
>> > +    || VRes == NULL
>> > +    || VSync == NULL
>> > +    || VBackPorch == NULL
>> > +    || VFrontPorch == NULL)
>> > +  {
>> > +    // 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);
>> >      return EFI_INVALID_PARAMETER;
>> >    }
>> >
>> > @@ -356,6 +391,7 @@ LcdPlatformGetTimings (
>> >    *
>> >    * @retval EFI_SUCCESS             The requested mode is found.
>> >    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
>> > +  * @retval EFI_INVALID_PARAMETER   Bpp is NULL.
>> >  **/
>> >  EFI_STATUS
>> >  LcdPlatformGetBpp (
>> > @@ -363,7 +399,9 @@ LcdPlatformGetBpp (
>> >    OUT LCD_BPP * CONST                    Bpp
>> >    )
>> >  {
>> > -  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
>> > +  if (ModeNumber >= LcdPlatformGetMaxMode () || Bpp == NULL) {
>> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
>> > +    ASSERT (Bpp != NULL);
>> >      return EFI_INVALID_PARAMETER;
>> >    }
>> >
>> > diff --git
>> >
>> a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
>> 11Lc
>> > dArmVExpress.c
>> >
>> b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
>> 11Lc
>> > dArmVExpress.c index
>> >
>> 6ae13f06d8b396ea1c67f0bcd735a9d70f476400..5a4abd4c6f9e84d3d690a
>> f7233c1
>> > cebfe1ad339b 100644
>> > ---
>> >
>> a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
>> 11Lc
>> > dArmVExpress.c
>> > +++
>> b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
>> > +++ 11LcdArmVExpress.c
>> > @@ -191,6 +191,7 @@ LcdPlatformInitializeDisplay (
>> >    *                                 buffer in bytes
>> >    *
>> >    * @retval EFI_SUCCESS             Frame buffer memory allocation success.
>> > +  * @retval EFI_INVALID_PARAMETER   VramBaseAddress or VramSize is
>> NULL.
>> >    * @retval !(EFI_SUCCESS)          Other errors.
>> >  **/
>> >  EFI_STATUS
>> > @@ -203,6 +204,13 @@ LcdPlatformGetVram (
>> >
>> >    Status = EFI_SUCCESS;
>> >
>> > +  // Check VramBaseAddress and VramSize are not NULL.
>> > +  if (VramBaseAddress == NULL || VramSize == NULL) {
>> > +    ASSERT (VramBaseAddress != NULL);
>> > +    ASSERT (VramSize != NULL);
>> > +    return EFI_INVALID_PARAMETER;
>> > +  }
>> > +
>> >    // Is it on the motherboard or on the daughterboard?
>> >    switch (PL111_CLCD_SITE) {
>> >
>> > @@ -223,6 +231,7 @@ LcdPlatformGetVram (
>> >                      VramBaseAddress
>> >                      );
>> >      if (EFI_ERROR (Status)) {
>> > +      ASSERT_EFI_ERROR (Status);
>> >        return Status;
>> >      }
>> >
>> > @@ -233,8 +242,8 @@ LcdPlatformGetVram (
>> >                      *VramSize,
>> >                      EFI_MEMORY_WC
>> >                      );
>> > -    ASSERT_EFI_ERROR (Status);
>> >      if (EFI_ERROR (Status)) {
>> > +      ASSERT_EFI_ERROR (Status);
>>
>> Please drop this change
>
>  [[Evan Lloyd]] As stated above, this is not inconsequential for some use cases.  The ASSERT can only fire when the if is satisfied, so moving the ASSERT reduces the number of tests executed in normal execution (depending on optimisation level).
>

As stated above, let's fix DEBUG vs NOOPT instead.

>>
>> >        gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES
>> (*VramSize));
>> >        return Status;
>> >      }
>> > @@ -293,6 +302,7 @@ LcdPlatformSetMode (
>> >    UINT32                SysId;
>> >
>> >    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
>> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
>>
>> No function calls inside ASSERT () please
> [[Evan Lloyd]] Agreed
>
>>
>> >      return EFI_INVALID_PARAMETER;
>> >    }
>> >
>> > @@ -359,6 +369,7 @@ LcdPlatformSetMode (
>> >    *                                 (on success).
>> >    *
>> >    * @retval EFI_SUCCESS             Success if the requested mode is found.
>> > +  * @retval EFI_INVALID_PARAMETER   Info is NULL.
>> >    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
>> >  **/
>> >  EFI_STATUS
>> > @@ -367,7 +378,9 @@ LcdPlatformQueryMode (
>> >    OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST Info
>> >    )
>> >  {
>> > -  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
>> > +  if (ModeNumber >= LcdPlatformGetMaxMode () || Info == NULL) {
>> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
>> > +    ASSERT (Info != NULL);
>> >      return EFI_INVALID_PARAMETER;
>> >    }
>> >
>> > @@ -415,6 +428,7 @@ LcdPlatformQueryMode (
>> >    *
>> >    * @retval EFI_SUCCESS             Success if the requested mode is found.
>> >    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
>> > +  * @retval EFI_INVALID_PARAMETER   One of the OUT parameters is
>> NULL.
>> >  **/
>> >  EFI_STATUS
>> >  LcdPlatformGetTimings (
>> > @@ -430,6 +444,28 @@ LcdPlatformGetTimings (
>> >    )
>> >  {
>> >    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
>> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
>> > +    return EFI_INVALID_PARAMETER;
>> > +  }
>> > +
>> > +  if (HRes == NULL
>> > +    || HSync == NULL
>> > +    || HBackPorch == NULL
>> > +    || HFrontPorch == NULL
>> > +    || VRes == NULL
>> > +    || VSync == NULL
>> > +    || VBackPorch == NULL
>> > +    || VFrontPorch == NULL)
>> > +  {
>> > +    // 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);
>> >      return EFI_INVALID_PARAMETER;
>> >    }
>> >
>> > @@ -452,6 +488,7 @@ LcdPlatformGetTimings (
>> >    *
>> >    * @retval EFI_SUCCESS             The requested mode is found.
>> >    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
>> > +  * @retval EFI_INVALID_PARAMETER   Bpp is NULL.
>> >  **/
>> >  EFI_STATUS
>> >  LcdPlatformGetBpp (
>> > @@ -460,6 +497,8 @@ LcdPlatformGetBpp (
>> >    )
>> >  {
>> >    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
>> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
>> > +    ASSERT (Bpp != NULL);
>> >      return EFI_INVALID_PARAMETER;
>> >    }
>> >
>> > diff --git a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
>> > b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
>> > index
>> >
>> 5f950579720fb69e0a481f697a5cc4038158b409..a266671a26f01d31e8dd
>> b0cf7cbf
>> > e59d2f4dc49c 100644
>> > --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
>> > +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
>> > @@ -109,15 +109,15 @@ LcdSetMode (
>> >               &VBackPorch,
>> >               &VFrontPorch
>> >               );
>> > -  ASSERT_EFI_ERROR (Status);
>> >    if (EFI_ERROR (Status)) {
>> > -    return EFI_DEVICE_ERROR;
>> > +    ASSERT_EFI_ERROR (Status);
>> > +    return Status;
>> >    }
>> >
>> >    Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
>> > -  ASSERT_EFI_ERROR (Status);
>> >    if (EFI_ERROR (Status)) {
>> > -    return EFI_DEVICE_ERROR;
>> > +    ASSERT_EFI_ERROR (Status);
>> > +    return Status;
>> >    }
>> >
>> >    BytesPerPixel = GetBytesPerPixel (LcdBpp); diff --git
>> > a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
>> > b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
>> > index
>> >
>> 386e6140a69b045f77ee7fa60c4587d8bf4e7d54..f432c8d802614e8a0e4dd
>> ab3898f
>> > 6e0dbf9a1572 100644
>> > --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
>> > +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
>> > @@ -110,15 +110,15 @@ LcdSetMode (
>> >               &VBackPorch,
>> >               &VFrontPorch
>> >               );
>> > -  ASSERT_EFI_ERROR (Status);
>> >    if (EFI_ERROR (Status)) {
>> > -    return EFI_DEVICE_ERROR;
>> > +    ASSERT_EFI_ERROR (Status);
>> > +    return Status;
>> >    }
>> >
>> >    Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
>> > -  ASSERT_EFI_ERROR (Status);
>> >    if (EFI_ERROR (Status)) {
>> > -    return EFI_DEVICE_ERROR;
>> > +    ASSERT_EFI_ERROR (Status);
>> > +    return Status;
>> >    }
>> >
>> >    // Disable the CLCD_LcdEn bit
>> > --
>> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>> >
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts

Posted by Leif Lindholm 7 years ago

On Fri, Dec 01, 2017 at 05:34:52PM +0000, Ard Biesheuvel wrote:
> On 1 December 2017 at 16:33, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
> > Responses inline:
> >
> >> -----Original Message-----
> >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> >> Sent: 13 October 2017 08:33
> >> To: Evan Lloyd <Evan.Lloyd@arm.com>
> >> Cc: edk2-devel@lists.01.org; "ard.biesheuvel@linaro.org"@arm.com;
> >> "leif.lindholm@linaro.org"@arm.com;
> >> "Matteo.Carlini@arm.com"@arm.com; "nd@arm.com"@arm.com
> >> Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add
> >> debug asserts
> >>
> >> On 26 September 2017 at 21:15,  <evan.lloyd@arm.com> wrote:
> >> > From: Girish Pathak <girish.pathak@arm.com>
> >> >
> >> > This change adds some debug assertions e.g to catch NULL pointer
> >> > errors missing in PL11Lcd and HdLcd modules.
> >> >
> >> > This change also improves related error handling code.
> >> >
> >> > 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>
> >> > ---
> >> >
> >> ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdAr
> >> mVExpress.c       | 44 ++++++++++++++++++--
> >> >
> >> ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL11
> >> 1LcdArmVExpress.c | 43 ++++++++++++++++++-
> >> >  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> >> |  8 ++--
> >> >  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> >> |  8 ++--
> >> >  4 files changed, 90 insertions(+), 13 deletions(-)
> >> >
> >> > diff --git
> >> >
> >> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> ArmVE
> >> > xpress.c
> >> >
> >> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> ArmVE
> >> > xpress.c index
> >> >
> >> b9859a56988f7e5be0adbaa49048a683fe586bfe..58dd9f0c77e1bc9af559a
> >> 71d0c7c
> >> > ce72d71c6da5 100644
> >> > ---
> >> >
> >> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> ArmVE
> >> > xpress.c
> >> > +++
> >> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> A
> >> > +++ rmVExpress.c
> >> > @@ -140,6 +140,7 @@ LcdPlatformInitializeDisplay (
> >> >    *                                 buffer in bytes
> >> >    *
> >> >    * @retval EFI_SUCCESS             Frame buffer memory allocation success.
> >> > +  * @retval EFI_INVALID_PARAMETER   VramBaseAddress or VramSize
> >> are NULL.
> >> >    * @retval !(EFI_SUCCESS)          Other errors.
> >> >  **/
> >> >  EFI_STATUS
> >> > @@ -151,6 +152,13 @@ LcdPlatformGetVram (
> >> >    EFI_STATUS              Status;
> >> >    EFI_ALLOCATE_TYPE       AllocationType;
> >> >
> >> > +  // Check VramBaseAddress and VramSize are not NULL.
> >> > +  if (VramBaseAddress == NULL || VramSize == NULL) {
> >> > +    ASSERT (VramBaseAddress != NULL);
> >> > +    ASSERT (VramSize != NULL);
> >> > +    return EFI_INVALID_PARAMETER;
> >> > +  }
> >> > +
> >> >    // Set the vram size
> >> >    *VramSize = LCD_VRAM_SIZE;
> >> >
> >> > @@ -169,6 +177,7 @@ LcdPlatformGetVram (
> >> >                    VramBaseAddress
> >> >                    );
> >> >    if (EFI_ERROR (Status)) {
> >> > +    ASSERT_EFI_ERROR (Status);
> >> >      return Status;
> >> >    }
> >> >
> >> > @@ -179,8 +188,8 @@ LcdPlatformGetVram (
> >> >                    *VramSize,
> >> >                    EFI_MEMORY_WC
> >> >                    );
> >> > -  ASSERT_EFI_ERROR (Status);
> >> >    if (EFI_ERROR (Status)) {
> >> > +    ASSERT_EFI_ERROR (Status);
> >>
> >> What is the point of this change?
> > [[Evan Lloyd]] It is a minor efficiency improvement.  Since the
> > ASSERT can only fire when the if condition is true, it removes a
> > duplicated test from the main (non-error) code flow.  This is
> > irrelevant on hardware, but actually significant when running
> > debug builds on an emulator environment.
> 
> Fair enough. But I'd prefer to finally fix the DEBUG vs NOOPT build
> targets for ARM/AARCH64. DEBUG was never intended to be -O0 (and it is
> not on X86 either).
> 
> Code is complex enough as it is, and how to move trivial tests like
> this around for 'performance' reasons is a dimension I would like to
> avoid.

Echoing from an irc discussion with Ard:
I think NOOPT (and a !-O0 DEBUG) would be good to have for the ARM
architectures as well, both for GCC and CLANG.

Evan, have you done any investigation into the impact of optimization
on emulation performance?

My completely unfounded guess would be -O1 should be a good fit for
that environment, and hence for DEBUG. It would remove some of the
more insane bloat of -O0 without starting heavy inlining.
But as always, real numbers beat guesses.

(If -O2 is better than -O1, is -Os better than either?)

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

Re: [edk2] [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts

Posted by Evan Lloyd 7 years ago


> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: 01 December 2017 17:35
> To: Evan Lloyd <Evan.Lloyd@arm.com>
> Cc: edk2-devel@lists.01.org; ard.biesheuvel@linaro.org@arm.com
> <"ard.biesheuvel@linaro.org"@arm.com>;
> leif.lindholm@linaro.org@arm.com <"leif.lindholm@linaro.org"@arm.com>;
> Matteo.Carlini@arm.com@arm.com
> <"Matteo.Carlini@arm.com"@arm.com>; nd@arm.com@arm.com
> <"nd@arm.com"@arm.com>
> Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add
> debug asserts
>
> On 1 December 2017 at 16:33, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
> > Responses inline:
> >
> >> -----Original Message-----
> >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> >> Sent: 13 October 2017 08:33
> >> To: Evan Lloyd <Evan.Lloyd@arm.com>
> >> Cc: edk2-devel@lists.01.org; "ard.biesheuvel@linaro.org"@arm.com;
> >> "leif.lindholm@linaro.org"@arm.com;
> >> "Matteo.Carlini@arm.com"@arm.com; "nd@arm.com"@arm.com
> >> Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe:
> Add
> >> debug asserts
> >>
> >> On 26 September 2017 at 21:15,  <evan.lloyd@arm.com> wrote:
> >> > From: Girish Pathak <girish.pathak@arm.com>
> >> >
> >> > This change adds some debug assertions e.g to catch NULL pointer
> >> > errors missing in PL11Lcd and HdLcd modules.
> >> >
> >> > This change also improves related error handling code.
> >> >
> >> > 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>
> >> > ---
> >> >
> >>
> ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdAr
> >> mVExpress.c       | 44 ++++++++++++++++++--
> >> >
> >>
> ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL11
> >> 1LcdArmVExpress.c | 43 ++++++++++++++++++-
> >> >  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> >> |  8 ++--
> >> >  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> >> |  8 ++--
> >> >  4 files changed, 90 insertions(+), 13 deletions(-)
> >> >
> >> > diff --git
> >> >
> >>
> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> ArmVE
> >> > xpress.c
> >> >
> >>
> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> ArmVE
> >> > xpress.c index
> >> >
> >>
> b9859a56988f7e5be0adbaa49048a683fe586bfe..58dd9f0c77e1bc9af559a
> >> 71d0c7c
> >> > ce72d71c6da5 100644
> >> > ---
> >> >
> >>
> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> ArmVE
> >> > xpress.c
> >> > +++
> >>
> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> A
> >> > +++ rmVExpress.c
> >> > @@ -140,6 +140,7 @@ LcdPlatformInitializeDisplay (
> >> >    *                                 buffer in bytes
> >> >    *
> >> >    * @retval EFI_SUCCESS             Frame buffer memory allocation
> success.
> >> > +  * @retval EFI_INVALID_PARAMETER   VramBaseAddress or
> VramSize
> >> are NULL.
> >> >    * @retval !(EFI_SUCCESS)          Other errors.
> >> >  **/
> >> >  EFI_STATUS
> >> > @@ -151,6 +152,13 @@ LcdPlatformGetVram (
> >> >    EFI_STATUS              Status;
> >> >    EFI_ALLOCATE_TYPE       AllocationType;
> >> >
> >> > +  // Check VramBaseAddress and VramSize are not NULL.
> >> > +  if (VramBaseAddress == NULL || VramSize == NULL) {
> >> > +    ASSERT (VramBaseAddress != NULL);
> >> > +    ASSERT (VramSize != NULL);
> >> > +    return EFI_INVALID_PARAMETER;
> >> > +  }
> >> > +
> >> >    // Set the vram size
> >> >    *VramSize = LCD_VRAM_SIZE;
> >> >
> >> > @@ -169,6 +177,7 @@ LcdPlatformGetVram (
> >> >                    VramBaseAddress
> >> >                    );
> >> >    if (EFI_ERROR (Status)) {
> >> > +    ASSERT_EFI_ERROR (Status);
> >> >      return Status;
> >> >    }
> >> >
> >> > @@ -179,8 +188,8 @@ LcdPlatformGetVram (
> >> >                    *VramSize,
> >> >                    EFI_MEMORY_WC
> >> >                    );
> >> > -  ASSERT_EFI_ERROR (Status);
> >> >    if (EFI_ERROR (Status)) {
> >> > +    ASSERT_EFI_ERROR (Status);
> >>
> >> What is the point of this change?
> > [[Evan Lloyd]] It is a minor efficiency improvement.  Since the ASSERT can
> only fire when the if condition is true, it removes a duplicated test from the
> main (non-error) code flow.  This is irrelevant on hardware, but actually
> significant when running debug builds on an emulator environment.
> >
>
> Fair enough. But I'd prefer to finally fix the DEBUG vs NOOPT build targets
> for ARM/AARCH64. DEBUG was never intended to be -O0 (and it is not on
> X86 either).
>
> Code is complex enough as it is, and how to move trivial tests like this
> around for 'performance' reasons is a dimension I would like to avoid.

 [[Evan Lloyd]] I have no objection to the DEBUG build being updated from -O0.
I strongly suspect that it was set to that because it reduced the incidence of the r18 problem that has now been fixed.
However, I can certainly envisage circumstances when we might need to do an emulator run with -O0 used for the build - so the reordering would still be beneficial.

>
> >>
> >> >      gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES
> (*VramSize));
> >> >      return Status;
> >> >    }
> >> > @@ -215,6 +224,7 @@ LcdPlatformSetMode (
> >> >    EFI_STATUS            Status;
> >> >
> >> >    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> >> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> >> >      return EFI_INVALID_PARAMETER;
> >> >    }
> >> >
> >> > @@ -264,6 +274,7 @@ LcdPlatformSetMode (
> >> >    *
> >> >    * @retval EFI_SUCCESS             Success if the requested mode is
> found.
> >> >    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> >> > +  * @retval EFI_INVALID_PARAMETER   Info is NULL.
> >> >  **/
> >> >  EFI_STATUS
> >> >  LcdPlatformQueryMode (
> >> > @@ -271,7 +282,9 @@ LcdPlatformQueryMode (
> >> >    OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST  Info
> >> >    )
> >> >  {
> >> > -  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> >> > +  if (ModeNumber >= LcdPlatformGetMaxMode () || Info == NULL) {
> >> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> >>
> >> Please don't put anything that may have a side effect inside ASSERT
> >> (), since they are dropped from RELEASE builds. You can just use
> >> ASSERT (FALSE) here, or use a temp variable.
> >
> >  [[Evan Lloyd]] Agreed
> >
> >>
> >>
> >> > +    ASSERT (Info != NULL);
> >> >      return EFI_INVALID_PARAMETER;
> >> >    }
> >> >
> >> > @@ -334,6 +347,28 @@ LcdPlatformGetTimings (
> >> >    )
> >> >  {
> >> >    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> >> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> >>
> >> same here
> > [[Evan Lloyd]] Agreed
> >>
> >> > +    return EFI_INVALID_PARAMETER;
> >> > +  }
> >> > +
> >> > +  if (HRes == NULL
> >> > +    || HSync == NULL
> >> > +    || HBackPorch == NULL
> >> > +    || HFrontPorch == NULL
> >> > +    || VRes == NULL
> >> > +    || VSync == NULL
> >> > +    || VBackPorch == NULL
> >> > +    || VFrontPorch == NULL)
> >> > +  {
> >> > +    // 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);
> >> >      return EFI_INVALID_PARAMETER;
> >> >    }
> >> >
> >> > @@ -356,6 +391,7 @@ LcdPlatformGetTimings (
> >> >    *
> >> >    * @retval EFI_SUCCESS             The requested mode is found.
> >> >    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> >> > +  * @retval EFI_INVALID_PARAMETER   Bpp is NULL.
> >> >  **/
> >> >  EFI_STATUS
> >> >  LcdPlatformGetBpp (
> >> > @@ -363,7 +399,9 @@ LcdPlatformGetBpp (
> >> >    OUT LCD_BPP * CONST                    Bpp
> >> >    )
> >> >  {
> >> > -  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> >> > +  if (ModeNumber >= LcdPlatformGetMaxMode () || Bpp == NULL) {
> >> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> >> > +    ASSERT (Bpp != NULL);
> >> >      return EFI_INVALID_PARAMETER;
> >> >    }
> >> >
> >> > diff --git
> >> >
> >>
> a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
> >> 11Lc
> >> > dArmVExpress.c
> >> >
> >>
> b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
> >> 11Lc
> >> > dArmVExpress.c index
> >> >
> >>
> 6ae13f06d8b396ea1c67f0bcd735a9d70f476400..5a4abd4c6f9e84d3d690a
> >> f7233c1
> >> > cebfe1ad339b 100644
> >> > ---
> >> >
> >>
> a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
> >> 11Lc
> >> > dArmVExpress.c
> >> > +++
> >>
> b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
> >> > +++ 11LcdArmVExpress.c
> >> > @@ -191,6 +191,7 @@ LcdPlatformInitializeDisplay (
> >> >    *                                 buffer in bytes
> >> >    *
> >> >    * @retval EFI_SUCCESS             Frame buffer memory allocation
> success.
> >> > +  * @retval EFI_INVALID_PARAMETER   VramBaseAddress or
> VramSize is
> >> NULL.
> >> >    * @retval !(EFI_SUCCESS)          Other errors.
> >> >  **/
> >> >  EFI_STATUS
> >> > @@ -203,6 +204,13 @@ LcdPlatformGetVram (
> >> >
> >> >    Status = EFI_SUCCESS;
> >> >
> >> > +  // Check VramBaseAddress and VramSize are not NULL.
> >> > +  if (VramBaseAddress == NULL || VramSize == NULL) {
> >> > +    ASSERT (VramBaseAddress != NULL);
> >> > +    ASSERT (VramSize != NULL);
> >> > +    return EFI_INVALID_PARAMETER;
> >> > +  }
> >> > +
> >> >    // Is it on the motherboard or on the daughterboard?
> >> >    switch (PL111_CLCD_SITE) {
> >> >
> >> > @@ -223,6 +231,7 @@ LcdPlatformGetVram (
> >> >                      VramBaseAddress
> >> >                      );
> >> >      if (EFI_ERROR (Status)) {
> >> > +      ASSERT_EFI_ERROR (Status);
> >> >        return Status;
> >> >      }
> >> >
> >> > @@ -233,8 +242,8 @@ LcdPlatformGetVram (
> >> >                      *VramSize,
> >> >                      EFI_MEMORY_WC
> >> >                      );
> >> > -    ASSERT_EFI_ERROR (Status);
> >> >      if (EFI_ERROR (Status)) {
> >> > +      ASSERT_EFI_ERROR (Status);
> >>
> >> Please drop this change
> >
> >  [[Evan Lloyd]] As stated above, this is not inconsequential for some use
> cases.  The ASSERT can only fire when the if is satisfied, so moving the
> ASSERT reduces the number of tests executed in normal execution
> (depending on optimisation level).
> >
>
> As stated above, let's fix DEBUG vs NOOPT instead.
>
> >>
> >> >        gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES
> >> (*VramSize));
> >> >        return Status;
> >> >      }
> >> > @@ -293,6 +302,7 @@ LcdPlatformSetMode (
> >> >    UINT32                SysId;
> >> >
> >> >    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> >> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> >>
> >> No function calls inside ASSERT () please
> > [[Evan Lloyd]] Agreed
> >
> >>
> >> >      return EFI_INVALID_PARAMETER;
> >> >    }
> >> >
> >> > @@ -359,6 +369,7 @@ LcdPlatformSetMode (
> >> >    *                                 (on success).
> >> >    *
> >> >    * @retval EFI_SUCCESS             Success if the requested mode is
> found.
> >> > +  * @retval EFI_INVALID_PARAMETER   Info is NULL.
> >> >    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> >> >  **/
> >> >  EFI_STATUS
> >> > @@ -367,7 +378,9 @@ LcdPlatformQueryMode (
> >> >    OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST Info
> >> >    )
> >> >  {
> >> > -  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> >> > +  if (ModeNumber >= LcdPlatformGetMaxMode () || Info == NULL) {
> >> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> >> > +    ASSERT (Info != NULL);
> >> >      return EFI_INVALID_PARAMETER;
> >> >    }
> >> >
> >> > @@ -415,6 +428,7 @@ LcdPlatformQueryMode (
> >> >    *
> >> >    * @retval EFI_SUCCESS             Success if the requested mode is
> found.
> >> >    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> >> > +  * @retval EFI_INVALID_PARAMETER   One of the OUT parameters is
> >> NULL.
> >> >  **/
> >> >  EFI_STATUS
> >> >  LcdPlatformGetTimings (
> >> > @@ -430,6 +444,28 @@ LcdPlatformGetTimings (
> >> >    )
> >> >  {
> >> >    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> >> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> >> > +    return EFI_INVALID_PARAMETER;
> >> > +  }
> >> > +
> >> > +  if (HRes == NULL
> >> > +    || HSync == NULL
> >> > +    || HBackPorch == NULL
> >> > +    || HFrontPorch == NULL
> >> > +    || VRes == NULL
> >> > +    || VSync == NULL
> >> > +    || VBackPorch == NULL
> >> > +    || VFrontPorch == NULL)
> >> > +  {
> >> > +    // 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);
> >> >      return EFI_INVALID_PARAMETER;
> >> >    }
> >> >
> >> > @@ -452,6 +488,7 @@ LcdPlatformGetTimings (
> >> >    *
> >> >    * @retval EFI_SUCCESS             The requested mode is found.
> >> >    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> >> > +  * @retval EFI_INVALID_PARAMETER   Bpp is NULL.
> >> >  **/
> >> >  EFI_STATUS
> >> >  LcdPlatformGetBpp (
> >> > @@ -460,6 +497,8 @@ LcdPlatformGetBpp (
> >> >    )
> >> >  {
> >> >    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> >> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> >> > +    ASSERT (Bpp != NULL);
> >> >      return EFI_INVALID_PARAMETER;
> >> >    }
> >> >
> >> > diff --git a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> >> > b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> >> > index
> >> >
> >>
> 5f950579720fb69e0a481f697a5cc4038158b409..a266671a26f01d31e8dd
> >> b0cf7cbf
> >> > e59d2f4dc49c 100644
> >> > --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> >> > +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> >> > @@ -109,15 +109,15 @@ LcdSetMode (
> >> >               &VBackPorch,
> >> >               &VFrontPorch
> >> >               );
> >> > -  ASSERT_EFI_ERROR (Status);
> >> >    if (EFI_ERROR (Status)) {
> >> > -    return EFI_DEVICE_ERROR;
> >> > +    ASSERT_EFI_ERROR (Status);
> >> > +    return Status;
> >> >    }
> >> >
> >> >    Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
> >> > -  ASSERT_EFI_ERROR (Status);
> >> >    if (EFI_ERROR (Status)) {
> >> > -    return EFI_DEVICE_ERROR;
> >> > +    ASSERT_EFI_ERROR (Status);
> >> > +    return Status;
> >> >    }
> >> >
> >> >    BytesPerPixel = GetBytesPerPixel (LcdBpp); diff --git
> >> > a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> >> > b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> >> > index
> >> >
> >>
> 386e6140a69b045f77ee7fa60c4587d8bf4e7d54..f432c8d802614e8a0e4dd
> >> ab3898f
> >> > 6e0dbf9a1572 100644
> >> > --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> >> > +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> >> > @@ -110,15 +110,15 @@ LcdSetMode (
> >> >               &VBackPorch,
> >> >               &VFrontPorch
> >> >               );
> >> > -  ASSERT_EFI_ERROR (Status);
> >> >    if (EFI_ERROR (Status)) {
> >> > -    return EFI_DEVICE_ERROR;
> >> > +    ASSERT_EFI_ERROR (Status);
> >> > +    return Status;
> >> >    }
> >> >
> >> >    Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
> >> > -  ASSERT_EFI_ERROR (Status);
> >> >    if (EFI_ERROR (Status)) {
> >> > -    return EFI_DEVICE_ERROR;
> >> > +    ASSERT_EFI_ERROR (Status);
> >> > +    return Status;
> >> >    }
> >> >
> >> >    // Disable the CLCD_LcdEn bit
> >> > --
> >> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> >> >
> > IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium. Thank you.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts

Posted by Alexei Fedorov 7 years ago

Hi,

I've compiled current HdLcd.c code with different optimisation levels for DEBUG build using GCC 7.2.1, and the assembler code below was generated for:

  ASSERT_EFI_ERROR (Status);
  if (EFI_ERROR( Status )) {
    return EFI_DEVICE_ERROR;
  }


  1.  -O0 (default DEBUG option for AARCH64 before Ard's patch):


    str    x0, [x29, 72]    //, Status

// r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79:   ASSERT_EFI_ERROR (Status);
    .loc 1 79 0
    bl    DebugAssertEnabled    //
    and    w0, w0, 255    // _1, tmp150
    cmp    w0, 0    // _1,
    beq    .L4    //,
    ldr    x0, [x29, 72]    // Status.9_2, Status
    cmp    x0, 0    // Status.9_2,
    bge    .L4    //,

2.  -Os:

     mov    x19, x0    // Status,
// r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79:   ASSERT_EFI_ERROR (Status);
    .loc 1 79 0
    bl    DebugAssertEnabled    //
.LVL15:
    tst    w0, 255    //
    beq    .L4    //,
    tbz    x19, #63, .L8    // Status,

3.  -O3:

    mov    x19, x0    // Status,
.LVL14:
// r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79:   ASSERT_EFI_ERROR (Status);
    bl    DebugAssertEnabled    //
.LVL15:
    tst    w0, 255    //
    beq    .L5    //,
    tbnz    x19, #63, .L26    // Status,

with DebugAssertEnabled() compiled as:

DebugAssertEnabled:
.LFB4:
    .loc 1 203 0
    .cfi_startproc
// r:\edk2\MdePkg\Library\BaseDebugLibSerialPort\DebugLib.c:204:   return (BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) & DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED) != 0);
    .loc 1 204 0
    adrp    x0, _gPcd_FixedAtBuild_PcdDebugPropertyMask    // tmp80,
    add    x0, x0, :lo12:_gPcd_FixedAtBuild_PcdDebugPropertyMask    // tmp79, tmp80,
    ldrb    w0, [x0]    // _gPcd_FixedAtBuild_PcdDebugPropertyMask.4_1, _gPcd_FixedAtBuild_PcdDebugPropertyMask
    and    w0, w0, 1    // _3, _2,
    cmp    w0, 0    // _3,
    cset    w0, ne    // tmp82,
    and    w0, w0, 255    // _4, tmp81
// r:\edk2\MdePkg\Library\BaseDebugLibSerialPort\DebugLib.c:205: }
    .loc 1 205 0
    ret

As you can see each "ASSERT_EFI_ERROR (Status)" macro requires DebugAssertEnabled() call taking 8 instructions by itself + minimum 3 instructions (for -Os, -O3) for Status check, which will be performed by "if (EFI_ERROR( Status ))" anyway.
Please correct me if I'm wrong assuming that placing
ASSERT_EFI_ERROR (Status)
inside
if (EFI_ERROR( Status )) {
statement is an optimisation improvement.

Thank you for your cooperation.

Alexei.



> >> > -  ASSERT_EFI_ERROR (Status);
> >> >    if (EFI_ERROR (Status)) {
> >> > +    ASSERT_EFI_ERROR (Status);
> >>
> >> What is the point of this change?
> > [[Evan Lloyd]] It is a minor efficiency improvement.  Since the ASSERT can
> only fire when the if condition is true, it removes a duplicated test from the
> main (non-error) code flow.  This is irrelevant on hardware, but actually
> significant when running debug builds on an emulator environment.
> >
>
> Fair enough. But I'd prefer to finally fix the DEBUG vs NOOPT build targets
> for ARM/AARCH64. DEBUG was never intended to be -O0 (and it is not on
> X86 either).
>
> Code is complex enough as it is, and how to move trivial tests like this
> around for 'performance' reasons is a dimension I would like to avoid.

  >[[Evan Lloyd]] I have no objection to the DEBUG build being updated from -O0.
>I strongly suspect that it was set to that because it reduced the incidence of the r18 problem that has now been fixed.
>However, I can certainly envisage circumstances when we might need to do an emulator run with -O0 used for the build - so the >

>reordering would still be beneficial.


________________________________
From: edk2-devel <edk2-devel-bounces@lists.01.org> on behalf of Evan Lloyd <Evan.Lloyd@arm.com>
Sent: 05 December 2017 20:46
To: Ard Biesheuvel
Cc: edk2-devel@lists.01.org; leif.lindholm@linaro.org@arm.com; nd@arm.com@arm.com; ard.biesheuvel@linaro.org@arm.com; Matteo.Carlini@arm.com@arm.com
Subject: Re: [edk2] [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts



> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: 01 December 2017 17:35
> To: Evan Lloyd <Evan.Lloyd@arm.com>
> Cc: edk2-devel@lists.01.org; ard.biesheuvel@linaro.org@arm.com
> <"ard.biesheuvel@linaro.org"@arm.com>;
> leif.lindholm@linaro.org@arm.com <"leif.lindholm@linaro.org"@arm.com>;
> Matteo.Carlini@arm.com@arm.com
> <"Matteo.Carlini@arm.com"@arm.com>; nd@arm.com@arm.com
> <"nd@arm.com"@arm.com>
> Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add
> debug asserts
>
> On 1 December 2017 at 16:33, Evan Lloyd <Evan.Lloyd@arm.com> wrote:
> > Responses inline:
> >
> >> -----Original Message-----
> >> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> >> Sent: 13 October 2017 08:33
> >> To: Evan Lloyd <Evan.Lloyd@arm.com>
> >> Cc: edk2-devel@lists.01.org; "ard.biesheuvel@linaro.org"@arm.com;
> >> "leif.lindholm@linaro.org"@arm.com;
> >> "Matteo.Carlini@arm.com"@arm.com; "nd@arm.com"@arm.com
> >> Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe:
> Add
> >> debug asserts
> >>
> >> On 26 September 2017 at 21:15,  <evan.lloyd@arm.com> wrote:
> >> > From: Girish Pathak <girish.pathak@arm.com>
> >> >
> >> > This change adds some debug assertions e.g to catch NULL pointer
> >> > errors missing in PL11Lcd and HdLcd modules.
> >> >
> >> > This change also improves related error handling code.
> >> >
> >> > 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>
> >> > ---
> >> >
> >>
> ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdAr
> >> mVExpress.c       | 44 ++++++++++++++++++--
> >> >
> >>
> ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL11
> >> 1LcdArmVExpress.c | 43 ++++++++++++++++++-
> >> >  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> >> |  8 ++--
> >> >  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> >> |  8 ++--
> >> >  4 files changed, 90 insertions(+), 13 deletions(-)
> >> >
> >> > diff --git
> >> >
> >>
> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> ArmVE
> >> > xpress.c
> >> >
> >>
> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> ArmVE
> >> > xpress.c index
> >> >
> >>
> b9859a56988f7e5be0adbaa49048a683fe586bfe..58dd9f0c77e1bc9af559a
> >> 71d0c7c
> >> > ce72d71c6da5 100644
> >> > ---
> >> >
> >>
> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> ArmVE
> >> > xpress.c
> >> > +++
> >>
> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> A
> >> > +++ rmVExpress.c
> >> > @@ -140,6 +140,7 @@ LcdPlatformInitializeDisplay (
> >> >    *                                 buffer in bytes
> >> >    *
> >> >    * @retval EFI_SUCCESS             Frame buffer memory allocation
> success.
> >> > +  * @retval EFI_INVALID_PARAMETER   VramBaseAddress or
> VramSize
> >> are NULL.
> >> >    * @retval !(EFI_SUCCESS)          Other errors.
> >> >  **/
> >> >  EFI_STATUS
> >> > @@ -151,6 +152,13 @@ LcdPlatformGetVram (
> >> >    EFI_STATUS              Status;
> >> >    EFI_ALLOCATE_TYPE       AllocationType;
> >> >
> >> > +  // Check VramBaseAddress and VramSize are not NULL.
> >> > +  if (VramBaseAddress == NULL || VramSize == NULL) {
> >> > +    ASSERT (VramBaseAddress != NULL);
> >> > +    ASSERT (VramSize != NULL);
> >> > +    return EFI_INVALID_PARAMETER;
> >> > +  }
> >> > +
> >> >    // Set the vram size
> >> >    *VramSize = LCD_VRAM_SIZE;
> >> >
> >> > @@ -169,6 +177,7 @@ LcdPlatformGetVram (
> >> >                    VramBaseAddress
> >> >                    );
> >> >    if (EFI_ERROR (Status)) {
> >> > +    ASSERT_EFI_ERROR (Status);
> >> >      return Status;
> >> >    }
> >> >
> >> > @@ -179,8 +188,8 @@ LcdPlatformGetVram (
> >> >                    *VramSize,
> >> >                    EFI_MEMORY_WC
> >> >                    );
> >> > -  ASSERT_EFI_ERROR (Status);
> >> >    if (EFI_ERROR (Status)) {
> >> > +    ASSERT_EFI_ERROR (Status);
> >>
> >> What is the point of this change?
> > [[Evan Lloyd]] It is a minor efficiency improvement.  Since the ASSERT can
> only fire when the if condition is true, it removes a duplicated test from the
> main (non-error) code flow.  This is irrelevant on hardware, but actually
> significant when running debug builds on an emulator environment.
> >
>
> Fair enough. But I'd prefer to finally fix the DEBUG vs NOOPT build targets
> for ARM/AARCH64. DEBUG was never intended to be -O0 (and it is not on
> X86 either).
>
> Code is complex enough as it is, and how to move trivial tests like this
> around for 'performance' reasons is a dimension I would like to avoid.

 [[Evan Lloyd]] I have no objection to the DEBUG build being updated from -O0.
I strongly suspect that it was set to that because it reduced the incidence of the r18 problem that has now been fixed.
However, I can certainly envisage circumstances when we might need to do an emulator run with -O0 used for the build - so the reordering would still be beneficial.

>
> >>
> >> >      gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES
> (*VramSize));
> >> >      return Status;
> >> >    }
> >> > @@ -215,6 +224,7 @@ LcdPlatformSetMode (
> >> >    EFI_STATUS            Status;
> >> >
> >> >    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> >> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> >> >      return EFI_INVALID_PARAMETER;
> >> >    }
> >> >
> >> > @@ -264,6 +274,7 @@ LcdPlatformSetMode (
> >> >    *
> >> >    * @retval EFI_SUCCESS             Success if the requested mode is
> found.
> >> >    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> >> > +  * @retval EFI_INVALID_PARAMETER   Info is NULL.
> >> >  **/
> >> >  EFI_STATUS
> >> >  LcdPlatformQueryMode (
> >> > @@ -271,7 +282,9 @@ LcdPlatformQueryMode (
> >> >    OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST  Info
> >> >    )
> >> >  {
> >> > -  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> >> > +  if (ModeNumber >= LcdPlatformGetMaxMode () || Info == NULL) {
> >> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> >>
> >> Please don't put anything that may have a side effect inside ASSERT
> >> (), since they are dropped from RELEASE builds. You can just use
> >> ASSERT (FALSE) here, or use a temp variable.
> >
> >  [[Evan Lloyd]] Agreed
> >
> >>
> >>
> >> > +    ASSERT (Info != NULL);
> >> >      return EFI_INVALID_PARAMETER;
> >> >    }
> >> >
> >> > @@ -334,6 +347,28 @@ LcdPlatformGetTimings (
> >> >    )
> >> >  {
> >> >    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> >> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> >>
> >> same here
> > [[Evan Lloyd]] Agreed
> >>
> >> > +    return EFI_INVALID_PARAMETER;
> >> > +  }
> >> > +
> >> > +  if (HRes == NULL
> >> > +    || HSync == NULL
> >> > +    || HBackPorch == NULL
> >> > +    || HFrontPorch == NULL
> >> > +    || VRes == NULL
> >> > +    || VSync == NULL
> >> > +    || VBackPorch == NULL
> >> > +    || VFrontPorch == NULL)
> >> > +  {
> >> > +    // 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);
> >> >      return EFI_INVALID_PARAMETER;
> >> >    }
> >> >
> >> > @@ -356,6 +391,7 @@ LcdPlatformGetTimings (
> >> >    *
> >> >    * @retval EFI_SUCCESS             The requested mode is found.
> >> >    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> >> > +  * @retval EFI_INVALID_PARAMETER   Bpp is NULL.
> >> >  **/
> >> >  EFI_STATUS
> >> >  LcdPlatformGetBpp (
> >> > @@ -363,7 +399,9 @@ LcdPlatformGetBpp (
> >> >    OUT LCD_BPP * CONST                    Bpp
> >> >    )
> >> >  {
> >> > -  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> >> > +  if (ModeNumber >= LcdPlatformGetMaxMode () || Bpp == NULL) {
> >> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> >> > +    ASSERT (Bpp != NULL);
> >> >      return EFI_INVALID_PARAMETER;
> >> >    }
> >> >
> >> > diff --git
> >> >
> >>
> a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
> >> 11Lc
> >> > dArmVExpress.c
> >> >
> >>
> b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
> >> 11Lc
> >> > dArmVExpress.c index
> >> >
> >>
> 6ae13f06d8b396ea1c67f0bcd735a9d70f476400..5a4abd4c6f9e84d3d690a
> >> f7233c1
> >> > cebfe1ad339b 100644
> >> > ---
> >> >
> >>
> a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
> >> 11Lc
> >> > dArmVExpress.c
> >> > +++
> >>
> b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL1
> >> > +++ 11LcdArmVExpress.c
> >> > @@ -191,6 +191,7 @@ LcdPlatformInitializeDisplay (
> >> >    *                                 buffer in bytes
> >> >    *
> >> >    * @retval EFI_SUCCESS             Frame buffer memory allocation
> success.
> >> > +  * @retval EFI_INVALID_PARAMETER   VramBaseAddress or
> VramSize is
> >> NULL.
> >> >    * @retval !(EFI_SUCCESS)          Other errors.
> >> >  **/
> >> >  EFI_STATUS
> >> > @@ -203,6 +204,13 @@ LcdPlatformGetVram (
> >> >
> >> >    Status = EFI_SUCCESS;
> >> >
> >> > +  // Check VramBaseAddress and VramSize are not NULL.
> >> > +  if (VramBaseAddress == NULL || VramSize == NULL) {
> >> > +    ASSERT (VramBaseAddress != NULL);
> >> > +    ASSERT (VramSize != NULL);
> >> > +    return EFI_INVALID_PARAMETER;
> >> > +  }
> >> > +
> >> >    // Is it on the motherboard or on the daughterboard?
> >> >    switch (PL111_CLCD_SITE) {
> >> >
> >> > @@ -223,6 +231,7 @@ LcdPlatformGetVram (
> >> >                      VramBaseAddress
> >> >                      );
> >> >      if (EFI_ERROR (Status)) {
> >> > +      ASSERT_EFI_ERROR (Status);
> >> >        return Status;
> >> >      }
> >> >
> >> > @@ -233,8 +242,8 @@ LcdPlatformGetVram (
> >> >                      *VramSize,
> >> >                      EFI_MEMORY_WC
> >> >                      );
> >> > -    ASSERT_EFI_ERROR (Status);
> >> >      if (EFI_ERROR (Status)) {
> >> > +      ASSERT_EFI_ERROR (Status);
> >>
> >> Please drop this change
> >
> >  [[Evan Lloyd]] As stated above, this is not inconsequential for some use
> cases.  The ASSERT can only fire when the if is satisfied, so moving the
> ASSERT reduces the number of tests executed in normal execution
> (depending on optimisation level).
> >
>
> As stated above, let's fix DEBUG vs NOOPT instead.
>
> >>
> >> >        gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES
> >> (*VramSize));
> >> >        return Status;
> >> >      }
> >> > @@ -293,6 +302,7 @@ LcdPlatformSetMode (
> >> >    UINT32                SysId;
> >> >
> >> >    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> >> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> >>
> >> No function calls inside ASSERT () please
> > [[Evan Lloyd]] Agreed
> >
> >>
> >> >      return EFI_INVALID_PARAMETER;
> >> >    }
> >> >
> >> > @@ -359,6 +369,7 @@ LcdPlatformSetMode (
> >> >    *                                 (on success).
> >> >    *
> >> >    * @retval EFI_SUCCESS             Success if the requested mode is
> found.
> >> > +  * @retval EFI_INVALID_PARAMETER   Info is NULL.
> >> >    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> >> >  **/
> >> >  EFI_STATUS
> >> > @@ -367,7 +378,9 @@ LcdPlatformQueryMode (
> >> >    OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST Info
> >> >    )
> >> >  {
> >> > -  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> >> > +  if (ModeNumber >= LcdPlatformGetMaxMode () || Info == NULL) {
> >> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> >> > +    ASSERT (Info != NULL);
> >> >      return EFI_INVALID_PARAMETER;
> >> >    }
> >> >
> >> > @@ -415,6 +428,7 @@ LcdPlatformQueryMode (
> >> >    *
> >> >    * @retval EFI_SUCCESS             Success if the requested mode is
> found.
> >> >    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> >> > +  * @retval EFI_INVALID_PARAMETER   One of the OUT parameters is
> >> NULL.
> >> >  **/
> >> >  EFI_STATUS
> >> >  LcdPlatformGetTimings (
> >> > @@ -430,6 +444,28 @@ LcdPlatformGetTimings (
> >> >    )
> >> >  {
> >> >    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> >> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> >> > +    return EFI_INVALID_PARAMETER;
> >> > +  }
> >> > +
> >> > +  if (HRes == NULL
> >> > +    || HSync == NULL
> >> > +    || HBackPorch == NULL
> >> > +    || HFrontPorch == NULL
> >> > +    || VRes == NULL
> >> > +    || VSync == NULL
> >> > +    || VBackPorch == NULL
> >> > +    || VFrontPorch == NULL)
> >> > +  {
> >> > +    // 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);
> >> >      return EFI_INVALID_PARAMETER;
> >> >    }
> >> >
> >> > @@ -452,6 +488,7 @@ LcdPlatformGetTimings (
> >> >    *
> >> >    * @retval EFI_SUCCESS             The requested mode is found.
> >> >    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> >> > +  * @retval EFI_INVALID_PARAMETER   Bpp is NULL.
> >> >  **/
> >> >  EFI_STATUS
> >> >  LcdPlatformGetBpp (
> >> > @@ -460,6 +497,8 @@ LcdPlatformGetBpp (
> >> >    )
> >> >  {
> >> >    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> >> > +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> >> > +    ASSERT (Bpp != NULL);
> >> >      return EFI_INVALID_PARAMETER;
> >> >    }
> >> >
> >> > diff --git a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> >> > b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> >> > index
> >> >
> >>
> 5f950579720fb69e0a481f697a5cc4038158b409..a266671a26f01d31e8dd
> >> b0cf7cbf
> >> > e59d2f4dc49c 100644
> >> > --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> >> > +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> >> > @@ -109,15 +109,15 @@ LcdSetMode (
> >> >               &VBackPorch,
> >> >               &VFrontPorch
> >> >               );
> >> > -  ASSERT_EFI_ERROR (Status);
> >> >    if (EFI_ERROR (Status)) {
> >> > -    return EFI_DEVICE_ERROR;
> >> > +    ASSERT_EFI_ERROR (Status);
> >> > +    return Status;
> >> >    }
> >> >
> >> >    Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
> >> > -  ASSERT_EFI_ERROR (Status);
> >> >    if (EFI_ERROR (Status)) {
> >> > -    return EFI_DEVICE_ERROR;
> >> > +    ASSERT_EFI_ERROR (Status);
> >> > +    return Status;
> >> >    }
> >> >
> >> >    BytesPerPixel = GetBytesPerPixel (LcdBpp); diff --git
> >> > a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> >> > b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> >> > index
> >> >
> >>
> 386e6140a69b045f77ee7fa60c4587d8bf4e7d54..f432c8d802614e8a0e4dd
> >> ab3898f
> >> > 6e0dbf9a1572 100644
> >> > --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> >> > +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> >> > @@ -110,15 +110,15 @@ LcdSetMode (
> >> >               &VBackPorch,
> >> >               &VFrontPorch
> >> >               );
> >> > -  ASSERT_EFI_ERROR (Status);
> >> >    if (EFI_ERROR (Status)) {
> >> > -    return EFI_DEVICE_ERROR;
> >> > +    ASSERT_EFI_ERROR (Status);
> >> > +    return Status;
> >> >    }
> >> >
> >> >    Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
> >> > -  ASSERT_EFI_ERROR (Status);
> >> >    if (EFI_ERROR (Status)) {
> >> > -    return EFI_DEVICE_ERROR;
> >> > +    ASSERT_EFI_ERROR (Status);
> >> > +    return Status;
> >> >    }
> >> >
> >> >    // Disable the CLCD_LcdEn bit
> >> > --
> >> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> >> >
> > IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium. Thank you.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts

Posted by Ard Biesheuvel 7 years ago

On 7 December 2017 at 14:55, Alexei Fedorov <Alexei.Fedorov@arm.com> wrote:
> Hi,
>
> I've compiled current HdLcd.c code with different optimisation levels for
> DEBUG build using GCC 7.2.1, and the assembler code below was generated for:
>
>   ASSERT_EFI_ERROR (Status);
>   if (EFI_ERROR( Status )) {
>     return EFI_DEVICE_ERROR;
>   }
>
> -O0 (default DEBUG option for AARCH64 before Ard's patch):
>
>
>     str    x0, [x29, 72]    //, Status
>
> // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79:
> ASSERT_EFI_ERROR (Status);
>     .loc 1 79 0
>     bl    DebugAssertEnabled    //
>     and    w0, w0, 255    // _1, tmp150
>     cmp    w0, 0    // _1,
>     beq    .L4    //,
>     ldr    x0, [x29, 72]    // Status.9_2, Status
>     cmp    x0, 0    // Status.9_2,
>     bge    .L4    //,
>
> 2.  -Os:
>
>      mov    x19, x0    // Status,
> // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79:
> ASSERT_EFI_ERROR (Status);
>     .loc 1 79 0
>     bl    DebugAssertEnabled    //
> .LVL15:
>     tst    w0, 255    //
>     beq    .L4    //,
>     tbz    x19, #63, .L8    // Status,
>
> 3.  -O3:
>
>     mov    x19, x0    // Status,
> .LVL14:
> // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79:
> ASSERT_EFI_ERROR (Status);
>     bl    DebugAssertEnabled    //
> .LVL15:
>     tst    w0, 255    //
>     beq    .L5    //,
>     tbnz    x19, #63, .L26    // Status,
>
> with DebugAssertEnabled() compiled as:
>
> DebugAssertEnabled:
> .LFB4:
>     .loc 1 203 0
>     .cfi_startproc
> // r:\edk2\MdePkg\Library\BaseDebugLibSerialPort\DebugLib.c:204:   return
> (BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) &
> DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED) != 0);
>     .loc 1 204 0
>     adrp    x0, _gPcd_FixedAtBuild_PcdDebugPropertyMask    // tmp80,
>     add    x0, x0, :lo12:_gPcd_FixedAtBuild_PcdDebugPropertyMask    //
> tmp79, tmp80,
>     ldrb    w0, [x0]    // _gPcd_FixedAtBuild_PcdDebugPropertyMask.4_1,
> _gPcd_FixedAtBuild_PcdDebugPropertyMask
>     and    w0, w0, 1    // _3, _2,
>     cmp    w0, 0    // _3,
>     cset    w0, ne    // tmp82,
>     and    w0, w0, 255    // _4, tmp81
> // r:\edk2\MdePkg\Library\BaseDebugLibSerialPort\DebugLib.c:205: }
>     .loc 1 205 0
>     ret
>
> As you can see each "ASSERT_EFI_ERROR (Status)" macro requires
> DebugAssertEnabled() call taking 8 instructions by itself + minimum 3
> instructions (for -Os, -O3) for Status check, which will be performed by "if
> (EFI_ERROR( Status ))" anyway.
> Please correct me if I'm wrong assuming that placing
> ASSERT_EFI_ERROR (Status)
> inside
> if (EFI_ERROR( Status )) {
> statement is an optimisation improvement.
>

Thanks for digging this up. This appears to be an oversight in the
definition of the ASSERT_EFI_ERROR () macro, which is currently
defined as

#if !defined(MDEPKG_NDEBUG)
  #define ASSERT_EFI_ERROR(StatusParameter)                              \
    do {                                                                 \
      if (DebugAssertEnabled ()) {                                       \
        if (EFI_ERROR (StatusParameter)) {                               \

which is obviously the wrong way around, given that the compiler can
never optimize away the function call, due to its potential side
effects (which it doesn't have)

So I will propose to fix this, by swapping the two if () statements.
Please let me know if the issue is still reproducible with that patch
applied.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts

Posted by Alexei Fedorov 7 years ago

As expected with new ASSERT_EFI_ERROR () definition compiler generates 1 conditional branch at the start:


// r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79:   ASSERT_EFI_ERROR (Status);
    .loc 1 79 0
    tbz    x0, #63, .L4    // Status,



________________________________
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Sent: 07 December 2017 15:10:05
To: Alexei Fedorov
Cc: Evan Lloyd; edk2-devel@lists.01.org; leif.lindholm@linaro.org@arm.com; nd@arm.com@arm.com; ard.biesheuvel@linaro.org@arm.com; Matteo.Carlini@arm.com@arm.com
Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts

On 7 December 2017 at 14:55, Alexei Fedorov <Alexei.Fedorov@arm.com> wrote:
> Hi,
>
> I've compiled current HdLcd.c code with different optimisation levels for
> DEBUG build using GCC 7.2.1, and the assembler code below was generated for:
>
>   ASSERT_EFI_ERROR (Status);
>   if (EFI_ERROR( Status )) {
>     return EFI_DEVICE_ERROR;
>   }
>
> -O0 (default DEBUG option for AARCH64 before Ard's patch):
>
>
>     str    x0, [x29, 72]    //, Status
>
> // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79:
> ASSERT_EFI_ERROR (Status);
>     .loc 1 79 0
>     bl    DebugAssertEnabled    //
>     and    w0, w0, 255    // _1, tmp150
>     cmp    w0, 0    // _1,
>     beq    .L4    //,
>     ldr    x0, [x29, 72]    // Status.9_2, Status
>     cmp    x0, 0    // Status.9_2,
>     bge    .L4    //,
>
> 2.  -Os:
>
>      mov    x19, x0    // Status,
> // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79:
> ASSERT_EFI_ERROR (Status);
>     .loc 1 79 0
>     bl    DebugAssertEnabled    //
> .LVL15:
>     tst    w0, 255    //
>     beq    .L4    //,
>     tbz    x19, #63, .L8    // Status,
>
> 3.  -O3:
>
>     mov    x19, x0    // Status,
> .LVL14:
> // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79:
> ASSERT_EFI_ERROR (Status);
>     bl    DebugAssertEnabled    //
> .LVL15:
>     tst    w0, 255    //
>     beq    .L5    //,
>     tbnz    x19, #63, .L26    // Status,
>
> with DebugAssertEnabled() compiled as:
>
> DebugAssertEnabled:
> .LFB4:
>     .loc 1 203 0
>     .cfi_startproc
> // r:\edk2\MdePkg\Library\BaseDebugLibSerialPort\DebugLib.c:204:   return
> (BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) &
> DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED) != 0);
>     .loc 1 204 0
>     adrp    x0, _gPcd_FixedAtBuild_PcdDebugPropertyMask    // tmp80,
>     add    x0, x0, :lo12:_gPcd_FixedAtBuild_PcdDebugPropertyMask    //
> tmp79, tmp80,
>     ldrb    w0, [x0]    // _gPcd_FixedAtBuild_PcdDebugPropertyMask.4_1,
> _gPcd_FixedAtBuild_PcdDebugPropertyMask
>     and    w0, w0, 1    // _3, _2,
>     cmp    w0, 0    // _3,
>     cset    w0, ne    // tmp82,
>     and    w0, w0, 255    // _4, tmp81
> // r:\edk2\MdePkg\Library\BaseDebugLibSerialPort\DebugLib.c:205: }
>     .loc 1 205 0
>     ret
>
> As you can see each "ASSERT_EFI_ERROR (Status)" macro requires
> DebugAssertEnabled() call taking 8 instructions by itself + minimum 3
> instructions (for -Os, -O3) for Status check, which will be performed by "if
> (EFI_ERROR( Status ))" anyway.
> Please correct me if I'm wrong assuming that placing
> ASSERT_EFI_ERROR (Status)
> inside
> if (EFI_ERROR( Status )) {
> statement is an optimisation improvement.
>

Thanks for digging this up. This appears to be an oversight in the
definition of the ASSERT_EFI_ERROR () macro, which is currently
defined as

#if !defined(MDEPKG_NDEBUG)
  #define ASSERT_EFI_ERROR(StatusParameter)                              \
    do {                                                                 \
      if (DebugAssertEnabled ()) {                                       \
        if (EFI_ERROR (StatusParameter)) {                               \

which is obviously the wrong way around, given that the compiler can
never optimize away the function call, due to its potential side
effects (which it doesn't have)

So I will propose to fix this, by swapping the two if () statements.
Please let me know if the issue is still reproducible with that patch
applied.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts

Posted by Ard Biesheuvel 7 years ago

On 7 December 2017 at 16:53, Alexei Fedorov <Alexei.Fedorov@arm.com> wrote:
> As expected with new ASSERT_EFI_ERROR () definition compiler generates 1
> conditional branch at the start:
>
>
> // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79:
> ASSERT_EFI_ERROR (Status);
>     .loc 1 79 0
>     tbz    x0, #63, .L4    // Status,
>

Thanks for confirming!


>
> ________________________________
> From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Sent: 07 December 2017 15:10:05
> To: Alexei Fedorov
> Cc: Evan Lloyd; edk2-devel@lists.01.org; leif.lindholm@linaro.org@arm.com;
> nd@arm.com@arm.com; ard.biesheuvel@linaro.org@arm.com;
> Matteo.Carlini@arm.com@arm.com
> Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug
> asserts
>
> On 7 December 2017 at 14:55, Alexei Fedorov <Alexei.Fedorov@arm.com> wrote:
>> Hi,
>>
>> I've compiled current HdLcd.c code with different optimisation levels for
>> DEBUG build using GCC 7.2.1, and the assembler code below was generated
>> for:
>>
>>   ASSERT_EFI_ERROR (Status);
>>   if (EFI_ERROR( Status )) {
>>     return EFI_DEVICE_ERROR;
>>   }
>>
>> -O0 (default DEBUG option for AARCH64 before Ard's patch):
>>
>>
>>     str    x0, [x29, 72]    //, Status
>>
>> // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79:
>> ASSERT_EFI_ERROR (Status);
>>     .loc 1 79 0
>>     bl    DebugAssertEnabled    //
>>     and    w0, w0, 255    // _1, tmp150
>>     cmp    w0, 0    // _1,
>>     beq    .L4    //,
>>     ldr    x0, [x29, 72]    // Status.9_2, Status
>>     cmp    x0, 0    // Status.9_2,
>>     bge    .L4    //,
>>
>> 2.  -Os:
>>
>>      mov    x19, x0    // Status,
>> // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79:
>> ASSERT_EFI_ERROR (Status);
>>     .loc 1 79 0
>>     bl    DebugAssertEnabled    //
>> .LVL15:
>>     tst    w0, 255    //
>>     beq    .L4    //,
>>     tbz    x19, #63, .L8    // Status,
>>
>> 3.  -O3:
>>
>>     mov    x19, x0    // Status,
>> .LVL14:
>> // r:\edk2\ArmPlatformPkg\Drivers\LcdGraphicsOutputDxe\HdLcd.c:79:
>> ASSERT_EFI_ERROR (Status);
>>     bl    DebugAssertEnabled    //
>> .LVL15:
>>     tst    w0, 255    //
>>     beq    .L5    //,
>>     tbnz    x19, #63, .L26    // Status,
>>
>> with DebugAssertEnabled() compiled as:
>>
>> DebugAssertEnabled:
>> .LFB4:
>>     .loc 1 203 0
>>     .cfi_startproc
>> // r:\edk2\MdePkg\Library\BaseDebugLibSerialPort\DebugLib.c:204:   return
>> (BOOLEAN) ((PcdGet8(PcdDebugPropertyMask) &
>> DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED) != 0);
>>     .loc 1 204 0
>>     adrp    x0, _gPcd_FixedAtBuild_PcdDebugPropertyMask    // tmp80,
>>     add    x0, x0, :lo12:_gPcd_FixedAtBuild_PcdDebugPropertyMask    //
>> tmp79, tmp80,
>>     ldrb    w0, [x0]    // _gPcd_FixedAtBuild_PcdDebugPropertyMask.4_1,
>> _gPcd_FixedAtBuild_PcdDebugPropertyMask
>>     and    w0, w0, 1    // _3, _2,
>>     cmp    w0, 0    // _3,
>>     cset    w0, ne    // tmp82,
>>     and    w0, w0, 255    // _4, tmp81
>> // r:\edk2\MdePkg\Library\BaseDebugLibSerialPort\DebugLib.c:205: }
>>     .loc 1 205 0
>>     ret
>>
>> As you can see each "ASSERT_EFI_ERROR (Status)" macro requires
>> DebugAssertEnabled() call taking 8 instructions by itself + minimum 3
>> instructions (for -Os, -O3) for Status check, which will be performed by
>> "if
>> (EFI_ERROR( Status ))" anyway.
>> Please correct me if I'm wrong assuming that placing
>> ASSERT_EFI_ERROR (Status)
>> inside
>> if (EFI_ERROR( Status )) {
>> statement is an optimisation improvement.
>>
>
> Thanks for digging this up. This appears to be an oversight in the
> definition of the ASSERT_EFI_ERROR () macro, which is currently
> defined as
>
> #if !defined(MDEPKG_NDEBUG)
>   #define ASSERT_EFI_ERROR(StatusParameter)                              \
>     do {                                                                 \
>       if (DebugAssertEnabled ()) {                                       \
>         if (EFI_ERROR (StatusParameter)) {                               \
>
> which is obviously the wrong way around, given that the compiler can
> never optimize away the function call, due to its potential side
> effects (which it doesn't have)
>
> So I will propose to fix this, by swapping the two if () statements.
> Please let me know if the issue is still reproducible with that patch
> applied.
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium. Thank you.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts

Posted by Leif Lindholm 7 years, 2 months ago

On Tue, Sep 26, 2017 at 09:15:14PM +0100, evan.lloyd@arm.com wrote:
> From: Girish Pathak <girish.pathak@arm.com>
> 
> This change adds some debug assertions e.g to catch NULL pointer errors
> missing in PL11Lcd and HdLcd modules.
> 
> This change also improves related error handling code.
> 
> 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>
> ---
>  ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c       | 44 ++++++++++++++++++--
>  ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c | 43 ++++++++++++++++++-
>  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c                                |  8 ++--
>  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c                             |  8 ++--
>  4 files changed, 90 insertions(+), 13 deletions(-)
> 
> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> index b9859a56988f7e5be0adbaa49048a683fe586bfe..58dd9f0c77e1bc9af559a71d0c7cce72d71c6da5 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> @@ -140,6 +140,7 @@ LcdPlatformInitializeDisplay (
>    *                                 buffer in bytes
>    *
>    * @retval EFI_SUCCESS             Frame buffer memory allocation success.
> +  * @retval EFI_INVALID_PARAMETER   VramBaseAddress or VramSize are NULL.
>    * @retval !(EFI_SUCCESS)          Other errors.
>  **/
>  EFI_STATUS
> @@ -151,6 +152,13 @@ LcdPlatformGetVram (
>    EFI_STATUS              Status;
>    EFI_ALLOCATE_TYPE       AllocationType;
>  
> +  // Check VramBaseAddress and VramSize are not NULL.
> +  if (VramBaseAddress == NULL || VramSize == NULL) {
> +    ASSERT (VramBaseAddress != NULL);
> +    ASSERT (VramSize != NULL);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +

The above is not just a debug assertion change, it also changes
behaviour for RELEASE builds.

I have no objection to these changes in the same patch, but it makes
the commit message misleading. Can you update that please, to say
"adds debug assertions and runtime checks"?

>    // Set the vram size
>    *VramSize = LCD_VRAM_SIZE;
>  
> @@ -169,6 +177,7 @@ LcdPlatformGetVram (
>                    VramBaseAddress
>                    );
>    if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
>      return Status;
>    }
>  
> @@ -179,8 +188,8 @@ LcdPlatformGetVram (
>                    *VramSize,
>                    EFI_MEMORY_WC
>                    );
> -  ASSERT_EFI_ERROR (Status);
>    if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
>      gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize));
>      return Status;
>    }
> @@ -215,6 +224,7 @@ LcdPlatformSetMode (
>    EFI_STATUS            Status;
>  
>    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());

Is this asserting that LcdPlatformGetMaxMode() returns the same thing
if called twice?

If you just always want to assert when reached, use ASSERT (FALSE).
If you explicitly want the message to tell you which operation went
wrong, please use a temporary variable rather than calling a function twice.

>      return EFI_INVALID_PARAMETER;
>    }
>  
> @@ -264,6 +274,7 @@ LcdPlatformSetMode (
>    *
>    * @retval EFI_SUCCESS             Success if the requested mode is found.
>    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> +  * @retval EFI_INVALID_PARAMETER   Info is NULL.
>  **/
>  EFI_STATUS
>  LcdPlatformQueryMode (
> @@ -271,7 +282,9 @@ LcdPlatformQueryMode (
>    OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST  Info
>    )
>  {
> -  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +  if (ModeNumber >= LcdPlatformGetMaxMode () || Info == NULL) {
> +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> +    ASSERT (Info != NULL);
>      return EFI_INVALID_PARAMETER;
>    }
>  
> @@ -334,6 +347,28 @@ LcdPlatformGetTimings (
>    )
>  {
>    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());

ASSERT (FALSE)? Or temporary variable.

> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (HRes == NULL
> +    || HSync == NULL
> +    || HBackPorch == NULL
> +    || HFrontPorch == NULL
> +    || VRes == NULL
> +    || VSync == NULL
> +    || VBackPorch == NULL
> +    || VFrontPorch == NULL)
> +  {
> +    // 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);
>      return EFI_INVALID_PARAMETER;
>    }
>  
> @@ -356,6 +391,7 @@ LcdPlatformGetTimings (
>    *
>    * @retval EFI_SUCCESS             The requested mode is found.
>    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> +  * @retval EFI_INVALID_PARAMETER   Bpp is NULL.
>  **/
>  EFI_STATUS
>  LcdPlatformGetBpp (
> @@ -363,7 +399,9 @@ LcdPlatformGetBpp (
>    OUT LCD_BPP * CONST                    Bpp
>    )
>  {
> -  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +  if (ModeNumber >= LcdPlatformGetMaxMode () || Bpp == NULL) {
> +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> +    ASSERT (Bpp != NULL);
>      return EFI_INVALID_PARAMETER;
>    }
>  
> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> index 6ae13f06d8b396ea1c67f0bcd735a9d70f476400..5a4abd4c6f9e84d3d690af7233c1cebfe1ad339b 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> @@ -191,6 +191,7 @@ LcdPlatformInitializeDisplay (
>    *                                 buffer in bytes
>    *
>    * @retval EFI_SUCCESS             Frame buffer memory allocation success.
> +  * @retval EFI_INVALID_PARAMETER   VramBaseAddress or VramSize is NULL.
>    * @retval !(EFI_SUCCESS)          Other errors.
>  **/
>  EFI_STATUS
> @@ -203,6 +204,13 @@ LcdPlatformGetVram (
>  
>    Status = EFI_SUCCESS;
>  
> +  // Check VramBaseAddress and VramSize are not NULL.
> +  if (VramBaseAddress == NULL || VramSize == NULL) {
> +    ASSERT (VramBaseAddress != NULL);
> +    ASSERT (VramSize != NULL);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    // Is it on the motherboard or on the daughterboard?
>    switch (PL111_CLCD_SITE) {
>  
> @@ -223,6 +231,7 @@ LcdPlatformGetVram (
>                      VramBaseAddress
>                      );
>      if (EFI_ERROR (Status)) {
> +      ASSERT_EFI_ERROR (Status);

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

>        return Status;
>      }
>  
> @@ -233,8 +242,8 @@ LcdPlatformGetVram (
>                      *VramSize,
>                      EFI_MEMORY_WC
>                      );
> -    ASSERT_EFI_ERROR (Status);
>      if (EFI_ERROR (Status)) {
> +      ASSERT_EFI_ERROR (Status);

ASSERT (FALSE)?

>        gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize));
>        return Status;
>      }
> @@ -293,6 +302,7 @@ LcdPlatformSetMode (
>    UINT32                SysId;
>  
>    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());

ASSERT (FALSE)? Or temporary variable?

>      return EFI_INVALID_PARAMETER;
>    }
>  
> @@ -359,6 +369,7 @@ LcdPlatformSetMode (
>    *                                 (on success).
>    *
>    * @retval EFI_SUCCESS             Success if the requested mode is found.
> +  * @retval EFI_INVALID_PARAMETER   Info is NULL.
>    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
>  **/
>  EFI_STATUS
> @@ -367,7 +378,9 @@ LcdPlatformQueryMode (
>    OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST Info
>    )
>  {
> -  if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +  if (ModeNumber >= LcdPlatformGetMaxMode () || Info == NULL) {
> +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());

Temporary variable?

> +    ASSERT (Info != NULL);
>      return EFI_INVALID_PARAMETER;
>    }
>  
> @@ -415,6 +428,7 @@ LcdPlatformQueryMode (
>    *
>    * @retval EFI_SUCCESS             Success if the requested mode is found.
>    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> +  * @retval EFI_INVALID_PARAMETER   One of the OUT parameters is NULL.
>  **/
>  EFI_STATUS
>  LcdPlatformGetTimings (
> @@ -430,6 +444,28 @@ LcdPlatformGetTimings (
>    )
>  {
>    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if (HRes == NULL
> +    || HSync == NULL
> +    || HBackPorch == NULL
> +    || HFrontPorch == NULL
> +    || VRes == NULL
> +    || VSync == NULL
> +    || VBackPorch == NULL
> +    || VFrontPorch == NULL)
> +  {
> +    // 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);
>      return EFI_INVALID_PARAMETER;
>    }
>  
> @@ -452,6 +488,7 @@ LcdPlatformGetTimings (
>    *
>    * @retval EFI_SUCCESS             The requested mode is found.
>    * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> +  * @retval EFI_INVALID_PARAMETER   Bpp is NULL.
>  **/
>  EFI_STATUS
>  LcdPlatformGetBpp (
> @@ -460,6 +497,8 @@ LcdPlatformGetBpp (
>    )
>  {
>    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +    ASSERT (ModeNumber < LcdPlatformGetMaxMode ());
> +    ASSERT (Bpp != NULL);
>      return EFI_INVALID_PARAMETER;
>    }
>  
> diff --git a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> index 5f950579720fb69e0a481f697a5cc4038158b409..a266671a26f01d31e8ddb0cf7cbfe59d2f4dc49c 100644
> --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> @@ -109,15 +109,15 @@ LcdSetMode (
>               &VBackPorch,
>               &VFrontPorch
>               );
> -  ASSERT_EFI_ERROR (Status);
>    if (EFI_ERROR (Status)) {
> -    return EFI_DEVICE_ERROR;
> +    ASSERT_EFI_ERROR (Status);

ASSERT (FALSE).

> +    return Status;

This is a change in behaviour, since the function can now potentially
return other values than it has in the past. Again, it may be fine,
but deserves a mention in the commit message.

>    }
>  
>    Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
> -  ASSERT_EFI_ERROR (Status);
>    if (EFI_ERROR (Status)) {
> -    return EFI_DEVICE_ERROR;
> +    ASSERT_EFI_ERROR (Status);
> +    return Status;

Same as above.

>    }
>  
>    BytesPerPixel = GetBytesPerPixel (LcdBpp);
> diff --git a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> index 386e6140a69b045f77ee7fa60c4587d8bf4e7d54..f432c8d802614e8a0e4ddab3898f6e0dbf9a1572 100644
> --- a/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> +++ b/ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> @@ -110,15 +110,15 @@ LcdSetMode (
>               &VBackPorch,
>               &VFrontPorch
>               );
> -  ASSERT_EFI_ERROR (Status);
>    if (EFI_ERROR (Status)) {
> -    return EFI_DEVICE_ERROR;
> +    ASSERT_EFI_ERROR (Status);
> +    return Status;

Same as above.

>    }
>  
>    Status = LcdPlatformGetBpp (ModeNumber, &LcdBpp);
> -  ASSERT_EFI_ERROR (Status);
>    if (EFI_ERROR (Status)) {
> -    return EFI_DEVICE_ERROR;
> +    ASSERT_EFI_ERROR (Status);
> +    return Status;

Same as above.

/
    Leif

>    }
>  
>    // Disable the CLCD_LcdEn bit
> -- 
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel