[PATCH v2 0/6] BaseTools: get rid of MAX_UINTN

[PATCH v2 0/6] BaseTools: get rid of MAX_UINTN

[Ard Biesheuvel]

There should be no reason for the build tools to care about the native
word size of a particular target, so relying on a definition of MAX_UINTN
is definitely wrong, and most likely inaccurate on 32-bit build hosts.

So refactor the code in CommonLib and DevicePath so we no longer rely
on this definition.

Changes since v1:
- miss type change in #1 causing a build failure on MSVC
- add acks from Jaben

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Yonghong Zhu <yonghong.zhu@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Bob Feng <bob.c.feng@intel.com>
Cc: Jaben Carsey <jaben.carsey@intel.com>

Ard Biesheuvel (6):
  BaseTools/CommonLib: avoid using 'native' word size in IP address
    handling
  BaseTools/CommonLib: use explicit 64-bit type in Strtoi()
  BaseTools/DevicePath: use explicit 64-bit number parsing routines
  BaseTools/DevicePath: use MAX_UINT16 as default device path max size
  BaseTools/CommonLib: get rid of 'native' type string parsing routines
  BaseTools/CommonLib: drop definition of MAX_UINTN

 BaseTools/Source/C/Common/CommonLib.h         |  25 ---
 BaseTools/Source/C/Common/CommonLib.c         | 206 ++----------------
 .../Source/C/DevicePath/DevicePathFromText.c  |   4 +-
 .../Source/C/DevicePath/DevicePathUtilities.c |   4 +-
 4 files changed, 25 insertions(+), 214 deletions(-)

-- 
2.19.1

[PATCH v2 1/6] BaseTools/CommonLib: avoid using 'native' word size in IP address handling

[Ard Biesheuvel]

In the context of the BaseTools, there is no such thing as a native word
size, given that the same set of tools may be used to build a firmware
image consisting of both 32-bit and 64-bit modules.

So update StrToIpv4Address() and StrToIpv6Address() to use UINT64
types instead of UINTN types when parsing strings.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 BaseTools/Source/C/Common/CommonLib.c | 28 ++++++++++----------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/BaseTools/Source/C/Common/CommonLib.c b/BaseTools/Source/C/Common/CommonLib.c
index 618aadac781a..7c559bc3c222 100644
--- a/BaseTools/Source/C/Common/CommonLib.c
+++ b/BaseTools/Source/C/Common/CommonLib.c
@@ -1785,7 +1785,7 @@ StrToIpv4Address (
 {
   RETURN_STATUS          Status;
   UINTN                  AddressIndex;
-  UINTN                  Uintn;
+  UINT64                 Uint64;
   EFI_IPv4_ADDRESS       LocalAddress;
   UINT8                  LocalPrefixLength;
   CHAR16                 *Pointer;
@@ -1812,7 +1812,7 @@ StrToIpv4Address (
     //
     // Get D or P.
     //
-    Status = StrDecimalToUintnS ((CONST CHAR16 *) Pointer, &Pointer, &Uintn);
+    Status = StrDecimalToUint64S ((CONST CHAR16 *) Pointer, &Pointer, &Uint64);
     if (RETURN_ERROR (Status)) {
       return RETURN_UNSUPPORTED;
     }
@@ -1820,18 +1820,18 @@ StrToIpv4Address (
       //
       // It's P.
       //
-      if (Uintn > 32) {
+      if (Uint64 > 32) {
         return RETURN_UNSUPPORTED;
       }
-      LocalPrefixLength = (UINT8) Uintn;
+      LocalPrefixLength = (UINT8) Uint64;
     } else {
       //
       // It's D.
       //
-      if (Uintn > MAX_UINT8) {
+      if (Uint64 > MAX_UINT8) {
         return RETURN_UNSUPPORTED;
       }
-      LocalAddress.Addr[AddressIndex] = (UINT8) Uintn;
+      LocalAddress.Addr[AddressIndex] = (UINT8) Uint64;
       AddressIndex++;
     }
 
@@ -1888,7 +1888,7 @@ StrToIpv6Address (
 {
   RETURN_STATUS          Status;
   UINTN                  AddressIndex;
-  UINTN                  Uintn;
+  UINT64                 Uint64;
   EFI_IPv6_ADDRESS       LocalAddress;
   UINT8                  LocalPrefixLength;
   CONST CHAR16           *Pointer;
@@ -1969,7 +1969,7 @@ StrToIpv6Address (
         //
         // Get X.
         //
-        Status = StrHexToUintnS (Pointer, &End, &Uintn);
+        Status = StrHexToUint64S (Pointer, &End, &Uint64);
         if (RETURN_ERROR (Status) || End - Pointer > 4) {
           //
           // Number of hexadecimal digit characters is no more than 4.
@@ -1978,24 +1978,24 @@ StrToIpv6Address (
         }
         Pointer = End;
         //
-        // Uintn won't exceed MAX_UINT16 if number of hexadecimal digit characters is no more than 4.
+        // Uint64 won't exceed MAX_UINT16 if number of hexadecimal digit characters is no more than 4.
         //
         ASSERT (AddressIndex + 1 < ARRAY_SIZE (Address->Addr));
-        LocalAddress.Addr[AddressIndex] = (UINT8) ((UINT16) Uintn >> 8);
-        LocalAddress.Addr[AddressIndex + 1] = (UINT8) Uintn;
+        LocalAddress.Addr[AddressIndex] = (UINT8) ((UINT16) Uint64 >> 8);
+        LocalAddress.Addr[AddressIndex + 1] = (UINT8) Uint64;
         AddressIndex += 2;
       } else {
         //
         // Get P, then exit the loop.
         //
-        Status = StrDecimalToUintnS (Pointer, &End, &Uintn);
-        if (RETURN_ERROR (Status) || End == Pointer || Uintn > 128) {
+        Status = StrDecimalToUint64S (Pointer, &End, &Uint64);
+        if (RETURN_ERROR (Status) || End == Pointer || Uint64 > 128) {
           //
           // Prefix length should not exceed 128.
           //
           return RETURN_UNSUPPORTED;
         }
-        LocalPrefixLength = (UINT8) Uintn;
+        LocalPrefixLength = (UINT8) Uint64;
         Pointer = End;
         break;
       }
-- 
2.19.1

[PATCH v2 2/6] BaseTools/CommonLib: use explicit 64-bit type in Strtoi()

[Ard Biesheuvel]

Don't use the native word size string to number parsing routines,
but instead, use the 64-bit one and cast to UINTN.

Currently, the only user is in Source/C/DevicePath/DevicePathFromText.c
which takes care to use Strtoi64 () unless it assumes the value fits
in 32-bit, so this change is a no-op even on 32-bit build hosts.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
---
 BaseTools/Source/C/Common/CommonLib.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Source/C/Common/CommonLib.c b/BaseTools/Source/C/Common/CommonLib.c
index 7c559bc3c222..4a28f635f3a8 100644
--- a/BaseTools/Source/C/Common/CommonLib.c
+++ b/BaseTools/Source/C/Common/CommonLib.c
@@ -2252,9 +2252,9 @@ Strtoi (
   )
 {
   if (IsHexStr (Str)) {
-    return StrHexToUintn (Str);
+    return (UINTN)StrHexToUint64 (Str);
   } else {
-    return StrDecimalToUintn (Str);
+    return (UINTN)StrDecimalToUint64 (Str);
   }
 }
 
-- 
2.19.1

[PATCH v2 3/6] BaseTools/DevicePath: use explicit 64-bit number parsing routines

[Ard Biesheuvel]

Replace invocations of StrHexToUintn() with StrHexToUint64(), so
that we can drop the former.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
---
 BaseTools/Source/C/DevicePath/DevicePathFromText.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Source/C/DevicePath/DevicePathFromText.c b/BaseTools/Source/C/DevicePath/DevicePathFromText.c
index 555efa1acdde..6151926af9aa 100644
--- a/BaseTools/Source/C/DevicePath/DevicePathFromText.c
+++ b/BaseTools/Source/C/DevicePath/DevicePathFromText.c
@@ -520,7 +520,7 @@ EisaIdFromText (
   return (((Text[0] - 'A' + 1) & 0x1f) << 10)
        + (((Text[1] - 'A' + 1) & 0x1f) <<  5)
        + (((Text[2] - 'A' + 1) & 0x1f) <<  0)
-       + (UINT32) (StrHexToUintn (&Text[3]) << 16)
+       + (UINT32) (StrHexToUint64 (&Text[3]) << 16)
        ;
 }
 
@@ -1506,7 +1506,7 @@ DevPathFromTextNVMe (
 
   Index = sizeof (Nvme->NamespaceUuid) / sizeof (UINT8);
   while (Index-- != 0) {
-    Uuid[Index] = (UINT8) StrHexToUintn (SplitStr (&NamespaceUuidStr, L'-'));
+    Uuid[Index] = (UINT8) StrHexToUint64 (SplitStr (&NamespaceUuidStr, L'-'));
   }
 
   return (EFI_DEVICE_PATH_PROTOCOL *) Nvme;
-- 
2.19.1

[PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as default device path max size

[Ard Biesheuvel]

Replace the default size limit of IsDevicePathValid() with a value
that does not depend on the native word size of the build host.

64 KB seems sufficient as the upper bound of a device path handled
by UEFI.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
---
 BaseTools/Source/C/DevicePath/DevicePathUtilities.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
index d4ec2742b7c8..ba7f83e53070 100644
--- a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
+++ b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
@@ -62,7 +62,7 @@ IsDevicePathValid (
   ASSERT (DevicePath != NULL);
 
   if (MaxSize == 0) {
-    MaxSize = MAX_UINTN;
+    MaxSize = MAX_UINT16;
  }
 
   //
@@ -78,7 +78,7 @@ IsDevicePathValid (
       return FALSE;
     }
 
-    if (NodeLength > MAX_UINTN - Size) {
+    if (NodeLength > MAX_UINT16 - Size) {
       return FALSE;
     }
     Size += NodeLength;
-- 
2.19.1

[PATCH v2 5/6] BaseTools/CommonLib: get rid of 'native' type string parsing routines

[Ard Biesheuvel]

Parsing a string into an integer variable of the native word size
is not defined for the BaseTools, since the same tools may be used
to build firmware for different targets with different native word
sizes.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 BaseTools/Source/C/Common/CommonLib.h |  24 ---
 BaseTools/Source/C/Common/CommonLib.c | 174 +-------------------
 2 files changed, 5 insertions(+), 193 deletions(-)

diff --git a/BaseTools/Source/C/Common/CommonLib.h b/BaseTools/Source/C/Common/CommonLib.h
index fa10fac4682a..6930d9227b87 100644
--- a/BaseTools/Source/C/Common/CommonLib.h
+++ b/BaseTools/Source/C/Common/CommonLib.h
@@ -250,16 +250,6 @@ StrSize (
   CONST CHAR16              *String
   );
 
-UINTN
-StrHexToUintn (
-  CONST CHAR16              *String
-  );
-
-UINTN
-StrDecimalToUintn (
-  CONST CHAR16              *String
-  );
-
 UINT64
 StrHexToUint64 (
   CONST CHAR16             *String
@@ -277,13 +267,6 @@ StrHexToUint64S (
     UINT64             *Data
   );
 
-RETURN_STATUS
-StrHexToUintnS (
-    CONST CHAR16             *String,
-         CHAR16             **EndPointer,  OPTIONAL
-         UINTN              *Data
-  );
-
 RETURN_STATUS
 StrDecimalToUint64S (
     CONST CHAR16             *String,
@@ -291,13 +274,6 @@ StrDecimalToUint64S (
          UINT64             *Data
   );
 
-RETURN_STATUS
-StrDecimalToUintnS (
-    CONST CHAR16             *String,
-         CHAR16             **EndPointer,  OPTIONAL
-         UINTN              *Data
-  );
-
 VOID *
 ReallocatePool (
    UINTN  OldSize,
diff --git a/BaseTools/Source/C/Common/CommonLib.c b/BaseTools/Source/C/Common/CommonLib.c
index 4a28f635f3a8..42dfa821624d 100644
--- a/BaseTools/Source/C/Common/CommonLib.c
+++ b/BaseTools/Source/C/Common/CommonLib.c
@@ -882,72 +882,6 @@ InternalSafeStringNoStrOverlap (
   return !InternalSafeStringIsOverlap (Str1, Size1 * sizeof(CHAR16), Str2, Size2 * sizeof(CHAR16));
 }
 
-RETURN_STATUS
-StrDecimalToUintnS (
-    CONST CHAR16             *String,
-         CHAR16             **EndPointer,  OPTIONAL
-         UINTN              *Data
-  )
-{
-  ASSERT (((UINTN) String & BIT0) == 0);
-
-  //
-  // 1. Neither String nor Data shall be a null pointer.
-  //
-  SAFE_STRING_CONSTRAINT_CHECK ((String != NULL), RETURN_INVALID_PARAMETER);
-  SAFE_STRING_CONSTRAINT_CHECK ((Data != NULL), RETURN_INVALID_PARAMETER);
-
-  //
-  // 2. The length of String shall not be greater than RSIZE_MAX.
-  //
-  if (RSIZE_MAX != 0) {
-    SAFE_STRING_CONSTRAINT_CHECK ((StrnLenS (String, RSIZE_MAX + 1) <= RSIZE_MAX), RETURN_INVALID_PARAMETER);
-  }
-
-  if (EndPointer != NULL) {
-    *EndPointer = (CHAR16 *) String;
-  }
-
-  //
-  // Ignore the pad spaces (space or tab)
-  //
-  while ((*String == L' ') || (*String == L'\t')) {
-    String++;
-  }
-
-  //
-  // Ignore leading Zeros after the spaces
-  //
-  while (*String == L'0') {
-    String++;
-  }
-
-  *Data = 0;
-
-  while (InternalIsDecimalDigitCharacter (*String)) {
-    //
-    // If the number represented by String overflows according to the range
-    // defined by UINTN, then MAX_UINTN is stored in *Data and
-    // RETURN_UNSUPPORTED is returned.
-    //
-    if (*Data > ((MAX_UINTN - (*String - L'0')) / 10)) {
-      *Data = MAX_UINTN;
-      if (EndPointer != NULL) {
-        *EndPointer = (CHAR16 *) String;
-      }
-      return RETURN_UNSUPPORTED;
-    }
-
-    *Data = *Data * 10 + (*String - L'0');
-    String++;
-  }
-
-  if (EndPointer != NULL) {
-    *EndPointer = (CHAR16 *) String;
-  }
-  return RETURN_SUCCESS;
-}
-
 /**
   Convert a Null-terminated Unicode decimal string to a value of type UINT64.
 
@@ -1064,9 +998,9 @@ StrDecimalToUint64S (
 
 /**
   Convert a Null-terminated Unicode hexadecimal string to a value of type
-  UINTN.
+  UINT64.
 
-  This function outputs a value of type UINTN by interpreting the contents of
+  This function outputs a value of type UINT64 by interpreting the contents of
   the Unicode string specified by String as a hexadecimal number. The format of
   the input Unicode string String is:
 
@@ -1091,8 +1025,8 @@ StrDecimalToUint64S (
 
   If String has no valid hexadecimal digits in the above format, then 0 is
   stored at the location pointed to by Data.
-  If the number represented by String exceeds the range defined by UINTN, then
-  MAX_UINTN is stored at the location pointed to by Data.
+  If the number represented by String exceeds the range defined by UINT64, then
+  MAX_UINT64 is stored at the location pointed to by Data.
 
   If EndPointer is not NULL, a pointer to the character that stopped the scan
   is stored at the location pointed to by EndPointer. If String has no valid
@@ -1112,86 +1046,10 @@ StrDecimalToUint64S (
                                    characters, not including the
                                    Null-terminator.
   @retval RETURN_UNSUPPORTED       If the number represented by String exceeds
-                                   the range defined by UINTN.
+                                   the range defined by UINT64.
 
 **/
 RETURN_STATUS
-StrHexToUintnS (
-    CONST CHAR16             *String,
-         CHAR16             **EndPointer,  OPTIONAL
-         UINTN              *Data
-  )
-{
-  ASSERT (((UINTN) String & BIT0) == 0);
-
-  //
-  // 1. Neither String nor Data shall be a null pointer.
-  //
-  SAFE_STRING_CONSTRAINT_CHECK ((String != NULL), RETURN_INVALID_PARAMETER);
-  SAFE_STRING_CONSTRAINT_CHECK ((Data != NULL), RETURN_INVALID_PARAMETER);
-
-  //
-  // 2. The length of String shall not be greater than RSIZE_MAX.
-  //
-  if (RSIZE_MAX != 0) {
-    SAFE_STRING_CONSTRAINT_CHECK ((StrnLenS (String, RSIZE_MAX + 1) <= RSIZE_MAX), RETURN_INVALID_PARAMETER);
-  }
-
-  if (EndPointer != NULL) {
-    *EndPointer = (CHAR16 *) String;
-  }
-
-  //
-  // Ignore the pad spaces (space or tab)
-  //
-  while ((*String == L' ') || (*String == L'\t')) {
-    String++;
-  }
-
-  //
-  // Ignore leading Zeros after the spaces
-  //
-  while (*String == L'0') {
-    String++;
-  }
-
-  if (InternalCharToUpper (*String) == L'X') {
-    if (*(String - 1) != L'0') {
-      *Data = 0;
-      return RETURN_SUCCESS;
-    }
-    //
-    // Skip the 'X'
-    //
-    String++;
-  }
-
-  *Data = 0;
-
-  while (InternalIsHexaDecimalDigitCharacter (*String)) {
-    //
-    // If the number represented by String overflows according to the range
-    // defined by UINTN, then MAX_UINTN is stored in *Data and
-    // RETURN_UNSUPPORTED is returned.
-    //
-    if (*Data > ((MAX_UINTN - InternalHexCharToUintn (*String)) >> 4)) {
-      *Data = MAX_UINTN;
-      if (EndPointer != NULL) {
-        *EndPointer = (CHAR16 *) String;
-      }
-      return RETURN_UNSUPPORTED;
-    }
-
-    *Data = (*Data << 4) + InternalHexCharToUintn (*String);
-    String++;
-  }
-
-  if (EndPointer != NULL) {
-    *EndPointer = (CHAR16 *) String;
-  }
-  return RETURN_SUCCESS;
-}
-RETURN_STATUS
 StrHexToUint64S (
     CONST CHAR16             *String,
          CHAR16             **EndPointer,  OPTIONAL
@@ -1291,28 +1149,6 @@ StrHexToUint64 (
   return Result;
 }
 
-UINTN
-StrDecimalToUintn (
-  CONST CHAR16              *String
-  )
-{
-  UINTN     Result;
-
-  StrDecimalToUintnS (String, (CHAR16 **) NULL, &Result);
-  return Result;
-}
-
-UINTN
-StrHexToUintn (
-  CONST CHAR16              *String
-  )
-{
-  UINTN     Result;
-
-  StrHexToUintnS (String, (CHAR16 **) NULL, &Result);
-  return Result;
-}
-
 UINTN
 StrSize (
   CONST CHAR16              *String
-- 
2.19.1

[PATCH v2 6/6] BaseTools/CommonLib: drop definition of MAX_UINTN

[Ard Biesheuvel]

The maximum value that can be represented by the native word size
of the *target* should be irrelevant when compiling tools that
run on the build *host*. So drop the definition of MAX_UINTN, now
that we no longer use it.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
---
 BaseTools/Source/C/Common/CommonLib.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/BaseTools/Source/C/Common/CommonLib.h b/BaseTools/Source/C/Common/CommonLib.h
index 6930d9227b87..b1c6c00a3478 100644
--- a/BaseTools/Source/C/Common/CommonLib.h
+++ b/BaseTools/Source/C/Common/CommonLib.h
@@ -22,7 +22,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 
 #define MAX_LONG_FILE_PATH 500
 
-#define MAX_UINTN MAX_ADDRESS
 #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL)
 #define MAX_UINT16  ((UINT16)0xFFFF)
 #define MAX_UINT8   ((UINT8)0xFF)
-- 
2.19.1

Re: [PATCH v2 1/6] BaseTools/CommonLib: avoid using 'native' word size in IP address handling

[Philippe Mathieu-Daudé]

On 30/11/18 23:45, Ard Biesheuvel wrote:
> In the context of the BaseTools, there is no such thing as a native word
> size, given that the same set of tools may be used to build a firmware
> image consisting of both 32-bit and 64-bit modules.
> 
> So update StrToIpv4Address() and StrToIpv6Address() to use UINT64
> types instead of UINTN types when parsing strings.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  BaseTools/Source/C/Common/CommonLib.c | 28 ++++++++++----------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/BaseTools/Source/C/Common/CommonLib.c b/BaseTools/Source/C/Common/CommonLib.c
> index 618aadac781a..7c559bc3c222 100644
> --- a/BaseTools/Source/C/Common/CommonLib.c
> +++ b/BaseTools/Source/C/Common/CommonLib.c
> @@ -1785,7 +1785,7 @@ StrToIpv4Address (
>  {
>    RETURN_STATUS          Status;
>    UINTN                  AddressIndex;
> -  UINTN                  Uintn;
> +  UINT64                 Uint64;
>    EFI_IPv4_ADDRESS       LocalAddress;
>    UINT8                  LocalPrefixLength;
>    CHAR16                 *Pointer;
> @@ -1812,7 +1812,7 @@ StrToIpv4Address (
>      //
>      // Get D or P.
>      //
> -    Status = StrDecimalToUintnS ((CONST CHAR16 *) Pointer, &Pointer, &Uintn);
> +    Status = StrDecimalToUint64S ((CONST CHAR16 *) Pointer, &Pointer, &Uint64);
>      if (RETURN_ERROR (Status)) {
>        return RETURN_UNSUPPORTED;
>      }
> @@ -1820,18 +1820,18 @@ StrToIpv4Address (
>        //
>        // It's P.
>        //
> -      if (Uintn > 32) {
> +      if (Uint64 > 32) {
>          return RETURN_UNSUPPORTED;
>        }
> -      LocalPrefixLength = (UINT8) Uintn;
> +      LocalPrefixLength = (UINT8) Uint64;
>      } else {
>        //
>        // It's D.
>        //
> -      if (Uintn > MAX_UINT8) {
> +      if (Uint64 > MAX_UINT8) {
>          return RETURN_UNSUPPORTED;
>        }
> -      LocalAddress.Addr[AddressIndex] = (UINT8) Uintn;
> +      LocalAddress.Addr[AddressIndex] = (UINT8) Uint64;
>        AddressIndex++;
>      }
>  
> @@ -1888,7 +1888,7 @@ StrToIpv6Address (
>  {
>    RETURN_STATUS          Status;
>    UINTN                  AddressIndex;
> -  UINTN                  Uintn;
> +  UINT64                 Uint64;
>    EFI_IPv6_ADDRESS       LocalAddress;
>    UINT8                  LocalPrefixLength;
>    CONST CHAR16           *Pointer;
> @@ -1969,7 +1969,7 @@ StrToIpv6Address (
>          //
>          // Get X.
>          //
> -        Status = StrHexToUintnS (Pointer, &End, &Uintn);
> +        Status = StrHexToUint64S (Pointer, &End, &Uint64);
>          if (RETURN_ERROR (Status) || End - Pointer > 4) {
>            //
>            // Number of hexadecimal digit characters is no more than 4.
> @@ -1978,24 +1978,24 @@ StrToIpv6Address (
>          }
>          Pointer = End;
>          //
> -        // Uintn won't exceed MAX_UINT16 if number of hexadecimal digit characters is no more than 4.
> +        // Uint64 won't exceed MAX_UINT16 if number of hexadecimal digit characters is no more than 4.
>          //
>          ASSERT (AddressIndex + 1 < ARRAY_SIZE (Address->Addr));
> -        LocalAddress.Addr[AddressIndex] = (UINT8) ((UINT16) Uintn >> 8);
> -        LocalAddress.Addr[AddressIndex + 1] = (UINT8) Uintn;
> +        LocalAddress.Addr[AddressIndex] = (UINT8) ((UINT16) Uint64 >> 8);
> +        LocalAddress.Addr[AddressIndex + 1] = (UINT8) Uint64;
>          AddressIndex += 2;
>        } else {
>          //
>          // Get P, then exit the loop.
>          //
> -        Status = StrDecimalToUintnS (Pointer, &End, &Uintn);
> -        if (RETURN_ERROR (Status) || End == Pointer || Uintn > 128) {
> +        Status = StrDecimalToUint64S (Pointer, &End, &Uint64);
> +        if (RETURN_ERROR (Status) || End == Pointer || Uint64 > 128) {
>            //
>            // Prefix length should not exceed 128.
>            //
>            return RETURN_UNSUPPORTED;
>          }
> -        LocalPrefixLength = (UINT8) Uintn;
> +        LocalPrefixLength = (UINT8) Uint64;
>          Pointer = End;
>          break;
>        }
> 

Re: [PATCH v2 2/6] BaseTools/CommonLib: use explicit 64-bit type in Strtoi()

[Philippe Mathieu-Daudé]

On 30/11/18 23:45, Ard Biesheuvel wrote:
> Don't use the native word size string to number parsing routines,
> but instead, use the 64-bit one and cast to UINTN.
> 
> Currently, the only user is in Source/C/DevicePath/DevicePathFromText.c
> which takes care to use Strtoi64 () unless it assumes the value fits
> in 32-bit, so this change is a no-op even on 32-bit build hosts.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  BaseTools/Source/C/Common/CommonLib.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/BaseTools/Source/C/Common/CommonLib.c b/BaseTools/Source/C/Common/CommonLib.c
> index 7c559bc3c222..4a28f635f3a8 100644
> --- a/BaseTools/Source/C/Common/CommonLib.c
> +++ b/BaseTools/Source/C/Common/CommonLib.c
> @@ -2252,9 +2252,9 @@ Strtoi (
>    )
>  {
>    if (IsHexStr (Str)) {
> -    return StrHexToUintn (Str);
> +    return (UINTN)StrHexToUint64 (Str);
>    } else {
> -    return StrDecimalToUintn (Str);
> +    return (UINTN)StrDecimalToUint64 (Str);
>    }
>  }
>  
> 

Re: [PATCH v2 3/6] BaseTools/DevicePath: use explicit 64-bit number parsing routines

[Philippe Mathieu-Daudé]

On 30/11/18 23:45, Ard Biesheuvel wrote:
> Replace invocations of StrHexToUintn() with StrHexToUint64(), so
> that we can drop the former.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  BaseTools/Source/C/DevicePath/DevicePathFromText.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/BaseTools/Source/C/DevicePath/DevicePathFromText.c b/BaseTools/Source/C/DevicePath/DevicePathFromText.c
> index 555efa1acdde..6151926af9aa 100644
> --- a/BaseTools/Source/C/DevicePath/DevicePathFromText.c
> +++ b/BaseTools/Source/C/DevicePath/DevicePathFromText.c
> @@ -520,7 +520,7 @@ EisaIdFromText (
>    return (((Text[0] - 'A' + 1) & 0x1f) << 10)
>         + (((Text[1] - 'A' + 1) & 0x1f) <<  5)
>         + (((Text[2] - 'A' + 1) & 0x1f) <<  0)
> -       + (UINT32) (StrHexToUintn (&Text[3]) << 16)
> +       + (UINT32) (StrHexToUint64 (&Text[3]) << 16)
>         ;
>  }
>  
> @@ -1506,7 +1506,7 @@ DevPathFromTextNVMe (
>  
>    Index = sizeof (Nvme->NamespaceUuid) / sizeof (UINT8);
>    while (Index-- != 0) {
> -    Uuid[Index] = (UINT8) StrHexToUintn (SplitStr (&NamespaceUuidStr, L'-'));
> +    Uuid[Index] = (UINT8) StrHexToUint64 (SplitStr (&NamespaceUuidStr, L'-'));
>    }
>  
>    return (EFI_DEVICE_PATH_PROTOCOL *) Nvme;
> 

Re: [PATCH v2 5/6] BaseTools/CommonLib: get rid of 'native' type string parsing routines

[Philippe Mathieu-Daudé]

On 30/11/18 23:45, Ard Biesheuvel wrote:
> Parsing a string into an integer variable of the native word size
> is not defined for the BaseTools, since the same tools may be used
> to build firmware for different targets with different native word
> sizes.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  BaseTools/Source/C/Common/CommonLib.h |  24 ---
>  BaseTools/Source/C/Common/CommonLib.c | 174 +-------------------
>  2 files changed, 5 insertions(+), 193 deletions(-)
> 
> diff --git a/BaseTools/Source/C/Common/CommonLib.h b/BaseTools/Source/C/Common/CommonLib.h
> index fa10fac4682a..6930d9227b87 100644
> --- a/BaseTools/Source/C/Common/CommonLib.h
> +++ b/BaseTools/Source/C/Common/CommonLib.h
> @@ -250,16 +250,6 @@ StrSize (
>    CONST CHAR16              *String
>    );
>  
> -UINTN
> -StrHexToUintn (
> -  CONST CHAR16              *String
> -  );
> -
> -UINTN
> -StrDecimalToUintn (
> -  CONST CHAR16              *String
> -  );
> -
>  UINT64
>  StrHexToUint64 (
>    CONST CHAR16             *String
> @@ -277,13 +267,6 @@ StrHexToUint64S (
>      UINT64             *Data
>    );
>  
> -RETURN_STATUS
> -StrHexToUintnS (
> -    CONST CHAR16             *String,
> -         CHAR16             **EndPointer,  OPTIONAL
> -         UINTN              *Data
> -  );
> -
>  RETURN_STATUS
>  StrDecimalToUint64S (
>      CONST CHAR16             *String,
> @@ -291,13 +274,6 @@ StrDecimalToUint64S (
>           UINT64             *Data
>    );
>  
> -RETURN_STATUS
> -StrDecimalToUintnS (
> -    CONST CHAR16             *String,
> -         CHAR16             **EndPointer,  OPTIONAL
> -         UINTN              *Data
> -  );
> -
>  VOID *
>  ReallocatePool (
>     UINTN  OldSize,
> diff --git a/BaseTools/Source/C/Common/CommonLib.c b/BaseTools/Source/C/Common/CommonLib.c
> index 4a28f635f3a8..42dfa821624d 100644
> --- a/BaseTools/Source/C/Common/CommonLib.c
> +++ b/BaseTools/Source/C/Common/CommonLib.c
> @@ -882,72 +882,6 @@ InternalSafeStringNoStrOverlap (
>    return !InternalSafeStringIsOverlap (Str1, Size1 * sizeof(CHAR16), Str2, Size2 * sizeof(CHAR16));
>  }
>  
> -RETURN_STATUS
> -StrDecimalToUintnS (
> -    CONST CHAR16             *String,
> -         CHAR16             **EndPointer,  OPTIONAL
> -         UINTN              *Data
> -  )
> -{
> -  ASSERT (((UINTN) String & BIT0) == 0);
> -
> -  //
> -  // 1. Neither String nor Data shall be a null pointer.
> -  //
> -  SAFE_STRING_CONSTRAINT_CHECK ((String != NULL), RETURN_INVALID_PARAMETER);
> -  SAFE_STRING_CONSTRAINT_CHECK ((Data != NULL), RETURN_INVALID_PARAMETER);
> -
> -  //
> -  // 2. The length of String shall not be greater than RSIZE_MAX.
> -  //
> -  if (RSIZE_MAX != 0) {
> -    SAFE_STRING_CONSTRAINT_CHECK ((StrnLenS (String, RSIZE_MAX + 1) <= RSIZE_MAX), RETURN_INVALID_PARAMETER);
> -  }
> -
> -  if (EndPointer != NULL) {
> -    *EndPointer = (CHAR16 *) String;
> -  }
> -
> -  //
> -  // Ignore the pad spaces (space or tab)
> -  //
> -  while ((*String == L' ') || (*String == L'\t')) {
> -    String++;
> -  }
> -
> -  //
> -  // Ignore leading Zeros after the spaces
> -  //
> -  while (*String == L'0') {
> -    String++;
> -  }
> -
> -  *Data = 0;
> -
> -  while (InternalIsDecimalDigitCharacter (*String)) {
> -    //
> -    // If the number represented by String overflows according to the range
> -    // defined by UINTN, then MAX_UINTN is stored in *Data and
> -    // RETURN_UNSUPPORTED is returned.
> -    //
> -    if (*Data > ((MAX_UINTN - (*String - L'0')) / 10)) {
> -      *Data = MAX_UINTN;
> -      if (EndPointer != NULL) {
> -        *EndPointer = (CHAR16 *) String;
> -      }
> -      return RETURN_UNSUPPORTED;
> -    }
> -
> -    *Data = *Data * 10 + (*String - L'0');
> -    String++;
> -  }
> -
> -  if (EndPointer != NULL) {
> -    *EndPointer = (CHAR16 *) String;
> -  }
> -  return RETURN_SUCCESS;
> -}
> -
>  /**
>    Convert a Null-terminated Unicode decimal string to a value of type UINT64.
>  
> @@ -1064,9 +998,9 @@ StrDecimalToUint64S (
>  
>  /**
>    Convert a Null-terminated Unicode hexadecimal string to a value of type
> -  UINTN.
> +  UINT64.
>  
> -  This function outputs a value of type UINTN by interpreting the contents of
> +  This function outputs a value of type UINT64 by interpreting the contents of
>    the Unicode string specified by String as a hexadecimal number. The format of
>    the input Unicode string String is:
>  
> @@ -1091,8 +1025,8 @@ StrDecimalToUint64S (
>  
>    If String has no valid hexadecimal digits in the above format, then 0 is
>    stored at the location pointed to by Data.
> -  If the number represented by String exceeds the range defined by UINTN, then
> -  MAX_UINTN is stored at the location pointed to by Data.
> +  If the number represented by String exceeds the range defined by UINT64, then
> +  MAX_UINT64 is stored at the location pointed to by Data.
>  
>    If EndPointer is not NULL, a pointer to the character that stopped the scan
>    is stored at the location pointed to by EndPointer. If String has no valid
> @@ -1112,86 +1046,10 @@ StrDecimalToUint64S (
>                                     characters, not including the
>                                     Null-terminator.
>    @retval RETURN_UNSUPPORTED       If the number represented by String exceeds
> -                                   the range defined by UINTN.
> +                                   the range defined by UINT64.
>  
>  **/
>  RETURN_STATUS
> -StrHexToUintnS (
> -    CONST CHAR16             *String,
> -         CHAR16             **EndPointer,  OPTIONAL
> -         UINTN              *Data
> -  )
> -{
> -  ASSERT (((UINTN) String & BIT0) == 0);
> -
> -  //
> -  // 1. Neither String nor Data shall be a null pointer.
> -  //
> -  SAFE_STRING_CONSTRAINT_CHECK ((String != NULL), RETURN_INVALID_PARAMETER);
> -  SAFE_STRING_CONSTRAINT_CHECK ((Data != NULL), RETURN_INVALID_PARAMETER);
> -
> -  //
> -  // 2. The length of String shall not be greater than RSIZE_MAX.
> -  //
> -  if (RSIZE_MAX != 0) {
> -    SAFE_STRING_CONSTRAINT_CHECK ((StrnLenS (String, RSIZE_MAX + 1) <= RSIZE_MAX), RETURN_INVALID_PARAMETER);
> -  }
> -
> -  if (EndPointer != NULL) {
> -    *EndPointer = (CHAR16 *) String;
> -  }
> -
> -  //
> -  // Ignore the pad spaces (space or tab)
> -  //
> -  while ((*String == L' ') || (*String == L'\t')) {
> -    String++;
> -  }
> -
> -  //
> -  // Ignore leading Zeros after the spaces
> -  //
> -  while (*String == L'0') {
> -    String++;
> -  }
> -
> -  if (InternalCharToUpper (*String) == L'X') {
> -    if (*(String - 1) != L'0') {
> -      *Data = 0;
> -      return RETURN_SUCCESS;
> -    }
> -    //
> -    // Skip the 'X'
> -    //
> -    String++;
> -  }
> -
> -  *Data = 0;
> -
> -  while (InternalIsHexaDecimalDigitCharacter (*String)) {
> -    //
> -    // If the number represented by String overflows according to the range
> -    // defined by UINTN, then MAX_UINTN is stored in *Data and
> -    // RETURN_UNSUPPORTED is returned.
> -    //
> -    if (*Data > ((MAX_UINTN - InternalHexCharToUintn (*String)) >> 4)) {
> -      *Data = MAX_UINTN;
> -      if (EndPointer != NULL) {
> -        *EndPointer = (CHAR16 *) String;
> -      }
> -      return RETURN_UNSUPPORTED;
> -    }
> -
> -    *Data = (*Data << 4) + InternalHexCharToUintn (*String);
> -    String++;
> -  }
> -
> -  if (EndPointer != NULL) {
> -    *EndPointer = (CHAR16 *) String;
> -  }
> -  return RETURN_SUCCESS;
> -}
> -RETURN_STATUS
>  StrHexToUint64S (
>      CONST CHAR16             *String,
>           CHAR16             **EndPointer,  OPTIONAL
> @@ -1291,28 +1149,6 @@ StrHexToUint64 (
>    return Result;
>  }
>  
> -UINTN
> -StrDecimalToUintn (
> -  CONST CHAR16              *String
> -  )
> -{
> -  UINTN     Result;
> -
> -  StrDecimalToUintnS (String, (CHAR16 **) NULL, &Result);
> -  return Result;
> -}
> -
> -UINTN
> -StrHexToUintn (
> -  CONST CHAR16              *String
> -  )
> -{
> -  UINTN     Result;
> -
> -  StrHexToUintnS (String, (CHAR16 **) NULL, &Result);
> -  return Result;
> -}
> -
>  UINTN
>  StrSize (
>    CONST CHAR16              *String
> 

Re: [PATCH v2 6/6] BaseTools/CommonLib: drop definition of MAX_UINTN

[Philippe Mathieu-Daudé]

On 30/11/18 23:45, Ard Biesheuvel wrote:
> The maximum value that can be represented by the native word size
> of the *target* should be irrelevant when compiling tools that
> run on the build *host*. So drop the definition of MAX_UINTN, now
> that we no longer use it.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  BaseTools/Source/C/Common/CommonLib.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/BaseTools/Source/C/Common/CommonLib.h b/BaseTools/Source/C/Common/CommonLib.h
> index 6930d9227b87..b1c6c00a3478 100644
> --- a/BaseTools/Source/C/Common/CommonLib.h
> +++ b/BaseTools/Source/C/Common/CommonLib.h
> @@ -22,7 +22,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  
>  #define MAX_LONG_FILE_PATH 500
>  
> -#define MAX_UINTN MAX_ADDRESS
>  #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL)
>  #define MAX_UINT16  ((UINT16)0xFFFF)
>  #define MAX_UINT8   ((UINT8)0xFF)
> 

Re: [PATCH v2 1/6] BaseTools/CommonLib: avoid using 'native' word size in IP address handling

[Laszlo Ersek]

On 11/30/18 23:45, Ard Biesheuvel wrote:
> In the context of the BaseTools, there is no such thing as a native word
> size, given that the same set of tools may be used to build a firmware
> image consisting of both 32-bit and 64-bit modules.
> 
> So update StrToIpv4Address() and StrToIpv6Address() to use UINT64
> types instead of UINTN types when parsing strings.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  BaseTools/Source/C/Common/CommonLib.c | 28 ++++++++++----------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/BaseTools/Source/C/Common/CommonLib.c b/BaseTools/Source/C/Common/CommonLib.c
> index 618aadac781a..7c559bc3c222 100644
> --- a/BaseTools/Source/C/Common/CommonLib.c
> +++ b/BaseTools/Source/C/Common/CommonLib.c
> @@ -1785,7 +1785,7 @@ StrToIpv4Address (
>  {
>    RETURN_STATUS          Status;
>    UINTN                  AddressIndex;
> -  UINTN                  Uintn;
> +  UINT64                 Uint64;
>    EFI_IPv4_ADDRESS       LocalAddress;
>    UINT8                  LocalPrefixLength;
>    CHAR16                 *Pointer;
> @@ -1812,7 +1812,7 @@ StrToIpv4Address (
>      //
>      // Get D or P.
>      //
> -    Status = StrDecimalToUintnS ((CONST CHAR16 *) Pointer, &Pointer, &Uintn);
> +    Status = StrDecimalToUint64S ((CONST CHAR16 *) Pointer, &Pointer, &Uint64);
>      if (RETURN_ERROR (Status)) {
>        return RETURN_UNSUPPORTED;
>      }
> @@ -1820,18 +1820,18 @@ StrToIpv4Address (
>        //
>        // It's P.
>        //
> -      if (Uintn > 32) {
> +      if (Uint64 > 32) {
>          return RETURN_UNSUPPORTED;
>        }
> -      LocalPrefixLength = (UINT8) Uintn;
> +      LocalPrefixLength = (UINT8) Uint64;
>      } else {
>        //
>        // It's D.
>        //
> -      if (Uintn > MAX_UINT8) {
> +      if (Uint64 > MAX_UINT8) {
>          return RETURN_UNSUPPORTED;
>        }
> -      LocalAddress.Addr[AddressIndex] = (UINT8) Uintn;
> +      LocalAddress.Addr[AddressIndex] = (UINT8) Uint64;
>        AddressIndex++;
>      }
>  
> @@ -1888,7 +1888,7 @@ StrToIpv6Address (
>  {
>    RETURN_STATUS          Status;
>    UINTN                  AddressIndex;
> -  UINTN                  Uintn;
> +  UINT64                 Uint64;
>    EFI_IPv6_ADDRESS       LocalAddress;
>    UINT8                  LocalPrefixLength;
>    CONST CHAR16           *Pointer;
> @@ -1969,7 +1969,7 @@ StrToIpv6Address (
>          //
>          // Get X.
>          //
> -        Status = StrHexToUintnS (Pointer, &End, &Uintn);
> +        Status = StrHexToUint64S (Pointer, &End, &Uint64);
>          if (RETURN_ERROR (Status) || End - Pointer > 4) {
>            //
>            // Number of hexadecimal digit characters is no more than 4.
> @@ -1978,24 +1978,24 @@ StrToIpv6Address (
>          }
>          Pointer = End;
>          //
> -        // Uintn won't exceed MAX_UINT16 if number of hexadecimal digit characters is no more than 4.
> +        // Uint64 won't exceed MAX_UINT16 if number of hexadecimal digit characters is no more than 4.
>          //
>          ASSERT (AddressIndex + 1 < ARRAY_SIZE (Address->Addr));
> -        LocalAddress.Addr[AddressIndex] = (UINT8) ((UINT16) Uintn >> 8);
> -        LocalAddress.Addr[AddressIndex + 1] = (UINT8) Uintn;
> +        LocalAddress.Addr[AddressIndex] = (UINT8) ((UINT16) Uint64 >> 8);
> +        LocalAddress.Addr[AddressIndex + 1] = (UINT8) Uint64;
>          AddressIndex += 2;
>        } else {
>          //
>          // Get P, then exit the loop.
>          //
> -        Status = StrDecimalToUintnS (Pointer, &End, &Uintn);
> -        if (RETURN_ERROR (Status) || End == Pointer || Uintn > 128) {
> +        Status = StrDecimalToUint64S (Pointer, &End, &Uint64);
> +        if (RETURN_ERROR (Status) || End == Pointer || Uint64 > 128) {
>            //
>            // Prefix length should not exceed 128.
>            //
>            return RETURN_UNSUPPORTED;
>          }
> -        LocalPrefixLength = (UINT8) Uintn;
> +        LocalPrefixLength = (UINT8) Uint64;
>          Pointer = End;
>          break;
>        }
> 

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

Re: [PATCH v2 2/6] BaseTools/CommonLib: use explicit 64-bit type in Strtoi()

[Laszlo Ersek]

On 11/30/18 23:45, Ard Biesheuvel wrote:
> Don't use the native word size string to number parsing routines,
> but instead, use the 64-bit one and cast to UINTN.
> 
> Currently, the only user is in Source/C/DevicePath/DevicePathFromText.c
> which takes care to use Strtoi64 () unless it assumes the value fits
> in 32-bit, so this change is a no-op even on 32-bit build hosts.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
> ---
>  BaseTools/Source/C/Common/CommonLib.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/BaseTools/Source/C/Common/CommonLib.c b/BaseTools/Source/C/Common/CommonLib.c
> index 7c559bc3c222..4a28f635f3a8 100644
> --- a/BaseTools/Source/C/Common/CommonLib.c
> +++ b/BaseTools/Source/C/Common/CommonLib.c
> @@ -2252,9 +2252,9 @@ Strtoi (
>    )
>  {
>    if (IsHexStr (Str)) {
> -    return StrHexToUintn (Str);
> +    return (UINTN)StrHexToUint64 (Str);
>    } else {
> -    return StrDecimalToUintn (Str);
> +    return (UINTN)StrDecimalToUint64 (Str);
>    }
>  }
>  
> 

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

Re: [PATCH v2 3/6] BaseTools/DevicePath: use explicit 64-bit number parsing routines

[Laszlo Ersek]

On 11/30/18 23:45, Ard Biesheuvel wrote:
> Replace invocations of StrHexToUintn() with StrHexToUint64(), so
> that we can drop the former.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
> ---
>  BaseTools/Source/C/DevicePath/DevicePathFromText.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/BaseTools/Source/C/DevicePath/DevicePathFromText.c b/BaseTools/Source/C/DevicePath/DevicePathFromText.c
> index 555efa1acdde..6151926af9aa 100644
> --- a/BaseTools/Source/C/DevicePath/DevicePathFromText.c
> +++ b/BaseTools/Source/C/DevicePath/DevicePathFromText.c
> @@ -520,7 +520,7 @@ EisaIdFromText (
>    return (((Text[0] - 'A' + 1) & 0x1f) << 10)
>         + (((Text[1] - 'A' + 1) & 0x1f) <<  5)
>         + (((Text[2] - 'A' + 1) & 0x1f) <<  0)
> -       + (UINT32) (StrHexToUintn (&Text[3]) << 16)
> +       + (UINT32) (StrHexToUint64 (&Text[3]) << 16)
>         ;
>  }
>  

This (theoretically) introduces a bit-shift on a UINT64 value, which
might result in a compiler intrinsic call on 32-bit build hosts. But
that's fine, because this is not firmware code, but hosted code.

> @@ -1506,7 +1506,7 @@ DevPathFromTextNVMe (
>  
>    Index = sizeof (Nvme->NamespaceUuid) / sizeof (UINT8);
>    while (Index-- != 0) {
> -    Uuid[Index] = (UINT8) StrHexToUintn (SplitStr (&NamespaceUuidStr, L'-'));
> +    Uuid[Index] = (UINT8) StrHexToUint64 (SplitStr (&NamespaceUuidStr, L'-'));
>    }
>  
>    return (EFI_DEVICE_PATH_PROTOCOL *) Nvme;
> 

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

Thanks
Laszlo

Re: [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as default device path max size

[Laszlo Ersek]

On 11/30/18 23:45, Ard Biesheuvel wrote:
> Replace the default size limit of IsDevicePathValid() with a value
> that does not depend on the native word size of the build host.
> 
> 64 KB seems sufficient as the upper bound of a device path handled
> by UEFI.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
> ---
>  BaseTools/Source/C/DevicePath/DevicePathUtilities.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
> index d4ec2742b7c8..ba7f83e53070 100644
> --- a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
> +++ b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
> @@ -62,7 +62,7 @@ IsDevicePathValid (
>    ASSERT (DevicePath != NULL);
>  
>    if (MaxSize == 0) {
> -    MaxSize = MAX_UINTN;
> +    MaxSize = MAX_UINT16;
>   }
>  
>    //
> @@ -78,7 +78,7 @@ IsDevicePathValid (
>        return FALSE;
>      }
>  
> -    if (NodeLength > MAX_UINTN - Size) {
> +    if (NodeLength > MAX_UINT16 - Size) {
>        return FALSE;
>      }
>      Size += NodeLength;
> 

I'm somewhat undecided about this patch.

(1) IsDevicePathValid() also exists in:

- MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
- MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLib.c

Both have:

  if (MaxSize == 0) {
    MaxSize = MAX_UINTN;
  }

Relative to those, this change departs quite strongly.


(2) In addition, a single device path node may extend up to 64KB. That
would be pathologic, yes, but the option is there.


... Of course, we are discussing theoretical limits. Still I'd feel more
comfortable with MAX_UINT32. Lifting the limit from 64K to 4G wouldn't
cost us anything (in development effort), it would be a no-op on 32-bit
build hosts, it would be a theoretical-only change on 64-bit build
hosts, and it would leave us with a larger "safety margin".

I won't insist, but I thought I should raise this. (Sorry if this has
been discussed under v1 already.) If you agree, no need to repost (from
my side anyway) just for this.

With or without the update:

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

Thanks
Laszlo

Re: [PATCH v2 5/6] BaseTools/CommonLib: get rid of 'native' type string parsing routines

[Laszlo Ersek]

On 11/30/18 23:45, Ard Biesheuvel wrote:
> Parsing a string into an integer variable of the native word size
> is not defined for the BaseTools, since the same tools may be used
> to build firmware for different targets with different native word
> sizes.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  BaseTools/Source/C/Common/CommonLib.h |  24 ---
>  BaseTools/Source/C/Common/CommonLib.c | 174 +-------------------
>  2 files changed, 5 insertions(+), 193 deletions(-)
> 
> diff --git a/BaseTools/Source/C/Common/CommonLib.h b/BaseTools/Source/C/Common/CommonLib.h
> index fa10fac4682a..6930d9227b87 100644
> --- a/BaseTools/Source/C/Common/CommonLib.h
> +++ b/BaseTools/Source/C/Common/CommonLib.h
> @@ -250,16 +250,6 @@ StrSize (
>    CONST CHAR16              *String
>    );
>  
> -UINTN
> -StrHexToUintn (
> -  CONST CHAR16              *String
> -  );
> -
> -UINTN
> -StrDecimalToUintn (
> -  CONST CHAR16              *String
> -  );
> -
>  UINT64
>  StrHexToUint64 (
>    CONST CHAR16             *String
> @@ -277,13 +267,6 @@ StrHexToUint64S (
>      UINT64             *Data
>    );
>  
> -RETURN_STATUS
> -StrHexToUintnS (
> -    CONST CHAR16             *String,
> -         CHAR16             **EndPointer,  OPTIONAL
> -         UINTN              *Data
> -  );
> -
>  RETURN_STATUS
>  StrDecimalToUint64S (
>      CONST CHAR16             *String,
> @@ -291,13 +274,6 @@ StrDecimalToUint64S (
>           UINT64             *Data
>    );
>  
> -RETURN_STATUS
> -StrDecimalToUintnS (
> -    CONST CHAR16             *String,
> -         CHAR16             **EndPointer,  OPTIONAL
> -         UINTN              *Data
> -  );
> -
>  VOID *
>  ReallocatePool (
>     UINTN  OldSize,
> diff --git a/BaseTools/Source/C/Common/CommonLib.c b/BaseTools/Source/C/Common/CommonLib.c
> index 4a28f635f3a8..42dfa821624d 100644
> --- a/BaseTools/Source/C/Common/CommonLib.c
> +++ b/BaseTools/Source/C/Common/CommonLib.c
> @@ -882,72 +882,6 @@ InternalSafeStringNoStrOverlap (
>    return !InternalSafeStringIsOverlap (Str1, Size1 * sizeof(CHAR16), Str2, Size2 * sizeof(CHAR16));
>  }
>  
> -RETURN_STATUS
> -StrDecimalToUintnS (
> -    CONST CHAR16             *String,
> -         CHAR16             **EndPointer,  OPTIONAL
> -         UINTN              *Data
> -  )
> -{
> -  ASSERT (((UINTN) String & BIT0) == 0);
> -
> -  //
> -  // 1. Neither String nor Data shall be a null pointer.
> -  //
> -  SAFE_STRING_CONSTRAINT_CHECK ((String != NULL), RETURN_INVALID_PARAMETER);
> -  SAFE_STRING_CONSTRAINT_CHECK ((Data != NULL), RETURN_INVALID_PARAMETER);
> -
> -  //
> -  // 2. The length of String shall not be greater than RSIZE_MAX.
> -  //
> -  if (RSIZE_MAX != 0) {
> -    SAFE_STRING_CONSTRAINT_CHECK ((StrnLenS (String, RSIZE_MAX + 1) <= RSIZE_MAX), RETURN_INVALID_PARAMETER);
> -  }
> -
> -  if (EndPointer != NULL) {
> -    *EndPointer = (CHAR16 *) String;
> -  }
> -
> -  //
> -  // Ignore the pad spaces (space or tab)
> -  //
> -  while ((*String == L' ') || (*String == L'\t')) {
> -    String++;
> -  }
> -
> -  //
> -  // Ignore leading Zeros after the spaces
> -  //
> -  while (*String == L'0') {
> -    String++;
> -  }
> -
> -  *Data = 0;
> -
> -  while (InternalIsDecimalDigitCharacter (*String)) {
> -    //
> -    // If the number represented by String overflows according to the range
> -    // defined by UINTN, then MAX_UINTN is stored in *Data and
> -    // RETURN_UNSUPPORTED is returned.
> -    //
> -    if (*Data > ((MAX_UINTN - (*String - L'0')) / 10)) {
> -      *Data = MAX_UINTN;
> -      if (EndPointer != NULL) {
> -        *EndPointer = (CHAR16 *) String;
> -      }
> -      return RETURN_UNSUPPORTED;
> -    }
> -
> -    *Data = *Data * 10 + (*String - L'0');
> -    String++;
> -  }
> -
> -  if (EndPointer != NULL) {
> -    *EndPointer = (CHAR16 *) String;
> -  }
> -  return RETURN_SUCCESS;
> -}
> -
>  /**
>    Convert a Null-terminated Unicode decimal string to a value of type UINT64.
>  
> @@ -1064,9 +998,9 @@ StrDecimalToUint64S (
>  
>  /**
>    Convert a Null-terminated Unicode hexadecimal string to a value of type
> -  UINTN.
> +  UINT64.
>  
> -  This function outputs a value of type UINTN by interpreting the contents of
> +  This function outputs a value of type UINT64 by interpreting the contents of
>    the Unicode string specified by String as a hexadecimal number. The format of
>    the input Unicode string String is:
>  
> @@ -1091,8 +1025,8 @@ StrDecimalToUint64S (
>  
>    If String has no valid hexadecimal digits in the above format, then 0 is
>    stored at the location pointed to by Data.
> -  If the number represented by String exceeds the range defined by UINTN, then
> -  MAX_UINTN is stored at the location pointed to by Data.
> +  If the number represented by String exceeds the range defined by UINT64, then
> +  MAX_UINT64 is stored at the location pointed to by Data.
>  
>    If EndPointer is not NULL, a pointer to the character that stopped the scan
>    is stored at the location pointed to by EndPointer. If String has no valid
> @@ -1112,86 +1046,10 @@ StrDecimalToUint64S (
>                                     characters, not including the
>                                     Null-terminator.
>    @retval RETURN_UNSUPPORTED       If the number represented by String exceeds
> -                                   the range defined by UINTN.
> +                                   the range defined by UINT64.
>  
>  **/
>  RETURN_STATUS
> -StrHexToUintnS (
> -    CONST CHAR16             *String,
> -         CHAR16             **EndPointer,  OPTIONAL
> -         UINTN              *Data
> -  )
> -{
> -  ASSERT (((UINTN) String & BIT0) == 0);
> -
> -  //
> -  // 1. Neither String nor Data shall be a null pointer.
> -  //
> -  SAFE_STRING_CONSTRAINT_CHECK ((String != NULL), RETURN_INVALID_PARAMETER);
> -  SAFE_STRING_CONSTRAINT_CHECK ((Data != NULL), RETURN_INVALID_PARAMETER);
> -
> -  //
> -  // 2. The length of String shall not be greater than RSIZE_MAX.
> -  //
> -  if (RSIZE_MAX != 0) {
> -    SAFE_STRING_CONSTRAINT_CHECK ((StrnLenS (String, RSIZE_MAX + 1) <= RSIZE_MAX), RETURN_INVALID_PARAMETER);
> -  }
> -
> -  if (EndPointer != NULL) {
> -    *EndPointer = (CHAR16 *) String;
> -  }
> -
> -  //
> -  // Ignore the pad spaces (space or tab)
> -  //
> -  while ((*String == L' ') || (*String == L'\t')) {
> -    String++;
> -  }
> -
> -  //
> -  // Ignore leading Zeros after the spaces
> -  //
> -  while (*String == L'0') {
> -    String++;
> -  }
> -
> -  if (InternalCharToUpper (*String) == L'X') {
> -    if (*(String - 1) != L'0') {
> -      *Data = 0;
> -      return RETURN_SUCCESS;
> -    }
> -    //
> -    // Skip the 'X'
> -    //
> -    String++;
> -  }
> -
> -  *Data = 0;
> -
> -  while (InternalIsHexaDecimalDigitCharacter (*String)) {
> -    //
> -    // If the number represented by String overflows according to the range
> -    // defined by UINTN, then MAX_UINTN is stored in *Data and
> -    // RETURN_UNSUPPORTED is returned.
> -    //
> -    if (*Data > ((MAX_UINTN - InternalHexCharToUintn (*String)) >> 4)) {
> -      *Data = MAX_UINTN;
> -      if (EndPointer != NULL) {
> -        *EndPointer = (CHAR16 *) String;
> -      }
> -      return RETURN_UNSUPPORTED;
> -    }
> -
> -    *Data = (*Data << 4) + InternalHexCharToUintn (*String);
> -    String++;
> -  }
> -
> -  if (EndPointer != NULL) {
> -    *EndPointer = (CHAR16 *) String;
> -  }
> -  return RETURN_SUCCESS;
> -}
> -RETURN_STATUS
>  StrHexToUint64S (
>      CONST CHAR16             *String,
>           CHAR16             **EndPointer,  OPTIONAL
> @@ -1291,28 +1149,6 @@ StrHexToUint64 (
>    return Result;
>  }
>  
> -UINTN
> -StrDecimalToUintn (
> -  CONST CHAR16              *String
> -  )
> -{
> -  UINTN     Result;
> -
> -  StrDecimalToUintnS (String, (CHAR16 **) NULL, &Result);
> -  return Result;
> -}
> -
> -UINTN
> -StrHexToUintn (
> -  CONST CHAR16              *String
> -  )
> -{
> -  UINTN     Result;
> -
> -  StrHexToUintnS (String, (CHAR16 **) NULL, &Result);
> -  return Result;
> -}
> -
>  UINTN
>  StrSize (
>    CONST CHAR16              *String
> 

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

Re: [PATCH v2 6/6] BaseTools/CommonLib: drop definition of MAX_UINTN

[Laszlo Ersek]

On 11/30/18 23:45, Ard Biesheuvel wrote:
> The maximum value that can be represented by the native word size
> of the *target* should be irrelevant when compiling tools that
> run on the build *host*. So drop the definition of MAX_UINTN, now
> that we no longer use it.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
> ---
>  BaseTools/Source/C/Common/CommonLib.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/BaseTools/Source/C/Common/CommonLib.h b/BaseTools/Source/C/Common/CommonLib.h
> index 6930d9227b87..b1c6c00a3478 100644
> --- a/BaseTools/Source/C/Common/CommonLib.h
> +++ b/BaseTools/Source/C/Common/CommonLib.h
> @@ -22,7 +22,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  
>  #define MAX_LONG_FILE_PATH 500
>  
> -#define MAX_UINTN MAX_ADDRESS
>  #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL)
>  #define MAX_UINT16  ((UINT16)0xFFFF)
>  #define MAX_UINT8   ((UINT8)0xFF)
> 

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

Re: [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as default device path max size

[Gao, Liming]

Laszlo:
  I agree with you. MAX_UINT32 is more comfortable. 

Thanks
Liming
>-----Original Message-----
>From: Laszlo Ersek [mailto:lersek@redhat.com]
>Sent: Monday, December 03, 2018 9:06 PM
>To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org
>Cc: Zhu, Yonghong <yonghong.zhu@intel.com>; Gao, Liming
><liming.gao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>; Carsey, Jaben
><jaben.carsey@intel.com>
>Subject: Re: [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as
>default device path max size
>
>On 11/30/18 23:45, Ard Biesheuvel wrote:
>> Replace the default size limit of IsDevicePathValid() with a value
>> that does not depend on the native word size of the build host.
>>
>> 64 KB seems sufficient as the upper bound of a device path handled
>> by UEFI.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
>> ---
>>  BaseTools/Source/C/DevicePath/DevicePathUtilities.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
>b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
>> index d4ec2742b7c8..ba7f83e53070 100644
>> --- a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
>> +++ b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
>> @@ -62,7 +62,7 @@ IsDevicePathValid (
>>    ASSERT (DevicePath != NULL);
>>
>>    if (MaxSize == 0) {
>> -    MaxSize = MAX_UINTN;
>> +    MaxSize = MAX_UINT16;
>>   }
>>
>>    //
>> @@ -78,7 +78,7 @@ IsDevicePathValid (
>>        return FALSE;
>>      }
>>
>> -    if (NodeLength > MAX_UINTN - Size) {
>> +    if (NodeLength > MAX_UINT16 - Size) {
>>        return FALSE;
>>      }
>>      Size += NodeLength;
>>
>
>I'm somewhat undecided about this patch.
>
>(1) IsDevicePathValid() also exists in:
>
>- MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
>- MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLib.c
>
>Both have:
>
>  if (MaxSize == 0) {
>    MaxSize = MAX_UINTN;
>  }
>
>Relative to those, this change departs quite strongly.
>
>
>(2) In addition, a single device path node may extend up to 64KB. That
>would be pathologic, yes, but the option is there.
>
>
>... Of course, we are discussing theoretical limits. Still I'd feel more
>comfortable with MAX_UINT32. Lifting the limit from 64K to 4G wouldn't
>cost us anything (in development effort), it would be a no-op on 32-bit
>build hosts, it would be a theoretical-only change on 64-bit build
>hosts, and it would leave us with a larger "safety margin".
>
>I won't insist, but I thought I should raise this. (Sorry if this has
>been discussed under v1 already.) If you agree, no need to repost (from
>my side anyway) just for this.
>
>With or without the update:
>
>Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
>Thanks
>Laszlo

Re: [PATCH v2 0/6] BaseTools: get rid of MAX_UINTN

[Gao, Liming]

Reviewed-by: Liming Gao <liming.gao@intel.com> for this serials. 

On patch 4, I have the same comments to Laszlo. 

>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
>Biesheuvel
>Sent: Saturday, December 01, 2018 6:46 AM
>To: edk2-devel@lists.01.org
>Cc: Gao, Liming <liming.gao@intel.com>; Carsey, Jaben
><jaben.carsey@intel.com>; Laszlo Ersek <lersek@redhat.com>
>Subject: [edk2] [PATCH v2 0/6] BaseTools: get rid of MAX_UINTN
>
>There should be no reason for the build tools to care about the native
>word size of a particular target, so relying on a definition of MAX_UINTN
>is definitely wrong, and most likely inaccurate on 32-bit build hosts.
>
>So refactor the code in CommonLib and DevicePath so we no longer rely
>on this definition.
>
>Changes since v1:
>- miss type change in #1 causing a build failure on MSVC
>- add acks from Jaben
>
>Cc: Laszlo Ersek <lersek@redhat.com>
>Cc: Yonghong Zhu <yonghong.zhu@intel.com>
>Cc: Liming Gao <liming.gao@intel.com>
>Cc: Bob Feng <bob.c.feng@intel.com>
>Cc: Jaben Carsey <jaben.carsey@intel.com>
>
>Ard Biesheuvel (6):
>  BaseTools/CommonLib: avoid using 'native' word size in IP address
>    handling
>  BaseTools/CommonLib: use explicit 64-bit type in Strtoi()
>  BaseTools/DevicePath: use explicit 64-bit number parsing routines
>  BaseTools/DevicePath: use MAX_UINT16 as default device path max size
>  BaseTools/CommonLib: get rid of 'native' type string parsing routines
>  BaseTools/CommonLib: drop definition of MAX_UINTN
>
> BaseTools/Source/C/Common/CommonLib.h         |  25 ---
> BaseTools/Source/C/Common/CommonLib.c         | 206 ++----------------
> .../Source/C/DevicePath/DevicePathFromText.c  |   4 +-
> .../Source/C/DevicePath/DevicePathUtilities.c |   4 +-
> 4 files changed, 25 insertions(+), 214 deletions(-)
>
>--
>2.19.1
>
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as default device path max size

[Ard Biesheuvel]

On Wed, 5 Dec 2018 at 01:04, Gao, Liming <liming.gao@intel.com> wrote:
>
> Laszlo:
>   I agree with you. MAX_UINT32 is more comfortable.
>

Liming,

No definitions for MAX_UINT32 exist currently in BaseTools, so I will
have to add the following:

diff --git a/BaseTools/Source/C/Common/CommonLib.h
b/BaseTools/Source/C/Common/CommonLib.h
index b1c6c00a3478..1c40180329c4 100644
--- a/BaseTools/Source/C/Common/CommonLib.h
+++ b/BaseTools/Source/C/Common/CommonLib.h
@@ -23,6 +23,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
EITHER EXPRESS OR IMPLIED.
 #define MAX_LONG_FILE_PATH 500

 #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL)
+#define MAX_UINT32 ((UINT32)0xFFFFFFFFULL)
 #define MAX_UINT16  ((UINT16)0xFFFF)
 #define MAX_UINT8   ((UINT8)0xFF)
 #define ARRAY_SIZE(Array) (sizeof (Array) / sizeof ((Array)[0]))

Does your Reviewed-by cover that as well?




> >-----Original Message-----
> >From: Laszlo Ersek [mailto:lersek@redhat.com]
> >Sent: Monday, December 03, 2018 9:06 PM
> >To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org
> >Cc: Zhu, Yonghong <yonghong.zhu@intel.com>; Gao, Liming
> ><liming.gao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>; Carsey, Jaben
> ><jaben.carsey@intel.com>
> >Subject: Re: [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as
> >default device path max size
> >
> >On 11/30/18 23:45, Ard Biesheuvel wrote:
> >> Replace the default size limit of IsDevicePathValid() with a value
> >> that does not depend on the native word size of the build host.
> >>
> >> 64 KB seems sufficient as the upper bound of a device path handled
> >> by UEFI.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
> >> ---
> >>  BaseTools/Source/C/DevicePath/DevicePathUtilities.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
> >b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
> >> index d4ec2742b7c8..ba7f83e53070 100644
> >> --- a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
> >> +++ b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
> >> @@ -62,7 +62,7 @@ IsDevicePathValid (
> >>    ASSERT (DevicePath != NULL);
> >>
> >>    if (MaxSize == 0) {
> >> -    MaxSize = MAX_UINTN;
> >> +    MaxSize = MAX_UINT16;
> >>   }
> >>
> >>    //
> >> @@ -78,7 +78,7 @@ IsDevicePathValid (
> >>        return FALSE;
> >>      }
> >>
> >> -    if (NodeLength > MAX_UINTN - Size) {
> >> +    if (NodeLength > MAX_UINT16 - Size) {
> >>        return FALSE;
> >>      }
> >>      Size += NodeLength;
> >>
> >
> >I'm somewhat undecided about this patch.
> >
> >(1) IsDevicePathValid() also exists in:
> >
> >- MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
> >- MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLib.c
> >
> >Both have:
> >
> >  if (MaxSize == 0) {
> >    MaxSize = MAX_UINTN;
> >  }
> >
> >Relative to those, this change departs quite strongly.
> >
> >
> >(2) In addition, a single device path node may extend up to 64KB. That
> >would be pathologic, yes, but the option is there.
> >
> >
> >... Of course, we are discussing theoretical limits. Still I'd feel more
> >comfortable with MAX_UINT32. Lifting the limit from 64K to 4G wouldn't
> >cost us anything (in development effort), it would be a no-op on 32-bit
> >build hosts, it would be a theoretical-only change on 64-bit build
> >hosts, and it would leave us with a larger "safety margin".
> >
> >I won't insist, but I thought I should raise this. (Sorry if this has
> >been discussed under v1 already.) If you agree, no need to repost (from
> >my side anyway) just for this.
> >
> >With or without the update:
> >
> >Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> >
> >Thanks
> >Laszlo

Re: [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as default device path max size

[Gao, Liming]

Yes. The change is necessary. 

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Wednesday, December 5, 2018 3:42 PM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org; Zhu, Yonghong <yonghong.zhu@intel.com>; Feng, Bob C
> <bob.c.feng@intel.com>; Carsey, Jaben <jaben.carsey@intel.com>
> Subject: Re: [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as default device path max size
> 
> On Wed, 5 Dec 2018 at 01:04, Gao, Liming <liming.gao@intel.com> wrote:
> >
> > Laszlo:
> >   I agree with you. MAX_UINT32 is more comfortable.
> >
> 
> Liming,
> 
> No definitions for MAX_UINT32 exist currently in BaseTools, so I will
> have to add the following:
> 
> diff --git a/BaseTools/Source/C/Common/CommonLib.h
> b/BaseTools/Source/C/Common/CommonLib.h
> index b1c6c00a3478..1c40180329c4 100644
> --- a/BaseTools/Source/C/Common/CommonLib.h
> +++ b/BaseTools/Source/C/Common/CommonLib.h
> @@ -23,6 +23,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
>  #define MAX_LONG_FILE_PATH 500
> 
>  #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL)
> +#define MAX_UINT32 ((UINT32)0xFFFFFFFFULL)
>  #define MAX_UINT16  ((UINT16)0xFFFF)
>  #define MAX_UINT8   ((UINT8)0xFF)
>  #define ARRAY_SIZE(Array) (sizeof (Array) / sizeof ((Array)[0]))
> 
> Does your Reviewed-by cover that as well?
> 
> 
> 
> 
> > >-----Original Message-----
> > >From: Laszlo Ersek [mailto:lersek@redhat.com]
> > >Sent: Monday, December 03, 2018 9:06 PM
> > >To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org
> > >Cc: Zhu, Yonghong <yonghong.zhu@intel.com>; Gao, Liming
> > ><liming.gao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>; Carsey, Jaben
> > ><jaben.carsey@intel.com>
> > >Subject: Re: [PATCH v2 4/6] BaseTools/DevicePath: use MAX_UINT16 as
> > >default device path max size
> > >
> > >On 11/30/18 23:45, Ard Biesheuvel wrote:
> > >> Replace the default size limit of IsDevicePathValid() with a value
> > >> that does not depend on the native word size of the build host.
> > >>
> > >> 64 KB seems sufficient as the upper bound of a device path handled
> > >> by UEFI.
> > >>
> > >> Contributed-under: TianoCore Contribution Agreement 1.1
> > >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > >> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
> > >> ---
> > >>  BaseTools/Source/C/DevicePath/DevicePathUtilities.c | 4 ++--
> > >>  1 file changed, 2 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
> > >b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
> > >> index d4ec2742b7c8..ba7f83e53070 100644
> > >> --- a/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
> > >> +++ b/BaseTools/Source/C/DevicePath/DevicePathUtilities.c
> > >> @@ -62,7 +62,7 @@ IsDevicePathValid (
> > >>    ASSERT (DevicePath != NULL);
> > >>
> > >>    if (MaxSize == 0) {
> > >> -    MaxSize = MAX_UINTN;
> > >> +    MaxSize = MAX_UINT16;
> > >>   }
> > >>
> > >>    //
> > >> @@ -78,7 +78,7 @@ IsDevicePathValid (
> > >>        return FALSE;
> > >>      }
> > >>
> > >> -    if (NodeLength > MAX_UINTN - Size) {
> > >> +    if (NodeLength > MAX_UINT16 - Size) {
> > >>        return FALSE;
> > >>      }
> > >>      Size += NodeLength;
> > >>
> > >
> > >I'm somewhat undecided about this patch.
> > >
> > >(1) IsDevicePathValid() also exists in:
> > >
> > >- MdePkg/Library/UefiDevicePathLib/DevicePathUtilities.c
> > >- MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLib.c
> > >
> > >Both have:
> > >
> > >  if (MaxSize == 0) {
> > >    MaxSize = MAX_UINTN;
> > >  }
> > >
> > >Relative to those, this change departs quite strongly.
> > >
> > >
> > >(2) In addition, a single device path node may extend up to 64KB. That
> > >would be pathologic, yes, but the option is there.
> > >
> > >
> > >... Of course, we are discussing theoretical limits. Still I'd feel more
> > >comfortable with MAX_UINT32. Lifting the limit from 64K to 4G wouldn't
> > >cost us anything (in development effort), it would be a no-op on 32-bit
> > >build hosts, it would be a theoretical-only change on 64-bit build
> > >hosts, and it would leave us with a larger "safety margin".
> > >
> > >I won't insist, but I thought I should raise this. (Sorry if this has
> > >been discussed under v1 already.) If you agree, no need to repost (from
> > >my side anyway) just for this.
> > >
> > >With or without the update:
> > >
> > >Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> > >
> > >Thanks
> > >Laszlo

Re: [PATCH v2 0/6] BaseTools: get rid of MAX_UINTN

[Ard Biesheuvel]

On Wed, 5 Dec 2018 at 01:04, Gao, Liming <liming.gao@intel.com> wrote:
>
> Reviewed-by: Liming Gao <liming.gao@intel.com> for this serials.
>

Thanks all

Series pushed as 64ab2c82e8f6..8efc6d84ca41

(with the requested MAX_UINT16 -> MAX_UINT32 change applied)

> On patch 4, I have the same comments to Laszlo.
>
> >-----Original Message-----
> >From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
> >Biesheuvel
> >Sent: Saturday, December 01, 2018 6:46 AM
> >To: edk2-devel@lists.01.org
> >Cc: Gao, Liming <liming.gao@intel.com>; Carsey, Jaben
> ><jaben.carsey@intel.com>; Laszlo Ersek <lersek@redhat.com>
> >Subject: [edk2] [PATCH v2 0/6] BaseTools: get rid of MAX_UINTN
> >
> >There should be no reason for the build tools to care about the native
> >word size of a particular target, so relying on a definition of MAX_UINTN
> >is definitely wrong, and most likely inaccurate on 32-bit build hosts.
> >
> >So refactor the code in CommonLib and DevicePath so we no longer rely
> >on this definition.
> >
> >Changes since v1:
> >- miss type change in #1 causing a build failure on MSVC
> >- add acks from Jaben
> >
> >Cc: Laszlo Ersek <lersek@redhat.com>
> >Cc: Yonghong Zhu <yonghong.zhu@intel.com>
> >Cc: Liming Gao <liming.gao@intel.com>
> >Cc: Bob Feng <bob.c.feng@intel.com>
> >Cc: Jaben Carsey <jaben.carsey@intel.com>
> >
> >Ard Biesheuvel (6):
> >  BaseTools/CommonLib: avoid using 'native' word size in IP address
> >    handling
> >  BaseTools/CommonLib: use explicit 64-bit type in Strtoi()
> >  BaseTools/DevicePath: use explicit 64-bit number parsing routines
> >  BaseTools/DevicePath: use MAX_UINT16 as default device path max size
> >  BaseTools/CommonLib: get rid of 'native' type string parsing routines
> >  BaseTools/CommonLib: drop definition of MAX_UINTN
> >
> > BaseTools/Source/C/Common/CommonLib.h         |  25 ---
> > BaseTools/Source/C/Common/CommonLib.c         | 206 ++----------------
> > .../Source/C/DevicePath/DevicePathFromText.c  |   4 +-
> > .../Source/C/DevicePath/DevicePathUtilities.c |   4 +-
> > 4 files changed, 25 insertions(+), 214 deletions(-)
> >
> >--
> >2.19.1
> >
> >_______________________________________________
> >edk2-devel mailing list
> >edk2-devel@lists.01.org
> >https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v2 6/6] BaseTools/CommonLib: drop definition of MAX_UINTN

[David F.]

Not sure why you'd take that out when someone using UINTN for variables may
want to use MAX_UINTN ?    Future may be different.

On Mon, Dec 3, 2018 at 5:08 AM Laszlo Ersek <lersek@redhat.com> wrote:

> On 11/30/18 23:45, Ard Biesheuvel wrote:
> > The maximum value that can be represented by the native word size
> > of the *target* should be irrelevant when compiling tools that
> > run on the build *host*. So drop the definition of MAX_UINTN, now
> > that we no longer use it.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
> > ---
> >  BaseTools/Source/C/Common/CommonLib.h | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/BaseTools/Source/C/Common/CommonLib.h
> b/BaseTools/Source/C/Common/CommonLib.h
> > index 6930d9227b87..b1c6c00a3478 100644
> > --- a/BaseTools/Source/C/Common/CommonLib.h
> > +++ b/BaseTools/Source/C/Common/CommonLib.h
> > @@ -22,7 +22,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
> >
> >  #define MAX_LONG_FILE_PATH 500
> >
> > -#define MAX_UINTN MAX_ADDRESS
> >  #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL)
> >  #define MAX_UINT16  ((UINT16)0xFFFF)
> >  #define MAX_UINT8   ((UINT8)0xFF)
> >
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>

Re: [PATCH v2 6/6] BaseTools/CommonLib: drop definition of MAX_UINTN

[Laszlo Ersek]

On 12/11/18 08:11, David F. wrote:
> Not sure why you'd take that out when someone using UINTN for variables may
> want to use MAX_UINTN ?    Future may be different.

The UINTN type comes from the UEFI spec:

    Unsigned value of native width. (4 bytes on supported 32-bit
    processor instructions, 8 bytes on supported 64-bit processor
    instructions, 16 bytes on supported 128-bit processor instructions)

In this sense, "native" refers to the firmware execution environment.
The firmware execution environment need not have anything in common with
the build environment. (You can build 32-bit ARM firmware on X64 hosts.)
In such a scenario, using UINTN *at all* is fraught with
misunderstandings. It *would* be possible to use UINTN as it applies to
the build (= hosted) environment, and in that sense MAX_UINTN would also
be possible to define. However, the code being removed (= defining
MAX_UINTN as MAX_ADDRESS) proves that that approach would be very easy
to misunderstand and misuse. People could easily mistake it for applying
to the firmware execution environment.

UINT32 and UINT64 are not affected by this ambiguity.

Optimally, given that the build utilities target a hosted C runtime,
they should use standard C types, such as "unsigned int", or e.g.
"uint32_t". Together with standard C macros expressing limits, such as
UINT_MAX (from <limits.h>) and UINT32_MAX (from <stdint.h>).

Clearly no-one has capacity to clean up BaseTools like this. For
starters, we should at least remove whatever actively causes confusion.

Thanks,
Laszlo

> On Mon, Dec 3, 2018 at 5:08 AM Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 11/30/18 23:45, Ard Biesheuvel wrote:
>>> The maximum value that can be represented by the native word size
>>> of the *target* should be irrelevant when compiling tools that
>>> run on the build *host*. So drop the definition of MAX_UINTN, now
>>> that we no longer use it.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
>>> ---
>>>  BaseTools/Source/C/Common/CommonLib.h | 1 -
>>>  1 file changed, 1 deletion(-)
>>>
>>> diff --git a/BaseTools/Source/C/Common/CommonLib.h
>> b/BaseTools/Source/C/Common/CommonLib.h
>>> index 6930d9227b87..b1c6c00a3478 100644
>>> --- a/BaseTools/Source/C/Common/CommonLib.h
>>> +++ b/BaseTools/Source/C/Common/CommonLib.h
>>> @@ -22,7 +22,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
>> EITHER EXPRESS OR IMPLIED.
>>>
>>>  #define MAX_LONG_FILE_PATH 500
>>>
>>> -#define MAX_UINTN MAX_ADDRESS
>>>  #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL)
>>>  #define MAX_UINT16  ((UINT16)0xFFFF)
>>>  #define MAX_UINT8   ((UINT8)0xFF)
>>>
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
> 

Re: [PATCH v2 6/6] BaseTools/CommonLib: drop definition of MAX_UINTN

[David F.]

I don't know, to me it's very clear that UINTN is talking about the target,
just like size_t would be.

There are/were a bunch of API's using UINTN so using UINTN was desirable,
and where needed UINTN_MAX.

I just don't see an advantage to removing it.   Do see disadvantage to
removing it for breaking existing code and for those that want the "native"
(best/fasted/most efficient) int size for the processor (similar again to
size_t)

On Tue, Dec 11, 2018 at 7:46 AM Laszlo Ersek <lersek@redhat.com> wrote:

> On 12/11/18 08:11, David F. wrote:
> > Not sure why you'd take that out when someone using UINTN for variables
> may
> > want to use MAX_UINTN ?    Future may be different.
>
> The UINTN type comes from the UEFI spec:
>
>     Unsigned value of native width. (4 bytes on supported 32-bit
>     processor instructions, 8 bytes on supported 64-bit processor
>     instructions, 16 bytes on supported 128-bit processor instructions)
>
> In this sense, "native" refers to the firmware execution environment.
> The firmware execution environment need not have anything in common with
> the build environment. (You can build 32-bit ARM firmware on X64 hosts.)
> In such a scenario, using UINTN *at all* is fraught with
> misunderstandings. It *would* be possible to use UINTN as it applies to
> the build (= hosted) environment, and in that sense MAX_UINTN would also
> be possible to define. However, the code being removed (= defining
> MAX_UINTN as MAX_ADDRESS) proves that that approach would be very easy
> to misunderstand and misuse. People could easily mistake it for applying
> to the firmware execution environment.
>
> UINT32 and UINT64 are not affected by this ambiguity.
>
> Optimally, given that the build utilities target a hosted C runtime,
> they should use standard C types, such as "unsigned int", or e.g.
> "uint32_t". Together with standard C macros expressing limits, such as
> UINT_MAX (from <limits.h>) and UINT32_MAX (from <stdint.h>).
>
> Clearly no-one has capacity to clean up BaseTools like this. For
> starters, we should at least remove whatever actively causes confusion.
>
> Thanks,
> Laszlo
>
> > On Mon, Dec 3, 2018 at 5:08 AM Laszlo Ersek <lersek@redhat.com> wrote:
> >
> >> On 11/30/18 23:45, Ard Biesheuvel wrote:
> >>> The maximum value that can be represented by the native word size
> >>> of the *target* should be irrelevant when compiling tools that
> >>> run on the build *host*. So drop the definition of MAX_UINTN, now
> >>> that we no longer use it.
> >>>
> >>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
> >>> ---
> >>>  BaseTools/Source/C/Common/CommonLib.h | 1 -
> >>>  1 file changed, 1 deletion(-)
> >>>
> >>> diff --git a/BaseTools/Source/C/Common/CommonLib.h
> >> b/BaseTools/Source/C/Common/CommonLib.h
> >>> index 6930d9227b87..b1c6c00a3478 100644
> >>> --- a/BaseTools/Source/C/Common/CommonLib.h
> >>> +++ b/BaseTools/Source/C/Common/CommonLib.h
> >>> @@ -22,7 +22,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> >> EITHER EXPRESS OR IMPLIED.
> >>>
> >>>  #define MAX_LONG_FILE_PATH 500
> >>>
> >>> -#define MAX_UINTN MAX_ADDRESS
> >>>  #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL)
> >>>  #define MAX_UINT16  ((UINT16)0xFFFF)
> >>>  #define MAX_UINT8   ((UINT8)0xFF)
> >>>
> >>
> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel
> >>
> >
>
>

Re: [PATCH v2 6/6] BaseTools/CommonLib: drop definition of MAX_UINTN

[Ard Biesheuvel]

On Tue, 11 Dec 2018 at 23:53, David F. <df7729@gmail.com> wrote:
>
> I don't know, to me it's very clear that UINTN is talking about the target, just like size_t would be.
>

But which target? This change is against the source of the BaseTools,
which are host tools that can be used to build a single target image
consisting of 32-bit and 64-bit modules. So which size is the native
size in this case?

> There are/were a bunch of API's using UINTN so using UINTN was desirable, and where needed UINTN_MAX.
>
> I just don't see an advantage to removing it.   Do see disadvantage to removing it for breaking existing code and for those that want the "native" (best/fasted/most efficient) int size for the processor (similar again to size_t)
>
> On Tue, Dec 11, 2018 at 7:46 AM Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 12/11/18 08:11, David F. wrote:
>> > Not sure why you'd take that out when someone using UINTN for variables may
>> > want to use MAX_UINTN ?    Future may be different.
>>
>> The UINTN type comes from the UEFI spec:
>>
>>     Unsigned value of native width. (4 bytes on supported 32-bit
>>     processor instructions, 8 bytes on supported 64-bit processor
>>     instructions, 16 bytes on supported 128-bit processor instructions)
>>
>> In this sense, "native" refers to the firmware execution environment.
>> The firmware execution environment need not have anything in common with
>> the build environment. (You can build 32-bit ARM firmware on X64 hosts.)
>> In such a scenario, using UINTN *at all* is fraught with
>> misunderstandings. It *would* be possible to use UINTN as it applies to
>> the build (= hosted) environment, and in that sense MAX_UINTN would also
>> be possible to define. However, the code being removed (= defining
>> MAX_UINTN as MAX_ADDRESS) proves that that approach would be very easy
>> to misunderstand and misuse. People could easily mistake it for applying
>> to the firmware execution environment.
>>
>> UINT32 and UINT64 are not affected by this ambiguity.
>>
>> Optimally, given that the build utilities target a hosted C runtime,
>> they should use standard C types, such as "unsigned int", or e.g.
>> "uint32_t". Together with standard C macros expressing limits, such as
>> UINT_MAX (from <limits.h>) and UINT32_MAX (from <stdint.h>).
>>
>> Clearly no-one has capacity to clean up BaseTools like this. For
>> starters, we should at least remove whatever actively causes confusion.
>>
>> Thanks,
>> Laszlo
>>
>> > On Mon, Dec 3, 2018 at 5:08 AM Laszlo Ersek <lersek@redhat.com> wrote:
>> >
>> >> On 11/30/18 23:45, Ard Biesheuvel wrote:
>> >>> The maximum value that can be represented by the native word size
>> >>> of the *target* should be irrelevant when compiling tools that
>> >>> run on the build *host*. So drop the definition of MAX_UINTN, now
>> >>> that we no longer use it.
>> >>>
>> >>> Contributed-under: TianoCore Contribution Agreement 1.1
>> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> >>> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
>> >>> ---
>> >>>  BaseTools/Source/C/Common/CommonLib.h | 1 -
>> >>>  1 file changed, 1 deletion(-)
>> >>>
>> >>> diff --git a/BaseTools/Source/C/Common/CommonLib.h
>> >> b/BaseTools/Source/C/Common/CommonLib.h
>> >>> index 6930d9227b87..b1c6c00a3478 100644
>> >>> --- a/BaseTools/Source/C/Common/CommonLib.h
>> >>> +++ b/BaseTools/Source/C/Common/CommonLib.h
>> >>> @@ -22,7 +22,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
>> >> EITHER EXPRESS OR IMPLIED.
>> >>>
>> >>>  #define MAX_LONG_FILE_PATH 500
>> >>>
>> >>> -#define MAX_UINTN MAX_ADDRESS
>> >>>  #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL)
>> >>>  #define MAX_UINT16  ((UINT16)0xFFFF)
>> >>>  #define MAX_UINT8   ((UINT8)0xFF)
>> >>>
>> >>
>> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> >> _______________________________________________
>> >> edk2-devel mailing list
>> >> edk2-devel@lists.01.org
>> >> https://lists.01.org/mailman/listinfo/edk2-devel
>> >>
>> >
>>

Re: [PATCH v2 6/6] BaseTools/CommonLib: drop definition of MAX_UINTN

[David F.]

I missed that it was for the build-tool source itself and not for the
targets that are built using edk2 and the API itself.


On Tue, Dec 11, 2018 at 2:55 PM Ard Biesheuvel <ard.biesheuvel@linaro.org>
wrote:

> On Tue, 11 Dec 2018 at 23:53, David F. <df7729@gmail.com> wrote:
> >
> > I don't know, to me it's very clear that UINTN is talking about the
> target, just like size_t would be.
> >
>
> But which target? This change is against the source of the BaseTools,
> which are host tools that can be used to build a single target image
> consisting of 32-bit and 64-bit modules. So which size is the native
> size in this case?
>
> > There are/were a bunch of API's using UINTN so using UINTN was
> desirable, and where needed UINTN_MAX.
> >
> > I just don't see an advantage to removing it.   Do see disadvantage to
> removing it for breaking existing code and for those that want the "native"
> (best/fasted/most efficient) int size for the processor (similar again to
> size_t)
> >
> > On Tue, Dec 11, 2018 at 7:46 AM Laszlo Ersek <lersek@redhat.com> wrote:
> >>
> >> On 12/11/18 08:11, David F. wrote:
> >> > Not sure why you'd take that out when someone using UINTN for
> variables may
> >> > want to use MAX_UINTN ?    Future may be different.
> >>
> >> The UINTN type comes from the UEFI spec:
> >>
> >>     Unsigned value of native width. (4 bytes on supported 32-bit
> >>     processor instructions, 8 bytes on supported 64-bit processor
> >>     instructions, 16 bytes on supported 128-bit processor instructions)
> >>
> >> In this sense, "native" refers to the firmware execution environment.
> >> The firmware execution environment need not have anything in common with
> >> the build environment. (You can build 32-bit ARM firmware on X64 hosts.)
> >> In such a scenario, using UINTN *at all* is fraught with
> >> misunderstandings. It *would* be possible to use UINTN as it applies to
> >> the build (= hosted) environment, and in that sense MAX_UINTN would also
> >> be possible to define. However, the code being removed (= defining
> >> MAX_UINTN as MAX_ADDRESS) proves that that approach would be very easy
> >> to misunderstand and misuse. People could easily mistake it for applying
> >> to the firmware execution environment.
> >>
> >> UINT32 and UINT64 are not affected by this ambiguity.
> >>
> >> Optimally, given that the build utilities target a hosted C runtime,
> >> they should use standard C types, such as "unsigned int", or e.g.
> >> "uint32_t". Together with standard C macros expressing limits, such as
> >> UINT_MAX (from <limits.h>) and UINT32_MAX (from <stdint.h>).
> >>
> >> Clearly no-one has capacity to clean up BaseTools like this. For
> >> starters, we should at least remove whatever actively causes confusion.
> >>
> >> Thanks,
> >> Laszlo
> >>
> >> > On Mon, Dec 3, 2018 at 5:08 AM Laszlo Ersek <lersek@redhat.com>
> wrote:
> >> >
> >> >> On 11/30/18 23:45, Ard Biesheuvel wrote:
> >> >>> The maximum value that can be represented by the native word size
> >> >>> of the *target* should be irrelevant when compiling tools that
> >> >>> run on the build *host*. So drop the definition of MAX_UINTN, now
> >> >>> that we no longer use it.
> >> >>>
> >> >>> Contributed-under: TianoCore Contribution Agreement 1.1
> >> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> >>> Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>
> >> >>> ---
> >> >>>  BaseTools/Source/C/Common/CommonLib.h | 1 -
> >> >>>  1 file changed, 1 deletion(-)
> >> >>>
> >> >>> diff --git a/BaseTools/Source/C/Common/CommonLib.h
> >> >> b/BaseTools/Source/C/Common/CommonLib.h
> >> >>> index 6930d9227b87..b1c6c00a3478 100644
> >> >>> --- a/BaseTools/Source/C/Common/CommonLib.h
> >> >>> +++ b/BaseTools/Source/C/Common/CommonLib.h
> >> >>> @@ -22,7 +22,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> >> >> EITHER EXPRESS OR IMPLIED.
> >> >>>
> >> >>>  #define MAX_LONG_FILE_PATH 500
> >> >>>
> >> >>> -#define MAX_UINTN MAX_ADDRESS
> >> >>>  #define MAX_UINT64 ((UINT64)0xFFFFFFFFFFFFFFFFULL)
> >> >>>  #define MAX_UINT16  ((UINT16)0xFFFF)
> >> >>>  #define MAX_UINT8   ((UINT8)0xFF)
> >> >>>
> >> >>
> >> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> >> >> _______________________________________________
> >> >> edk2-devel mailing list
> >> >> edk2-devel@lists.01.org
> >> >> https://lists.01.org/mailman/listinfo/edk2-devel
> >> >>
> >> >
> >>
>