Re: [PATCH 1/6] MdePkg: Disable some Level 4 warnings for VS2017/ARM

[Pete Batard <pete@akeo.ie>]

Hi Liming,

Thanks for reviewing these patches.

As explained in the git comment, I chose to disable warnings that I saw 
being generated more than 5 times during QEMU compilation, as I suspect 
we may inherit things from gcc that we may not want to spend too much 
time changing, just to keep MSVC happy.

Considering that we are introducing a new target for MS users, it may be 
worth being a bit more lenient with warnings for the time being, so that 
people don't have to spend too much time addressing issues that they 
wouldn't get with other compilers (gcc, Clang), if they decide to try 
VS2017/ARM, as it may drive them off otherwise.

Especially, if we try to align ARM warnings with IA32/X64, then we're 
going to have to address at least 78 instances of C4244 warnings 
("conversion from 'type1' to 'type2', possible loss of data") in our ARM 
codebase eventually, which is probably not in our best interest.

For reference, here is the list of Level 4 compiler warnings I got, 
during QEMU compilation, along with the number of times they occurred:

C4018:   7
C4101:   6
C4132:   1
C4146:   3
C4244:  78
C4366:   1
C4389:   1
C4456:  10
C4701:  50
C4703:  48

This being said, I suspect most of the warnings above may not be 
generated when compiling regular UEFI ARM applications or drivers, as I 
only started to see those when trying to compile QEMU.

So, if you think that's preferable, we can proceed as you suggest, and 
align ARM disabled warnings with IA32 and X64. Then we can wait and see 
if users of VS2017/ARM start to complain that more warnings need to be 
disabled...

Regards,

/Pete

On 2017.12.04 15:36, Gao, Liming wrote:
> Pete:
>    I suggest to align the disabled warning list to IA32/X64 arch. I find someone are not listed in IA32/X64 arch. Could you give the more detail on why disable them by default?
> 
> Thanks
> Liming
>> -----Original Message-----
>> From: Pete Batard [mailto:pete@akeo.ie]
>> Sent: Monday, December 4, 2017 9:12 PM
>> To: edk2-devel@lists.01.org
>> Cc: Gao, Liming <liming.gao@intel.com>
>> Subject: [PATCH 1/6] MdePkg: Disable some Level 4 warnings for VS2017/ARM
>>
>> Warnings 4018, 4100, 4101, 4127, 4214, 4244, 4456, 4701 and 4703 are
>> disabled as they were found to occur more than 5 times during QEMU
>> firmware compilation.
>>
>> Also create a dummy macro for PRESERVE8, as this is not supported by
>> the Microsoft ARM assembler.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Pete Batard <pete@akeo.ie>
>> ---
>>   MdePkg/Include/Arm/ProcessorBind.h | 96 +++++++++++++++-----
>>   1 file changed, 75 insertions(+), 21 deletions(-)
>>
>> diff --git a/MdePkg/Include/Arm/ProcessorBind.h b/MdePkg/Include/Arm/ProcessorBind.h
>> index dde1fd1152ba..00de80bafe0a 100644
>> --- a/MdePkg/Include/Arm/ProcessorBind.h
>> +++ b/MdePkg/Include/Arm/ProcessorBind.h
>> @@ -1,15 +1,15 @@
>>   /** @file
>>     Processor or Compiler specific defines and types for ARM.
>>
>> -  Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.<BR>
>> +  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
>>     Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.<BR>
>> -  This program and the accompanying materials
>> -  are licensed and made available under the terms and conditions of the BSD License
>> -  which accompanies this distribution.  The full text of the license may be found at
>> -  http://opensource.org/licenses/bsd-license.php
>> +  This program and the accompanying materials
>> +  are licensed and made available under the terms and conditions of the BSD License
>> +  which accompanies this distribution.  The full text of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php
>>
>> -  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> -  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>>
>>   **/
>>
>> @@ -28,14 +28,63 @@
>>   #pragma pack()
>>   #endif
>>
>> +#if defined(_MSC_EXTENSIONS)
>> +
>>   //
>> -// RVCT does not support the __builtin_unreachable() macro
>> +// Disable 'signed/unsigned mismatch' warnings.
>>   //
>> -#ifdef __ARMCC_VERSION
>> +#pragma warning ( disable : 4018 )
>> +
>> +//
>> +// Disable 'unreferenced formal parameter' warnings.
>> +//
>> +#pragma warning ( disable : 4100 )
>> +
>> +//
>> +// Disable 'unreferenced local variable' warnings.
>> +//
>> +#pragma warning ( disable : 4101 )
>> +
>> +//
>> +// Suppress warnings for ASSERT(FALSE) or while(TRUE).
>> +//
>> +#pragma warning ( disable : 4127 )
>> +
>> +//
>> +// Disable bitfield type check warnings.
>> +//
>> +#pragma warning ( disable : 4214 )
>> +
>> +//
>> +// Disable 'conversion from X to Y, possible loss of data' warnings
>> +//
>> +#pragma warning ( disable : 4244 )
>> +
>> +//
>> +// Disable 'declaration of X hides previous local declaration' warnings
>> +//
>> +#pragma warning ( disable : 4456 )
>> +
>> +//
>> +// Disable 'potentially uninitialized local variable X used' warnings
>> +//
>> +#pragma warning ( disable : 4701 )
>> +
>> +//
>> +// Disable 'potentially uninitialized local pointer variable X used' warnings
>> +//
>> +#pragma warning ( disable : 4703 )
>> +
>> +#endif
>> +
>> +//
>> +// RVCT and MSFT don't support the __builtin_unreachable() macro
>> +//
>> +#if defined(__ARMCC_VERSION) || defined(_MSC_EXTENSIONS)
>>   #define UNREACHABLE()
>>   #endif
>>
>> -#if _MSC_EXTENSIONS
>> +#if defined(_MSC_EXTENSIONS)
>>     //
>>     // use Microsoft* C compiler dependent integer width types
>>     //
>> @@ -52,7 +101,7 @@
>>     typedef signed char         INT8;
>>   #else
>>     //
>> -  // Assume standard ARM alignment.
>> +  // Assume standard ARM alignment.
>>     // Need to check portability of long long
>>     //
>>     typedef unsigned long long  UINT64;
>> @@ -121,7 +170,7 @@ typedef INT32   INTN;
>>   // use the correct C calling convention. All protocol member functions and
>>   // EFI intrinsics are required to modify their member functions with EFIAPI.
>>   //
>> -#define EFIAPI
>> +#define EFIAPI
>>
>>   // When compiling with Clang, we still use GNU as for the assembler, so we still
>>   // need to define the GCC_ASM* macros.
>> @@ -142,34 +191,39 @@ typedef INT32   INTN;
>>
>>       #define GCC_ASM_EXPORT(func__)  \
>>                .global  _CONCATENATE (__USER_LABEL_PREFIX__, func__)    ;\
>> -             .type ASM_PFX(func__), %function
>> +             .type ASM_PFX(func__), %function
>>
>>       #define GCC_ASM_IMPORT(func__)  \
>>                .extern  _CONCATENATE (__USER_LABEL_PREFIX__, func__)
>> -
>> +
>>     #else
>>       //
>> -    // .type not supported by Apple Xcode tools
>> +    // .type not supported by Apple Xcode tools
>>       //
>> -    #define INTERWORK_FUNC(func__)
>> +    #define INTERWORK_FUNC(func__)
>>
>>       #define GCC_ASM_EXPORT(func__)  \
>>                .globl  _CONCATENATE (__USER_LABEL_PREFIX__, func__)    \
>> -
>> -    #define GCC_ASM_IMPORT(name)
>> +
>> +    #define GCC_ASM_IMPORT(name)
>>
>>     #endif
>> +#elif defined(_MSC_EXTENSIONS)
>> +  //
>> +  // PRESERVE8 is not supported by the MSFT assembler.
>> +  //
>> +  #define PRESERVE8
>>   #endif
>>
>>   /**
>>     Return the pointer to the first instruction of a function given a function pointer.
>> -  On ARM CPU architectures, these two pointer values are the same,
>> +  On ARM CPU architectures, these two pointer values are the same,
>>     so the implementation of this macro is very simple.
>> -
>> +
>>     @param  FunctionPointer   A pointer to a function.
>>
>>     @return The pointer to the first instruction of a function given a function pointer.
>> -
>> +
>>   **/
>>   #define FUNCTION_ENTRY_POINT(FunctionPointer) (VOID *)(UINTN)(FunctionPointer)
>>
>> --
>> 2.9.3.windows.2
>