Jordan Justen [mailto:[email protected]] wrote:
]On Mon, Sep 15, 2014 at 5:38 PM, Scott Duplichan <[email protected]> wrote: ]> Jordan Justen [mailto:[email protected]] wrote: ]> ]> ]I think in most cases our EDK II code base could compile cleanly with ]> ]these toolchains if minor changes were made to the source. For ]> ]example, I sent a patch in February: ]> ]"OvmfPkg: Fix VS2005 compiler warnings" ]> ]and, you actually commented on the thread. ]> ] ]> ]Anyway, I don't work in windows too often, and I ended up not ]> ]finishing cleaning up that patch and getting it upstream. ]> ] ]> ]But, generally, I think this is the approach that we prefer to dealing ]> ]with compiler warnings. Can we clean up warnings in these packages ]> ]through code rework instead of disabling warnings? ]> ]> From my point of view there are a couple of disadvantages to this approach. ]> The patch I submitted is a permanent fix. The code rework method requires ]> on-going work. That is, even if code changes can make the build pass today, ]> future code rework will be required as the project evolves. That is because ]> few developers use the old Microsoft tools. They will submit code that builds ]> cleanly with newer tools. But eventually someone will build with the old ]> tools and encounter one of these problems. That will require new code rework. ]> A more fundamental objection to me is the idea of changing working, portable, ]> bug-free code to work around the 'broken warnings' of old build tools. If ]> newer build tools didn't exist, then working around these warning limitations ]> might make sense. But newer build tools do exist, and in fact most developers ]> use them. I won't object further if someone else wants to change the code ]> for this reason, but I don't want my name on such a change. ] ]This would be a major change to how EDK II addresses these kinds of issues. ] ]EDK II (and EDK before that) has always pushed for a high number of ]warnings and caused a build errors on warnings. ] ]This strategy goes way back. I don't even know when it started. Maybe ]Vincent or Andrew know. ] ]At any rate, it does indeed cause an ongoing drag, but changing this ]has never been seriously considered. Maybe this is as much due to ]momentum as anything. :) ] ]But, I don't think you've made the case that disabling these warnings ]will end up producing better code. (Personally, I couldn't say one way ]or the other.) Hello Jordan, The problem this patch set addresses is broken x86 builds using Windows tool chains. To say 5 of the 9 supported Windows tool chains are broken is not something to be proud of: DDK3790 broken VS2003 broken VS2005 broken VS2008 working VS2010 working VS2012 working VS2013 broken Cygwin broken (I have been told) Intel ? The patch set is intended to make currently broken Windows tool chains usable, without making the code worse. In fact, this patch set contains zero code changes. That makes it unlikely to break any previously working build. Employees of big companies like Intel Dell, HP, etc have site licenses to Microsoft build tools. But what about smaller companies? Many readers of this list have never worked for a small company. Microsoft compilers cost several hundred US dollars per license. That kind of expense is hard to get approved from a small company. How do you tell your manager your group needs a set of new Visual Studio licenses because the UEFI guys at Intel won't fix their build problems? Intel is saying F*** Y** to the small developer. I am pretty sure this problem of broken tool chains will never get fixed. Even if someone who has all the Microsoft tools fixes the breakage by putting in a bunch of code workarounds, it won't stay fixed. That is because the current policy of enabling /WX on old Microsoft tool chains is clearly not workable. The only workable solution is to do like coreboot and have only one official tool chain. Then disable all troublesome warnings on all other tool chains. Is there any other solution? I think not. I guess the big difference of opinion here is about the significance of 5 broken tool chains. I imagine some of these have been broken for years. What kind of first impression does that make on someone new to EDK2 when he downloads it and tries to build it? Thanks, Scott ]-Jordan ] ]> ]Of course, if a warning impacts a huge portion of the code base, and ]> ]clearly has no value, then it might be disabled. ]> ] ]> ]For Microsoft, we seem to disable warnings this in ProcessorBind.h: ]> ]MdePkg/Include/Ia32/ProcessorBind.h ]> ](Rather than on the compiler command line.) ]> ]> It might be fun to temporarily re-enable these warnings. That ]> is an advantage if handling unwanted warnings at the compiler level ]> rather than at the code level: you can easily reverse a 1000 suppressed ]> warnings this way. On the other hand, if a 1000 warnings are suppressed ]> using code changes such as adding type casts, reversing the code changes ]> is essentially impossible. ]> ]> ]Regarding the subjects on your patch emails, I think many of these ]> ]should look like "BaseTools: ..." since they are modifying BaseTools. ]> ]> I suppose so. I didn't think of that. ]> ]> ]Patch 2 seems interesting, since as you point out it makes VS2013 ]> ]match the behavior of older VS toolchains. I don't know if IA32 builds ]> ]get to assume SSE2 support or not. ]> ] ]> ]I appreciate that you've tried to emulate the patch numbering of git ]> ]format-patch and git send-email. In case you are interested, I noticed ]> ]two differences: ]> ]> All I was really trying to do was to break up the patches. It was not easy ]> because the same line of tools_def.template is modified by multiple ]> patches. So I had to make incremental patches using a local SVN db and ]> committing to it after each patch. ]> ]> ]1. git format-patch inlines the patch by default ]> ] ]> ]This is nice since you can easily reply and 'quote' the code during code review. ]> ]> I can try this, though Microsoft email clients tend to make it really ]> hard to control line breaks. Mine is set line length 132, so it should ]> work in most cases. Surprising it limits max line length to 132. ]> ]> ]2. git send-email uses the email In-reply-to header to thread ]> ]subsequent patch postings together ]> ]> I think I could do that. ]> ]> ]This is nice if you use an email program that can show the email ]> ]thread (and thus, the patch series) as one group. ]> ] ]> ]-Jordan ]> > On Sun, Sep 14, 2014 at 9:03 PM, Scott Duplichan <[email protected]> wrote: >> Fix build failures for Microsoft tool chains. Tool chains tested: >> DDK3790, VS2003, VS2005, VS2008, VS2010, VS2012 and VS2013. The >> build.exe command lines tested are attached. The bulk of the build >> failures are due to warnings produced by older tool chains but not >> by newer tool chains. The following C code illustrates how the newer >> tool chains have been improved to recognize and eliminate integer >> truncation warnings in cases where no integer truncation occurs. >> The code also shows that the gcc optional -Wconversion warning is >> similar to, but not identical, to its Microsoft counterpart when >> newer gcc releases are used. >> >> #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 >> */ >> >> This patch set disables warnings in older tool chains when they >> are not compatible with newer tool chains. One reason is practicality: >> fewer developers use or even have access to the older tool chains. >> Another reason is that the warnings produced _only_ by the older tool >> chains are generally not useful. Disabling such warnings from older >> tool chains means that a developer who uses the older tool chain might >> submit code that will have warnings when compiled with a newer tool chain. >> This problem will be caught quickly because more users use new tool chains. >> If instead it was decided that older tool chains must build warning free, >> then developers using new tool chains would have to try to learn about the >> quirks in the older tool chains. >> >> One interesting warning case disabled by this patch set is VS2013 warning >> C4701/C4703 (potentially uninitialized local variable used). In a few cases >> this warning is valid and useful. It sometimes finds cases where ignoring the >> return code from a function that initializes a pointer leaves the pointer >> uninitialized. But there are a some problems with this warning. One is that >> the warning is incorrect in many cases. Another is that few developers have >> access to VS2013. >> >> There is no perfect solution to the current problem of multiple tool chains. >> A long term solution is to get a gcc build working smoothly under Windows. >> Then choose a particular version of gcc as the official tool chain. Code >> must compile warning free with the official tool chain. Gcc is free and >> available for Windows, Linux, Apple, etc. So anyone could use it. This >> is the approach used by the coreboot project. >> >> Thanks, >> Scott >> > ------------------------------------------------------------------------------ Want excitement? Manually upgrade your production database. When you want reliability, choose Perforce. Perforce version control. Predictably reliable. http://pubads.g.doubleclick.net/gampad/clk?id=157508191&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
