Re: [PATCH v3 05/12] IntelFsp2WrapperPkg: Refine casting expression result to bigger size

["Yao, Jiewen" <jiewen.yao@intel.com>]

Reviewed-by: jiewen.yao@intel.com

> -----Original Message-----
> From: Wu, Hao A
> Sent: Saturday, February 25, 2017 1:12 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH v3 05/12] IntelFsp2WrapperPkg: Refine casting expression result
> to bigger size
> 
> There are cases that the operands of an expression are all with rank less
> than UINT64/INT64 and the result of the expression is explicitly cast to
> UINT64/INT64 to fit the target size.
> 
> An example will be:
> UINT32 a,b;
> // a and b can be any unsigned int type with rank less than UINT64, like
> // UINT8, UINT16, etc.
> UINT64 c;
> c = (UINT64) (a + b);
> 
> Some static code checkers may warn that the expression result might
> overflow within the rank of "int" (integer promotions) and the result is
> then cast to a bigger size.
> 
> The commit refines codes by the following rules:
> 1). When the expression is possible to overflow the range of unsigned int/
> int:
> c = (UINT64)a + b;
> 
> 2). When the expression will not overflow within the rank of "int", remove
> the explicit type casts:
> c = a + b;
> 
> 3). When the expression will be cast to pointer of possible greater size:
> UINT32 a,b;
> VOID *c;
> c = (VOID *)(UINTN)(a + b); --> c = (VOID *)((UINTN)a + b);
> 
> 4). When one side of a comparison expression contains only operands with
> rank less than UINT32:
> UINT8 a;
> UINT16 b;
> UINTN c;
> if ((UINTN)(a + b) > c) {...} --> if (((UINT32)a + b) > c) {...}
> 
> For rule 4), if we remove the 'UINTN' type cast like:
> if (a + b > c) {...}
> The VS compiler will complain with warning C4018 (signed/unsigned
> mismatch, level 3 warning) due to promoting 'a + b' to type 'int'.
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> ---
>  IntelFsp2WrapperPkg/FspWrapperNotifyDxe/LoadBelow4G.c               |
> 4 ++--
>  IntelFsp2WrapperPkg/Library/BaseFspWrapperApiLib/FspWrapperApiLib.c | 10
> +++++-----
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/LoadBelow4G.c
> b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/LoadBelow4G.c
> index ff2f563..dc5ef89 100644
> --- a/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/LoadBelow4G.c
> +++ b/IntelFsp2WrapperPkg/FspWrapperNotifyDxe/LoadBelow4G.c
> @@ -1,6 +1,6 @@
>  /** @file
> 
> -Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2015 - 2017, Intel Corporation. All rights reserved.<BR>
> 
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions
> @@ -115,7 +115,7 @@ RelocateImageUnder4GIfNeeded (
>    // Align buffer on section boundary
>    //
>    ImageContext.ImageAddress += ImageContext.SectionAlignment - 1;
> -  ImageContext.ImageAddress &=
> ~((EFI_PHYSICAL_ADDRESS)(ImageContext.SectionAlignment - 1));
> +  ImageContext.ImageAddress &=
> ~((EFI_PHYSICAL_ADDRESS)ImageContext.SectionAlignment - 1);
>    //
>    // Load the image to our new buffer
>    //
> diff --git
> a/IntelFsp2WrapperPkg/Library/BaseFspWrapperApiLib/FspWrapperApiLib.c
> b/IntelFsp2WrapperPkg/Library/BaseFspWrapperApiLib/FspWrapperApiLib.c
> index 8cf136f..38de415 100644
> --- a/IntelFsp2WrapperPkg/Library/BaseFspWrapperApiLib/FspWrapperApiLib.c
> +++
> b/IntelFsp2WrapperPkg/Library/BaseFspWrapperApiLib/FspWrapperApiLib.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Provide FSP API related function.
> 
> -  Copyright (c) 2014 - 2016, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2014 - 2017, Intel Corporation. All rights reserved.<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD
> License
>    which accompanies this distribution.  The full text of the license may be
> found at
> @@ -99,7 +99,7 @@ CallFspNotifyPhase (
>      return EFI_DEVICE_ERROR;
>    }
> 
> -  NotifyPhaseApi = (FSP_NOTIFY_PHASE)(UINTN)(FspHeader->ImageBase +
> FspHeader->NotifyPhaseEntryOffset);
> +  NotifyPhaseApi = (FSP_NOTIFY_PHASE)((UINTN)FspHeader->ImageBase +
> FspHeader->NotifyPhaseEntryOffset);
>    InterruptState = SaveAndDisableInterrupts ();
>    Status = Execute32BitCode ((UINTN)NotifyPhaseApi,
> (UINTN)NotifyPhaseParams, (UINTN)NULL);
>    SetInterruptState (InterruptState);
> @@ -132,7 +132,7 @@ CallFspMemoryInit (
>      return EFI_DEVICE_ERROR;
>    }
> 
> -  FspMemoryInitApi = (FSP_MEMORY_INIT)(UINTN)(FspHeader->ImageBase +
> FspHeader->FspMemoryInitEntryOffset);
> +  FspMemoryInitApi = (FSP_MEMORY_INIT)((UINTN)FspHeader->ImageBase +
> FspHeader->FspMemoryInitEntryOffset);
>    InterruptState = SaveAndDisableInterrupts ();
>    Status = Execute32BitCode ((UINTN)FspMemoryInitApi,
> (UINTN)FspmUpdDataPtr, (UINTN)HobListPtr);
>    SetInterruptState (InterruptState);
> @@ -163,7 +163,7 @@ CallTempRamExit (
>      return EFI_DEVICE_ERROR;
>    }
> 
> -  TempRamExitApi = (FSP_TEMP_RAM_EXIT)(UINTN)(FspHeader->ImageBase
> + FspHeader->TempRamExitEntryOffset);
> +  TempRamExitApi = (FSP_TEMP_RAM_EXIT)((UINTN)FspHeader->ImageBase
> + FspHeader->TempRamExitEntryOffset);
>    InterruptState = SaveAndDisableInterrupts ();
>    Status = Execute32BitCode ((UINTN)TempRamExitApi,
> (UINTN)TempRamExitParam, (UINTN)NULL);
>    SetInterruptState (InterruptState);
> @@ -194,7 +194,7 @@ CallFspSiliconInit (
>      return EFI_DEVICE_ERROR;
>    }
> 
> -  FspSiliconInitApi = (FSP_SILICON_INIT)(UINTN)(FspHeader->ImageBase +
> FspHeader->FspSiliconInitEntryOffset);
> +  FspSiliconInitApi = (FSP_SILICON_INIT)((UINTN)FspHeader->ImageBase +
> FspHeader->FspSiliconInitEntryOffset);
>    InterruptState = SaveAndDisableInterrupts ();
>    Status = Execute32BitCode ((UINTN)FspSiliconInitApi,
> (UINTN)FspsUpdDataPtr, (UINTN)NULL);
>    SetInterruptState (InterruptState);
> --
> 1.9.5.msysgit.0