Re: [PATCH] BaseTools/CommonLib: drop the use of MAX_ADDRESS

[Laszlo Ersek <lersek@redhat.com>]

On 12/05/18 09:24, Ard Biesheuvel wrote:
> The macro MAX_ADDRESS represents the largest virtual address that
> is valid for a certain architecture. For the BaseTools, this quantity
> is irrelevant, since the same tools can be used to build for different
> targets.
> 
> Since we only refer to it in a single place, which is an ASSERT() that
> doesn't seem particularly useful (it ensures that memcpy() will not
> be called with arguments that will make it read beyond the end of the
> address space and wrap around), let's drop the ASSERT and all references
> to MAX_ADDRESS.
> 
> Cc: Bob Feng <bob.c.feng@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Yonghong Zhu <yonghong.zhu@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  BaseTools/Source/C/Include/AArch64/ProcessorBind.h | 5 -----
>  BaseTools/Source/C/Include/Arm/ProcessorBind.h     | 5 -----
>  BaseTools/Source/C/Include/Common/UefiBaseTypes.h  | 1 -
>  BaseTools/Source/C/Include/Ia32/ProcessorBind.h    | 5 -----
>  BaseTools/Source/C/Include/X64/ProcessorBind.h     | 5 -----
>  BaseTools/Source/C/Common/CommonLib.c              | 1 -
>  6 files changed, 22 deletions(-)
> 
> diff --git a/BaseTools/Source/C/Include/AArch64/ProcessorBind.h b/BaseTools/Source/C/Include/AArch64/ProcessorBind.h
> index e7e9d83198a6..f956cab453f0 100644
> --- a/BaseTools/Source/C/Include/AArch64/ProcessorBind.h
> +++ b/BaseTools/Source/C/Include/AArch64/ProcessorBind.h
> @@ -90,11 +90,6 @@ typedef INT64   INTN;
>  ///
>  #define MAX_2_BITS   0xC000000000000000
>  
> -///
> -/// Maximum legal AARCH64  address
> -///
> -#define MAX_ADDRESS  0xFFFFFFFFFFFFFFFF
> -
>  ///
>  /// The stack alignment required for AARCH64
>  ///
> diff --git a/BaseTools/Source/C/Include/Arm/ProcessorBind.h b/BaseTools/Source/C/Include/Arm/ProcessorBind.h
> index be4aac97664d..856d2bd9eff7 100644
> --- a/BaseTools/Source/C/Include/Arm/ProcessorBind.h
> +++ b/BaseTools/Source/C/Include/Arm/ProcessorBind.h
> @@ -88,11 +88,6 @@ typedef INT32   INTN;
>  ///
>  #define MAX_2_BITS   0xC0000000
>  
> -///
> -/// Maximum legal ARM address
> -///
> -#define MAX_ADDRESS  0xFFFFFFFF
> -
>  ///
>  /// The stack alignment required for ARM
>  ///
> diff --git a/BaseTools/Source/C/Include/Common/UefiBaseTypes.h b/BaseTools/Source/C/Include/Common/UefiBaseTypes.h
> index aa1aef3ce638..696ac15e4cd5 100644
> --- a/BaseTools/Source/C/Include/Common/UefiBaseTypes.h
> +++ b/BaseTools/Source/C/Include/Common/UefiBaseTypes.h
> @@ -170,6 +170,5 @@ typedef union {
>  
>  
>  #define EFI_MAX_BIT               MAX_BIT
> -#define EFI_MAX_ADDRESS           MAX_ADDRESS
>  
>  #endif
> diff --git a/BaseTools/Source/C/Include/Ia32/ProcessorBind.h b/BaseTools/Source/C/Include/Ia32/ProcessorBind.h
> index 4719b53d37fa..96ac691df042 100644
> --- a/BaseTools/Source/C/Include/Ia32/ProcessorBind.h
> +++ b/BaseTools/Source/C/Include/Ia32/ProcessorBind.h
> @@ -131,11 +131,6 @@ typedef INT32   INTN;
>  #define MAX_BIT     0x80000000
>  #define MAX_2_BITS  0xC0000000
>  
> -//
> -// Maximum legal IA-32 address
> -//
> -#define MAX_ADDRESS   0xFFFFFFFF
> -
>  //
>  // Modifier to ensure that all protocol member functions and EFI intrinsics
>  // use the correct C calling convention. All protocol member functions and
> diff --git a/BaseTools/Source/C/Include/X64/ProcessorBind.h b/BaseTools/Source/C/Include/X64/ProcessorBind.h
> index c625f8cef4a1..987738508333 100644
> --- a/BaseTools/Source/C/Include/X64/ProcessorBind.h
> +++ b/BaseTools/Source/C/Include/X64/ProcessorBind.h
> @@ -150,11 +150,6 @@ typedef INT64   INTN;
>  #define MAX_BIT     0x8000000000000000ULL
>  #define MAX_2_BITS  0xC000000000000000ULL
>  
> -//
> -// Maximum legal Itanium-based address
> -//
> -#define MAX_ADDRESS   0xFFFFFFFFFFFFFFFFULL
> -
>  //
>  // Modifier to ensure that all protocol member functions and EFI intrinsics
>  // use the correct C calling convention. All protocol member functions and
> diff --git a/BaseTools/Source/C/Common/CommonLib.c b/BaseTools/Source/C/Common/CommonLib.c
> index 42dfa821624d..5c40fdb5fd49 100644
> --- a/BaseTools/Source/C/Common/CommonLib.c
> +++ b/BaseTools/Source/C/Common/CommonLib.c
> @@ -1236,7 +1236,6 @@ InternalAllocateCopyPool (
>    VOID  *Memory;
>  
>    ASSERT (Buffer != NULL);
> -  ASSERT (AllocationSize <= (MAX_ADDRESS - (UINTN) Buffer + 1));
>  
>    Memory = malloc (AllocationSize);
>    if (Memory != NULL) {
> 

I agree; the malloc() on the next line will cover this.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo