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

["Laszlo Ersek" <lersek@redhat.com>]

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)'
>>
>
> 
>