[PATCH v1 0/2] Fix VS2017 build errors

[PATCH v1 0/2] Fix VS2017 build errors

[PierreGondois]

Building edk2\EmbeddedPkg\EmbeddedPkg.dsc with
VS2017 triggered some errors. This patch set is
intended to fix them.

The changes can be seen at: https://github.com/PierreARM/edk2/commits/831_Fix_VS2017_build_error_v1

Pierre Gondois (2):
  EmbeddedPkg: Fix build error for MmcDxe
  EmbeddedPkg: Add cast from (void*) for VS2017 build

 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c | 13 ++++++++-----
 EmbeddedPkg/Universal/MmcDxe/Diagnostics.c                | 10 +++++++---
 EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c                 |  6 +++---
 3 files changed, 18 insertions(+), 11 deletions(-)

-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

[PATCH v1 1/2] EmbeddedPkg: Fix build error for MmcDxe

[PierreGondois]

From: Pierre Gondois <pierre.gondois@arm.com>

The following command line:
build -b NOOPT -a IA32 -t VS2017 -p edk2\EmbeddedPkg\EmbeddedPkg.dsc

Generates the following error:
MmcDxe.lib(Diagnostics.obj) : error LNK2001:
unresolved external symbol __allshl
MmcDxe.lib(Diagnostics.obj) : error LNK2001:
unresolved external symbol __aullshr
MmcDxe.lib(MmcBlockIo.obj) : error LNK2001:
unresolved external symbol __allmul

These erros are due to the use of shift/multiply operations
on UINT64 variable on a IA32 architecture.

Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---

The changes can be seen at: https://github.com/PierreARM/edk2/commits/831_Fix_VS2017_build_error_v1

Notes:
    v1:
     - Fix VS2017 build errors. [Pierre]

 EmbeddedPkg/Universal/MmcDxe/Diagnostics.c | 10 +++++++---
 EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c  |  6 +++---
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c b/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c
index 20defeb8745a2eb243f316ba9d4e0d03016e260b..49b069043093544a3cbadc46fda4de483803d638 100644
--- a/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c
+++ b/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c
@@ -1,7 +1,7 @@
 /** @file
   Diagnostics Protocol implementation for the MMC DXE driver
 
-  Copyright (c) 2011-2014, ARM Limited. All rights reserved.
+  Copyright (c) 2011-2020, ARM Limited. All rights reserved.
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -56,7 +56,7 @@ GenerateRandomBuffer (
   UINT64* Buffer64 = (UINT64*)Buffer;
 
   for (i = 0; i < (BufferSize >> 3); i++) {
-    *Buffer64 = i | (~i << 32);
+    *Buffer64 = i | LShiftU64 (~i, 32);
     Buffer64++;
   }
 }
@@ -227,7 +227,11 @@ MmcDriverDiagnosticsRunDiagnostics (
 
   // LBA=10 Size=BlockSize
   DiagnosticLog (L"MMC Driver Diagnostics - Test: Any Block\n");
-  Status = MmcReadWriteDataTest (MmcHostInstance, MmcHostInstance->BlockIo.Media->LastBlock >> 1, MmcHostInstance->BlockIo.Media->BlockSize);
+  Status = MmcReadWriteDataTest (
+             MmcHostInstance,
+             RShiftU64 (MmcHostInstance->BlockIo.Media->LastBlock, 1),
+             MmcHostInstance->BlockIo.Media->BlockSize
+             );
 
   // LBA=LastBlock Size=BlockSize
   DiagnosticLog (L"MMC Driver Diagnostics - Test: Last Block\n");
diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c b/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c
index b508c466d9c5c52ffff7855ea32cbd427927e27b..2a5d72d4daf6045e691e51d5b82ed8e6fb721121 100644
--- a/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c
+++ b/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c
@@ -1,6 +1,6 @@
 /** @file
 *
-*  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
+*  Copyright (c) 2011-2020, ARM Limited. All rights reserved.
 *
 *  SPDX-License-Identifier: BSD-2-Clause-Patent
 *
@@ -149,7 +149,7 @@ MmcTransferBlock (
     if (MmcHostInstance->CardInfo.OCRData.AccessMode & SD_CARD_CAPACITY) {
       CmdArg = Lba;
     } else {
-      CmdArg = Lba * This->Media->BlockSize;
+      CmdArg = MultU64x32 (Lba, This->Media->BlockSize);
     }
   } else {
     //Set command argument based on the card access mode (Byte mode or Block mode)
@@ -157,7 +157,7 @@ MmcTransferBlock (
         MMC_OCR_ACCESS_SECTOR) {
       CmdArg = Lba;
     } else {
-      CmdArg = Lba * This->Media->BlockSize;
+      CmdArg = MultU64x32 (Lba, This->Media->BlockSize);
     }
   }
 
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [PATCH v1 1/2] EmbeddedPkg: Fix build error for MmcDxe

[Leif Lindholm]

On Tue, Jun 30, 2020 at 11:49:00 +0100, PierreGondois wrote:
> From: Pierre Gondois <pierre.gondois@arm.com>
> 
> The following command line:
> build -b NOOPT -a IA32 -t VS2017 -p edk2\EmbeddedPkg\EmbeddedPkg.dsc
> 
> Generates the following error:
> MmcDxe.lib(Diagnostics.obj) : error LNK2001:
> unresolved external symbol __allshl
> MmcDxe.lib(Diagnostics.obj) : error LNK2001:
> unresolved external symbol __aullshr
> MmcDxe.lib(MmcBlockIo.obj) : error LNK2001:
> unresolved external symbol __allmul
> 
> These erros are due to the use of shift/multiply operations
> on UINT64 variable on a IA32 architecture.

Apart from how Ia32 should just bite the bullet and implement a
CompilerIntrinsicsLib for VS so the rest of us can go back to writing
C:

Reviewed-by: Leif Lindholm <leif@nuviainc.com>

> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
> 
> The changes can be seen at: https://github.com/PierreARM/edk2/commits/831_Fix_VS2017_build_error_v1
> 
> Notes:
>     v1:
>      - Fix VS2017 build errors. [Pierre]
> 
>  EmbeddedPkg/Universal/MmcDxe/Diagnostics.c | 10 +++++++---
>  EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c  |  6 +++---
>  2 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c b/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c
> index 20defeb8745a2eb243f316ba9d4e0d03016e260b..49b069043093544a3cbadc46fda4de483803d638 100644
> --- a/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c
> +++ b/EmbeddedPkg/Universal/MmcDxe/Diagnostics.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Diagnostics Protocol implementation for the MMC DXE driver
>  
> -  Copyright (c) 2011-2014, ARM Limited. All rights reserved.
> +  Copyright (c) 2011-2020, ARM Limited. All rights reserved.
>  
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
> @@ -56,7 +56,7 @@ GenerateRandomBuffer (
>    UINT64* Buffer64 = (UINT64*)Buffer;
>  
>    for (i = 0; i < (BufferSize >> 3); i++) {
> -    *Buffer64 = i | (~i << 32);
> +    *Buffer64 = i | LShiftU64 (~i, 32);
>      Buffer64++;
>    }
>  }
> @@ -227,7 +227,11 @@ MmcDriverDiagnosticsRunDiagnostics (
>  
>    // LBA=10 Size=BlockSize
>    DiagnosticLog (L"MMC Driver Diagnostics - Test: Any Block\n");
> -  Status = MmcReadWriteDataTest (MmcHostInstance, MmcHostInstance->BlockIo.Media->LastBlock >> 1, MmcHostInstance->BlockIo.Media->BlockSize);
> +  Status = MmcReadWriteDataTest (
> +             MmcHostInstance,
> +             RShiftU64 (MmcHostInstance->BlockIo.Media->LastBlock, 1),
> +             MmcHostInstance->BlockIo.Media->BlockSize
> +             );
>  
>    // LBA=LastBlock Size=BlockSize
>    DiagnosticLog (L"MMC Driver Diagnostics - Test: Last Block\n");
> diff --git a/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c b/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c
> index b508c466d9c5c52ffff7855ea32cbd427927e27b..2a5d72d4daf6045e691e51d5b82ed8e6fb721121 100644
> --- a/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c
> +++ b/EmbeddedPkg/Universal/MmcDxe/MmcBlockIo.c
> @@ -1,6 +1,6 @@
>  /** @file
>  *
> -*  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
> +*  Copyright (c) 2011-2020, ARM Limited. All rights reserved.
>  *
>  *  SPDX-License-Identifier: BSD-2-Clause-Patent
>  *
> @@ -149,7 +149,7 @@ MmcTransferBlock (
>      if (MmcHostInstance->CardInfo.OCRData.AccessMode & SD_CARD_CAPACITY) {
>        CmdArg = Lba;
>      } else {
> -      CmdArg = Lba * This->Media->BlockSize;
> +      CmdArg = MultU64x32 (Lba, This->Media->BlockSize);
>      }
>    } else {
>      //Set command argument based on the card access mode (Byte mode or Block mode)
> @@ -157,7 +157,7 @@ MmcTransferBlock (
>          MMC_OCR_ACCESS_SECTOR) {
>        CmdArg = Lba;
>      } else {
> -      CmdArg = Lba * This->Media->BlockSize;
> +      CmdArg = MultU64x32 (Lba, This->Media->BlockSize);
>      }
>    }
>  
> -- 
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 

[PATCH v1 2/2] EmbeddedPkg: Add cast from (void*) for VS2017 build

[PierreGondois]

From: Pierre Gondois <pierre.gondois@arm.com>

The following build configrations:
build -b DEBUG -a AARCH64 -t VS2017 -p edk2\EmbeddedPkg\EmbeddedPkg.dsc
build -b NOOPT -a AARCH64 -t VS2017 -p edk2\EmbeddedPkg\EmbeddedPkg.dsc
build -b RELEASE -a AARCH64 -t VS2017 -p edk2\EmbeddedPkg\EmbeddedPkg.dsc

are generating the following build errors:
edk2\EmbeddedPkg\Library\AndroidBootImgLib\AndroidBootImgLib.c(100):
error C2036: 'void *': unknown size
edk2\EmbeddedPkg\Library\AndroidBootImgLib\AndroidBootImgLib.c(347):
error C2036: 'void *': unknown size

Since the size of void* depends on the architecture, it can be
dangerous to use void* pointer arithmetic. Plus the C99 doesn't state
that void* pointer arithmetic is allowed.
This patch adds a cast to fix the Visual Studio errors reported.

Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
---

The changes can be seen at: https://github.com/PierreARM/edk2/commits/831_Fix_VS2017_build_error_v1

Notes:
    v1:
     - Fix VS2017 build errors. [Pierre]

 EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
index e1036954ee586dfc30266eec2897d71bfc949038..bbe0d41018b3d5665c72ee61efe737ae57b1b2eb 100644
--- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
+++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
@@ -1,6 +1,6 @@
 /** @file
 
-  Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
+  Copyright (c) 2013-2020, ARM Ltd. All rights reserved.<BR>
   Copyright (c) 2017, Linaro. All rights reserved.
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -97,7 +97,7 @@ AndroidBootImgGetKernelInfo (
   ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
 
   *KernelSize = Header->KernelSize;
-  *Kernel = BootImg + Header->PageSize;
+  *Kernel = (UINT8*)BootImg + Header->PageSize;
   return EFI_SUCCESS;
 }
 
@@ -339,9 +339,12 @@ AndroidBootImgUpdateFdt (
     goto Fdt_Exit;
   }
 
-  Status = AndroidBootImgSetProperty64 (UpdatedFdtBase, ChosenNode,
-                                        "linux,initrd-end",
-                                        (UINTN)(RamdiskData + RamdiskSize));
+  Status = AndroidBootImgSetProperty64 (
+             UpdatedFdtBase,
+             ChosenNode,
+             "linux,initrd-end",
+             (UINTN)((UINT8*)RamdiskData + RamdiskSize)
+             );
   if (EFI_ERROR (Status)) {
     goto Fdt_Exit;
   }
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'

Re: [PATCH v1 2/2] EmbeddedPkg: Add cast from (void*) for VS2017 build

[Leif Lindholm]

On Tue, Jun 30, 2020 at 11:49:01 +0100, PierreGondois wrote:
> From: Pierre Gondois <pierre.gondois@arm.com>
> 
> The following build configrations:
> build -b DEBUG -a AARCH64 -t VS2017 -p edk2\EmbeddedPkg\EmbeddedPkg.dsc
> build -b NOOPT -a AARCH64 -t VS2017 -p edk2\EmbeddedPkg\EmbeddedPkg.dsc
> build -b RELEASE -a AARCH64 -t VS2017 -p edk2\EmbeddedPkg\EmbeddedPkg.dsc
> 
> are generating the following build errors:
> edk2\EmbeddedPkg\Library\AndroidBootImgLib\AndroidBootImgLib.c(100):
> error C2036: 'void *': unknown size
> edk2\EmbeddedPkg\Library\AndroidBootImgLib\AndroidBootImgLib.c(347):
> error C2036: 'void *': unknown size
> 
> Since the size of void* depends on the architecture, it can be
> dangerous to use void* pointer arithmetic. Plus the C99 doesn't state
> that void* pointer arithmetic is allowed.
> This patch adds a cast to fix the Visual Studio errors reported.
> 
> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
> 
> The changes can be seen at: https://github.com/PierreARM/edk2/commits/831_Fix_VS2017_build_error_v1
> 
> Notes:
>     v1:
>      - Fix VS2017 build errors. [Pierre]
> 
>  EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> index e1036954ee586dfc30266eec2897d71bfc949038..bbe0d41018b3d5665c72ee61efe737ae57b1b2eb 100644
> --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> @@ -1,6 +1,6 @@
>  /** @file
>  
> -  Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
> +  Copyright (c) 2013-2020, ARM Ltd. All rights reserved.<BR>
>    Copyright (c) 2017, Linaro. All rights reserved.
>  
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> @@ -97,7 +97,7 @@ AndroidBootImgGetKernelInfo (
>    ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
>  
>    *KernelSize = Header->KernelSize;
> -  *Kernel = BootImg + Header->PageSize;
> +  *Kernel = (UINT8*)BootImg + Header->PageSize;

The reason I prefer my version, although this one would also solve the
compilation error, is that I really don't like casts to char * (which
this effectively is) as a workaround.

The problem I have with it is that a cast is a signal of intent (this
thing that we have been viewing as an X should now be seen as a Y) -
and the intent here is simply to get the side effect that a char has a
known size of 1 (whereas a void doesn't).
I will admit it is the first time I have seen it used for this purpose
:)

The same problem occure when casting to char * in order to not have to
deal with pointer arithmetic at all or to avoid thinking about
alignment requirements. And both of these are frequently indicators of
buggy code.

/
    Leif

>    return EFI_SUCCESS;
>  }
>  
> @@ -339,9 +339,12 @@ AndroidBootImgUpdateFdt (
>      goto Fdt_Exit;
>    }
>  
> -  Status = AndroidBootImgSetProperty64 (UpdatedFdtBase, ChosenNode,
> -                                        "linux,initrd-end",
> -                                        (UINTN)(RamdiskData + RamdiskSize));
> +  Status = AndroidBootImgSetProperty64 (
> +             UpdatedFdtBase,
> +             ChosenNode,
> +             "linux,initrd-end",
> +             (UINTN)((UINT8*)RamdiskData + RamdiskSize)
> +             );
>    if (EFI_ERROR (Status)) {
>      goto Fdt_Exit;
>    }
> -- 
> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
> 

Re: [edk2-devel] [PATCH v1 2/2] EmbeddedPkg: Add cast from (void*) for VS2017 build

[Laszlo Ersek]

On 07/22/20 17:35, Leif Lindholm wrote:
> On Tue, Jun 30, 2020 at 11:49:01 +0100, PierreGondois wrote:
>> From: Pierre Gondois <pierre.gondois@arm.com>
>>
>> The following build configrations:
>> build -b DEBUG -a AARCH64 -t VS2017 -p edk2\EmbeddedPkg\EmbeddedPkg.dsc
>> build -b NOOPT -a AARCH64 -t VS2017 -p edk2\EmbeddedPkg\EmbeddedPkg.dsc
>> build -b RELEASE -a AARCH64 -t VS2017 -p edk2\EmbeddedPkg\EmbeddedPkg.dsc
>>
>> are generating the following build errors:
>> edk2\EmbeddedPkg\Library\AndroidBootImgLib\AndroidBootImgLib.c(100):
>> error C2036: 'void *': unknown size
>> edk2\EmbeddedPkg\Library\AndroidBootImgLib\AndroidBootImgLib.c(347):
>> error C2036: 'void *': unknown size
>>
>> Since the size of void* depends on the architecture, it can be
>> dangerous to use void* pointer arithmetic. Plus the C99 doesn't state
>> that void* pointer arithmetic is allowed.
>> This patch adds a cast to fix the Visual Studio errors reported.
>>
>> Signed-off-by: Pierre Gondois <pierre.gondois@arm.com>
>> ---
>>
>> The changes can be seen at: https://github.com/PierreARM/edk2/commits/831_Fix_VS2017_build_error_v1
>>
>> Notes:
>>     v1:
>>      - Fix VS2017 build errors. [Pierre]
>>
>>  EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
>> index e1036954ee586dfc30266eec2897d71bfc949038..bbe0d41018b3d5665c72ee61efe737ae57b1b2eb 100644
>> --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
>> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
>> @@ -1,6 +1,6 @@
>>  /** @file
>>
>> -  Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
>> +  Copyright (c) 2013-2020, ARM Ltd. All rights reserved.<BR>
>>    Copyright (c) 2017, Linaro. All rights reserved.
>>
>>    SPDX-License-Identifier: BSD-2-Clause-Patent
>> @@ -97,7 +97,7 @@ AndroidBootImgGetKernelInfo (
>>    ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
>>
>>    *KernelSize = Header->KernelSize;
>> -  *Kernel = BootImg + Header->PageSize;
>> +  *Kernel = (UINT8*)BootImg + Header->PageSize;
>
> The reason I prefer my version, although this one would also solve the
> compilation error, is that I really don't like casts to char * (which
> this effectively is) as a workaround.
>
> The problem I have with it is that a cast is a signal of intent (this
> thing that we have been viewing as an X should now be seen as a Y) -
> and the intent here is simply to get the side effect that a char has a
> known size of 1 (whereas a void doesn't).
> I will admit it is the first time I have seen it used for this purpose
> :)

Personally I'd be OK with either fix (yours or Pierre's); to me both
express the exact same thing -- move Header->PageSize bytes past
BootImg.

Viewed from a portability perspective, Pierre's patch is actually more
portable (per C standard); as UINT8 stands for "unsigned char", and that
is called "object representation" by the C standard:

> 6.2.6 Representations of types
> 6.2.6.1 General
>
> 3 Values stored in unsigned bit-fields and objects of type unsigned
>   char shall be represented using a pure binary notation. (Footnote
>   40.)
>
> 4 Values stored in non-bit-field objects of any other object type
>   consist of n * CHAR_BIT bits, where n is the size of an object of
>   that type, in bytes. The value may be copied into an object of type
>   unsigned char [n] (e.g., by memcpy); the resulting set of bytes is
>   called the object representation of the value.
>
> Footnote 40: [...] A byte contains CHAR_BIT bits, and the values of
>              type unsigned char range from 0 to (2^CHAR_BIT)-1.

Further references:

> 6.2.5 Types
>
> 27 A pointer to void shall have the same representation and alignment
>    requirements as a pointer to a character type. Footnote 39 [...]
>
> Footnote 39: The same representation and alignment requirements are
>              meant to imply interchangeability as arguments to
>              functions, return values from functions, and members of
>              unions.

> 6.3.2.3 Pointers
>
> 7 [...] When a pointer to an object is converted to a pointer to a
>   character type, the result points to the lowest addressed byte of
>   the object. Successive increments of the result, up to the size of
>   the object, yield pointers to the remaining bytes of the object.

> 6.5 Expressions
>
> 6 The effective type of an object for an access to its stored value is
>   the declared type of the object, if any. (Footnote 75.) [...] If a
>   value is copied into an object having no declared type using memcpy
>   or memmove, or is copied as an array of character type, then the
>   effective type of the modified object for that access and for
>   subsequent accesses that do not modify the value is the effective
>   type of the object from which the value is copied, if it has one.

> Footnote 75: Allocated objects have no declared type.

(This is one of the most exciting parts of the standard BTW: it means
that, if you malloc() 4 bytes, and copy a local uint32_t variable into
it, with an open-coded (unsigned char*) loop, then the effective type of
the dynamically allocated object becomes uint32_t for subsequent reads!
Which means for example, considering the effective type rules strictly,
that reading the dynamically allocated object post-copy, even via
*correctly aligned* (uint16_t*) pointers, is undefined behavior.)

> 7 An object shall have its stored value accessed only by an lvalue
>   expression that has one of the following types (Footnote 76):
>   - [...]
>   - a character type.
>
> Footnote 76: The intent of this list is to specify those circumstances
>              in which an object may or may not be aliased.

Whereas converting a (VOID*) to UINTN (and maybe back) is only covered
here (if I remember correctly):

> 6.3 Conversions
> 6.3.2 Other operands
> 6.3.2.3 Pointers
>
> 5 An integer may be converted to any pointer type. Except as
>   previously specified, the result is implementation-defined, might
>   not be correctly aligned, might not point to an entity of the
>   referenced type, and might be a trap representation. (Footnote 56)
>
> 6 Any pointer type may be converted to an integer type. Except as
>   previously specified, the result is implementation-defined. If the
>   result cannot be represented in the integer type, the behavior is
>   undefined. The result need not be in the range of values of any
>   integer type.
>
> Footnote 56: The mapping functions for converting a pointer to an
>              integer or an integer to a pointer are intended to be
>              consistent with the addressing structure of the execution
>              environment.

So my only point is that (void*) --> (unsigned char *) has no
"implementation-defined" dependencies, while (VOID*) --> (UINTN) is
implementation-defined. (The dependency exploited in edk2 is that UINTN
is always just as wide as (VOID*).)

But, given both patches are for edk2, where UINTN <-> (VOID*)
interchangeability is guaranteed, I'm equally fine with both patches.

Thanks
Laszlo

>
> The same problem occure when casting to char * in order to not have to
> deal with pointer arithmetic at all or to avoid thinking about
> alignment requirements. And both of these are frequently indicators of
> buggy code.
>
> /
>     Leif
>
>>    return EFI_SUCCESS;
>>  }
>>
>> @@ -339,9 +339,12 @@ AndroidBootImgUpdateFdt (
>>      goto Fdt_Exit;
>>    }
>>
>> -  Status = AndroidBootImgSetProperty64 (UpdatedFdtBase, ChosenNode,
>> -                                        "linux,initrd-end",
>> -                                        (UINTN)(RamdiskData + RamdiskSize));
>> +  Status = AndroidBootImgSetProperty64 (
>> +             UpdatedFdtBase,
>> +             ChosenNode,
>> +             "linux,initrd-end",
>> +             (UINTN)((UINT8*)RamdiskData + RamdiskSize)
>> +             );
>>    if (EFI_ERROR (Status)) {
>>      goto Fdt_Exit;
>>    }
>> --
>> 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'
>>
>
> 
>

Re: [edk2-devel] [PATCH v1 2/2] EmbeddedPkg: Add cast from (void*) for VS2017 build

[Leif Lindholm]

On Wed, Jul 22, 2020 at 23:11:40 +0200, Laszlo Ersek wrote:
> >> diff --git a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> >> index e1036954ee586dfc30266eec2897d71bfc949038..bbe0d41018b3d5665c72ee61efe737ae57b1b2eb 100644
> >> --- a/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> >> +++ b/EmbeddedPkg/Library/AndroidBootImgLib/AndroidBootImgLib.c
> >> @@ -1,6 +1,6 @@
> >>  /** @file
> >>
> >> -  Copyright (c) 2013-2014, ARM Ltd. All rights reserved.<BR>
> >> +  Copyright (c) 2013-2020, ARM Ltd. All rights reserved.<BR>
> >>    Copyright (c) 2017, Linaro. All rights reserved.
> >>
> >>    SPDX-License-Identifier: BSD-2-Clause-Patent
> >> @@ -97,7 +97,7 @@ AndroidBootImgGetKernelInfo (
> >>    ASSERT (IS_VALID_ANDROID_PAGE_SIZE (Header->PageSize));
> >>
> >>    *KernelSize = Header->KernelSize;
> >> -  *Kernel = BootImg + Header->PageSize;
> >> +  *Kernel = (UINT8*)BootImg + Header->PageSize;
> >
> > The reason I prefer my version, although this one would also solve the
> > compilation error, is that I really don't like casts to char * (which
> > this effectively is) as a workaround.
> >
> > The problem I have with it is that a cast is a signal of intent (this
> > thing that we have been viewing as an X should now be seen as a Y) -
> > and the intent here is simply to get the side effect that a char has a
> > known size of 1 (whereas a void doesn't).
> > I will admit it is the first time I have seen it used for this purpose
> > :)
> 
> Personally I'd be OK with either fix (yours or Pierre's); to me both
> express the exact same thing -- move Header->PageSize bytes past
> BootImg.
> 
> Viewed from a portability perspective, Pierre's patch is actually more
> portable (per C standard); as UINT8 stands for "unsigned char", and that
> is called "object representation" by the C standard:

Because there was a time when C didn't have void pointers.

> > 6.2.6 Representations of types
> > 6.2.6.1 General
> >
> > 3 Values stored in unsigned bit-fields and objects of type unsigned
> >   char shall be represented using a pure binary notation. (Footnote
> >   40.)
> >
> > 4 Values stored in non-bit-field objects of any other object type
> >   consist of n * CHAR_BIT bits, where n is the size of an object of
> >   that type, in bytes. The value may be copied into an object of type
> >   unsigned char [n] (e.g., by memcpy); the resulting set of bytes is
> >   called the object representation of the value.
> >
> > Footnote 40: [...] A byte contains CHAR_BIT bits, and the values of
> >              type unsigned char range from 0 to (2^CHAR_BIT)-1.
> 
> Further references:
> 
> > 6.2.5 Types
> >
> > 27 A pointer to void shall have the same representation and alignment
> >    requirements as a pointer to a character type. Footnote 39 [...]
> >
> > Footnote 39: The same representation and alignment requirements are
> >              meant to imply interchangeability as arguments to
> >              functions, return values from functions, and members of
> >              unions.
> 
> > 6.3.2.3 Pointers
> >
> > 7 [...] When a pointer to an object is converted to a pointer to a
> >   character type, the result points to the lowest addressed byte of
> >   the object. Successive increments of the result, up to the size of
> >   the object, yield pointers to the remaining bytes of the object.
> 
> > 6.5 Expressions
> >
> > 6 The effective type of an object for an access to its stored value is
> >   the declared type of the object, if any. (Footnote 75.) [...] If a
> >   value is copied into an object having no declared type using memcpy
> >   or memmove, or is copied as an array of character type, then the
> >   effective type of the modified object for that access and for
> >   subsequent accesses that do not modify the value is the effective
> >   type of the object from which the value is copied, if it has one.
> 
> > Footnote 75: Allocated objects have no declared type.
> 
> (This is one of the most exciting parts of the standard BTW: it means
> that, if you malloc() 4 bytes, and copy a local uint32_t variable into
> it, with an open-coded (unsigned char*) loop, then the effective type of
> the dynamically allocated object becomes uint32_t for subsequent reads!
> Which means for example, considering the effective type rules strictly,
> that reading the dynamically allocated object post-copy, even via
> *correctly aligned* (uint16_t*) pointers, is undefined behavior.)

Which is only made funnier by the fact that you cannot implement a
compliant malloc that returns a pointer with less than your largest
data type alignment requirement.

> > 7 An object shall have its stored value accessed only by an lvalue
> >   expression that has one of the following types (Footnote 76):
> >   - [...]
> >   - a character type.
> >
> > Footnote 76: The intent of this list is to specify those circumstances
> >              in which an object may or may not be aliased.
> 
> Whereas converting a (VOID*) to UINTN (and maybe back) is only covered
> here (if I remember correctly):
> 
> > 6.3 Conversions
> > 6.3.2 Other operands
> > 6.3.2.3 Pointers
> >
> > 5 An integer may be converted to any pointer type. Except as
> >   previously specified, the result is implementation-defined, might
> >   not be correctly aligned, might not point to an entity of the
> >   referenced type, and might be a trap representation. (Footnote 56)
> >
> > 6 Any pointer type may be converted to an integer type. Except as
> >   previously specified, the result is implementation-defined. If the
> >   result cannot be represented in the integer type, the behavior is
> >   undefined. The result need not be in the range of values of any
> >   integer type.
> >
> > Footnote 56: The mapping functions for converting a pointer to an
> >              integer or an integer to a pointer are intended to be
> >              consistent with the addressing structure of the execution
> >              environment.
> 
> So my only point is that (void*) --> (unsigned char *) has no
> "implementation-defined" dependencies, while (VOID*) --> (UINTN) is
> implementation-defined.

Sure. But (unsigned char *) still reads as "pointer to region
containing stuff with 1-byte alignment requirements", whereas (void *)
reads as pointer to region containing
...well-let's-not-worry-about-that-for now...

See also https://martinfowler.com/bliki/CodeSmell.html

> (The dependency exploited in edk2 is that UINTN
> is always just as wide as (VOID*).)

Yes. I wouldn't like to to try to port EDK2 to CHERI.

I actually find most of these things easier to reason about in the
context of fat pointers. But that case also emphasises the danger
presented by the special treatment of unsigned char * (for legacy
purposes) in the standard is.
In a system where pointers contained information about the
size/alignment of the element pointed to, that would now in hardware
encode "1-byte alignment" instead of "unknown alignment". And would be
dereferenced without raising an exception.

> But, given both patches are for edk2, where UINTN <-> (VOID*)
> interchangeability is guaranteed, I'm equally fine with both patches.

Thanks!

/
    Leif