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

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

On 22 December 2017 at 19:08,  <evan.lloyd@arm.com> wrote:
> From: Girish Pathak <girish.pathak at arm.com>
>
> This change adds some debug assertions e.g to catch NULL pointer errors
> missing in PL11Lcd and HdLcd platform libraries.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Girish Pathak <girish.pathak@arm.com>
> Signed-off-by: Evan Lloyd <evan.lloyd@arm.com>
> ---
>  Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c       | 22 +++++++++++++++++-
>  Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c | 24 +++++++++++++++++++-
>  2 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> index 6afd764897f49c64490ce891682f99bb0f5d993b..a8fe8696da0653017ce9fa6e4a86caf283bc04c9 100644
> --- a/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> +++ b/Platform/ARM/VExpressPkg/Library/HdLcdArmVExpressLib/HdLcdArmVExpress.c
> @@ -153,6 +153,9 @@ LcdPlatformGetVram (
>    EFI_STATUS              Status;
>    EFI_ALLOCATE_TYPE       AllocationType;
>
> +  ASSERT (VramBaseAddress != NULL);
> +  ASSERT (VramSize != NULL);
> +
>    // Set the vram size
>    *VramSize = LCD_VRAM_SIZE;
>
> @@ -171,6 +174,7 @@ LcdPlatformGetVram (
>                    VramBaseAddress
>                    );
>    if (EFI_ERROR (Status)) {
> +    ASSERT (FALSE);
>      return Status;
>    }
>
> @@ -181,8 +185,8 @@ LcdPlatformGetVram (
>                    *VramSize,
>                    EFI_MEMORY_WC
>                    );
> -  ASSERT_EFI_ERROR (Status);
>    if (EFI_ERROR (Status)) {
> +    ASSERT (FALSE);

As in the sibling patch against EDK2, this patch makes it more
difficult to figure out what went wrong when you hit the ASSERT.
ASSERT_EFI_ERROR prints the value of Status, ASSERT(FALSE) only prints
'0 != 1'

>      gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize));
>      return Status;
>    }
> @@ -221,6 +225,7 @@ LcdPlatformSetMode (
>    EFI_STATUS            Status;
>
>    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +    ASSERT (FALSE);

These are fine: the code itself explains adequately which condition
triggered the ASSERT to fire.

>      return EFI_INVALID_PARAMETER;
>    }
>
> @@ -279,7 +284,10 @@ LcdPlatformQueryMode (
>    OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST  Info
>    )
>  {
> +  ASSERT (Info != NULL);
> +
>    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +    ASSERT (FALSE);
>      return EFI_INVALID_PARAMETER;
>    }
>
> @@ -343,7 +351,18 @@ LcdPlatformGetTimings (
>    OUT UINT32 * CONST                            VFrontPorch
>    )
>  {
> +  // One of the pointers is NULL
> +  ASSERT (HRes != NULL);
> +  ASSERT (HSync != NULL);
> +  ASSERT (HBackPorch != NULL);
> +  ASSERT (HFrontPorch != NULL);
> +  ASSERT (VRes != NULL);
> +  ASSERT (VSync != NULL);
> +  ASSERT (VBackPorch != NULL);
> +  ASSERT (VFrontPorch != NULL);
> +
>    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +    ASSERT (FALSE);
>      return EFI_INVALID_PARAMETER;
>    }
>
> @@ -376,6 +395,7 @@ LcdPlatformGetBpp (
>    )
>  {
>    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +    ASSERT (FALSE);
>      return EFI_INVALID_PARAMETER;
>    }
>
> diff --git a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> index 799fb3fc781ce04bb64cb1fa0b87f262a670ed78..fd4eea8f8e2397bc7d4ddf4cfe3dcc97a5109edb 100644
> --- a/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> +++ b/Platform/ARM/VExpressPkg/Library/PL111LcdArmVExpressLib/PL111LcdArmVExpress.c
> @@ -205,6 +205,9 @@ LcdPlatformGetVram (
>
>    Status = EFI_SUCCESS;
>
> +  ASSERT (VramBaseAddress != NULL);
> +  ASSERT (VramSize != NULL);
> +
>    // Is it on the motherboard or on the daughterboard?
>    switch (PL111_CLCD_SITE) {
>
> @@ -225,6 +228,7 @@ LcdPlatformGetVram (
>                      VramBaseAddress
>                      );
>      if (EFI_ERROR (Status)) {
> +      ASSERT (FALSE);
>        return Status;
>      }
>
> @@ -235,8 +239,8 @@ LcdPlatformGetVram (
>                      *VramSize,
>                      EFI_MEMORY_WC
>                      );
> -    ASSERT_EFI_ERROR (Status);
>      if (EFI_ERROR (Status)) {
> +      ASSERT (FALSE);
>        gBS->FreePages (*VramBaseAddress, EFI_SIZE_TO_PAGES (*VramSize));
>        return Status;
>      }
> @@ -294,6 +298,7 @@ LcdPlatformSetMode (
>    UINT32                SysId;
>
>    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +    ASSERT (FALSE);
>      return EFI_INVALID_PARAMETER;
>    }
>
> @@ -369,7 +374,10 @@ LcdPlatformQueryMode (
>    OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION * CONST Info
>    )
>  {
> +  ASSERT (Info != NULL);
> +
>    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +    ASSERT (FALSE);
>      return EFI_INVALID_PARAMETER;
>    }
>
> @@ -433,7 +441,18 @@ LcdPlatformGetTimings (
>    OUT UINT32 * CONST                      VFrontPorch
>    )
>  {
> +  // One of the pointers is NULL
> +  ASSERT (HRes != NULL);
> +  ASSERT (HSync != NULL);
> +  ASSERT (HBackPorch != NULL);
> +  ASSERT (HFrontPorch != NULL);
> +  ASSERT (VRes != NULL);
> +  ASSERT (VSync != NULL);
> +  ASSERT (VBackPorch != NULL);
> +  ASSERT (VFrontPorch != NULL);
> +
>    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +    ASSERT (FALSE);
>      return EFI_INVALID_PARAMETER;
>    }
>
> @@ -465,7 +484,10 @@ LcdPlatformGetBpp (
>    OUT LCD_BPP * CONST                     Bpp
>    )
>  {
> +  ASSERT (Bpp != NULL);
> +
>    if (ModeNumber >= LcdPlatformGetMaxMode ()) {
> +    ASSERT (FALSE);
>      return EFI_INVALID_PARAMETER;
>    }
>
> --
> Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel