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

["Leif Lindholm" <leif@nuviainc.com>]

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