Gao, Liming [mailto:[email protected]] wrote:
]Scott: ] 1. Could we enable this option in GCC to make it same to Microsoft? Enabling gcc -Wconversion is certainly an option. The big challenge with doing so is this: The operation of both the gcc and Microsoft version of this warning have been refined and improved over the years. Here is some discussion about the gcc version of this warning: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=40752 Here are examples of discrepancies with this warning for various compilers: #if defined (__GNUC__) #include <stdint.h> #else #define uint8_t unsigned __int8 #define uint16_t unsigned __int16 #define uint32_t unsigned __int32 #define uint64_t unsigned __int64 #endif uint8_t test1 (int x) {return !x;} uint8_t test2 (int x) {return x == 1;} uint8_t test3 (int x) {return x && 1;} uint32_t test4 (uint64_t x) {return x >> 32;} uint32_t test5 (uint64_t x) {return x & 0xFFFFFFFF;} enum {a=1, b=2}; uint16_t test6 (int x) {return x ? a : b;} uint8_t test7 (uint8_t a, uint8_t b) {return a + b;} /* W A R N I N G C O U N T S Tool chain test1 test2 test3 test4 test5 test6 test7 DDK3790 /W4 1 1 1 1 1 1 0 VS2003 /W4 1 1 1 1 1 1 0 VS2005 /W4 1 1 1 1 1 0 0 VS2008 /W4 0 0 0 0 0 0 0 VS2010 /W4 0 0 0 0 0 0 0 VS2012 /W4 0 0 0 0 0 0 0 VS2013 /W4 0 0 0 0 0 0 0 gcc430 -Wconversion 1 1 1 1 1 1 1 gcc490 -Wconversion 0 0 0 1 0 0 1 Test7 shows that gcc -Wconversion adds a new warning case not present in any Microsoft compiler. Because of this, enabling gcc -Wconversion might create new gcc build fails to fix. I have not tried gcc -Wconversion on EDK2 yet. ] 2. Seemly, there is the warning C4344 problem in older Microsoft compilers. But, ]this warning can detect the real case. Now, there are few failure cases. How about ]fix them all? I think I understand what you are saying. In other words, "how about work around the cases that fail with older Microsoft compilers?" I think the biggest problem with doing so is knowing when the old Microsoft tools fail. We could 'fix' them all today. But what about for future code? Apparently few of the developers here use the older Microsoft tools. It would help if someone would setup an automated build server that runs all supported compilers. Just to make it clear, I really believe warning C4244 should be removed entirely. But I am not going to argue for this because I don't think I can convince enough people. Here is my argument against Microsoft warning C4244 for anyone interested: http://notabs.org/coding/warningLevel4.htm Here is a list of possible resolutions: Microsoft compiler GCC compiler 1 Disable C4244 for VS2005 and older no change 2 Disable C4244 for VS2005 and older enable -Wconversion 3 no change enable -Wconversion 4 Disable C4244, all versions no change pros and cons: Disable C4244 for VS2005 and older solves the problem without code changes. Adding gcc -Wconversion lets gcc developers produce better Microsoft compatibility. Adding gcc -Wconversion may create a few new warnings in the gcc build. Eliminating C4244 entirely increases code portability. Thanks, Scott ]Thanks ]Liming -----Original Message----- From: Scott Duplichan [mailto:[email protected]] Sent: Wednesday, September 03, 2014 12:40 PM To: [email protected] Subject: [edk2] Warning C4244 causes build failure with older Microsoft compilers EDK2 builds with 'warning level 4' when using Microsoft tools. For the most part, that enables desirable warnings similar to those provided by gcc -Wall. There is one major difference though, Microsoft warning level 4 warns about the routine practice of assigning a larger size integer to a smaller size integer. The Microsoft warning looks like: warning C4244: conversion from 'UINT32' to 'UINT8', possible loss of data Gcc has a warning option that is similar to Microsoft C4244: -WConversion But the gcc option -Wall does _not_ enable -Wconversion. In fact, gcc option -Wextra doesn't even enable -Wconversion. That suggests that -Wconversion is a specialty warning not suitable for general use. EDK2 gcc builds do not enable -Wconversion. I think Microsoft warning C4244 (and similar) should be removed from EDK2 so that Microsoft warning settings are more consistent with gcc warning settings. But that might be a difficult change to push through. While it would be interesting to see what others think, completely removing Microsoft Warning C4244 is a discussion for another day. The immediate problem should be a lot easier to get agreement on. The problem is that warning C4244 is buggy/imperfect in older Microsoft compilers (DDK3790, VS2003, VS2005). As a result, some builds fail with those compilers, but pass with newer ones. Here are examples: OvmfPkg\Library\LoadLinuxLib\Linux.c(387) OvmfPkg\Library\LoadLinuxLib\Linux.c(388) OvmfPkg\VirtioBlkDxe\VirtioBlk.c(773) OvmfPkg\VirtioBlkDxe\VirtioBlk.c(774) OvmfPkg\VirtioNetDxe\DriverBinding.c(132) OvmfPkg\VirtioScsiDxe\VirtioScsi.c(751) StdLib\BsdSocketLib\res_mkupdate.c(186) Here are code snippets that demonstrate the warning C4344 problem with older Microsoft compilers: #if defined (__GNUC__) #include <stdint.h> #else #define uint16_t unsigned __int16 #define uint32_t unsigned __int32 #define uint64_t unsigned __int64 #endif char test1 (int x) {return !x;} char test2 (int x) {return x == 1;} char test3 (int x) {return x & 1;} uint32_t test4 (uint64_t x) {return x >> 32;} uint32_t test5 (uint64_t x) {return x && 0xFFFFFFFF;} enum {a=1, b=2}; uint16_t test6 (int x) {return x ? a : b;} /* W A R N I N G C O U N T S Tool chain test1 test2 test3 test4 test5 test6 DDK3790 /W4 1 1 1 1 0 1 VS2003 /W4 1 1 1 1 0 1 VS2005 /W4 1 1 1 1 0 0 VS2008 /W4 0 0 0 0 0 0 VS2010 /W4 0 0 0 0 0 0 VS2012 /W4 0 0 0 0 0 0 VS2013 /W4 0 0 0 0 0 0 With VS2008 and newer, Microsoft compilers eliminate several causes of unwanted warnings. If others agree, I can make a BaseTools\Conf\tools_def.template patch to disable warning C4244 for DDK3790, VS2003, and VS2005. Thanks, Scott ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/ _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
