[llvm] r234343 - Enable W4 warnings by default for MSVC builds

Mon Apr 13 12:21:24 PDT 2015

I think I figured all this out (mostly).  Basically it only happens with
Ninja.  Ninja doesn't know what command line options actually mean, it just
makes a big list of them and passes them through for each file.  The Visual
Studio generator has to know, because to make a meaningful Visual Studio
project it has to generate XML with semantic tags like
<WarningLevel>Level4</WarningLevel>.  So I think what happens is the Visual
Studio generator seems /W3 then later sees /W4 and only generates one tag
in the vcxproj (I confirmed that even in my case where /W3 and /W4 are part
of CMAKE_CXX_FLAGS, Only a single <WarningLevel>Level4</WarningLevel> goes
into the vcxproj.

There's still the question of why your CMAKE_CXX_FLAGS doesn't contain
anything, and I'm not sure about that.  But I don't think it matters much,
because you would be seeing the same behavior with Visual Studio either way
(namely, that it would work).

On the other hand, ninja just appends commands line and passes them all
through without knowing what they mean, so it can't be made smart enough to
know that /W3 and /W4 should be collapsed.  You can verify that running "cl
/W3 /W4 foo.cpp" will reproduce the error, for example.

The patch I included earlier removes /W3 from the command line, but only if
it's about to append /W4.  I think this should cover everything?  Aaron
raised a question on IRC about what happens if the user manually specifies
a warning level on the command line.  I don't have a good answer to that,
other than to say that if you manually write CMAKE_CXX_FLAGS:STRING="/W2"
or something at CMake time, the current patch is already messed up as it
will then append /W4 which in the best could would overwrite your /W2
setting, and in the worst case would give the warnings I'm seeing here.  So
I feel like we should just be able to ignore this case, and if it becomes
an issue for someone in the future, we can add something like
-DMSVC_WARNING_LEVEL which will allow us a better means to be able to
override the /W4 with something else.

On Mon, Apr 13, 2015 at 12:11 PM Kaylor, Andrew <andrew.kaylor at intel.com>
wrote:

>  I’m not set up for Ninja, so I get this:
>
>
>
> CMake Error: CMake was unable to find a build program corresponding to
> "Ninja".
>
>
>
>
>
> But really I don’t know how much time it’s worth spending to figure out
> why it isn’t happening to me.  It is happening to you so we need to fix it.
>
>
>
>
>
> *From:* Zachary Turner [mailto:zturner at google.com]
> *Sent:* Monday, April 13, 2015 12:02 PM
>
>
> *To:* Kaylor, Andrew; Aaron Ballman
> *Cc:* llvm-commits at cs.uiuc.edu
> *Subject:* Re: [llvm] r234343 - Enable W4 warnings by default for MSVC
> builds
>
>
>
> Can you try the ninja generator?  First run "vcvarsall amd64", then just
> run "cmake -G Ninja f:\users\akaylor\llvm-s\llvm".  That's the only
> difference I can see in what we're doing.
>
>
>
> On Mon, Apr 13, 2015 at 12:00 PM Kaylor, Andrew <andrew.kaylor at intel.com>
> wrote:
>
> I added the line you suggested to CMakeLists.txt, and this is what I see:
>
>
>
> f:\users\akaylor\llvm-s\test>cmake -G "Visual Studio 12 2013 Win64"
> f:\users\akaylor\llvm-s\llvm
>
> CMAKE_CXX_FLAGS =
>
> -- No build type selected, default to Debug
>
> -- The C compiler identification is MSVC 18.0.31101.0
>
> …
>
>
>
> Also, the /W3 flag isn’t being used during my build.
>
>
>
> -Andy
>
>
>
>
>
> *From:* Zachary Turner [mailto:zturner at google.com]
> *Sent:* Monday, April 13, 2015 11:57 AM
> *To:* Kaylor, Andrew; Aaron Ballman
> *Cc:* llvm-commits at cs.uiuc.edu
>
>
> *Subject:* Re: [llvm] r234343 - Enable W4 warnings by default for MSVC
> builds
>
>
>
> No, CMake itself initializes CMAKE_CXX_FLAGS to include /W3.  By default,
> if you do nothing, CMAKE_CXX_FLAGS will have /W3.  You can verify this by
> printing the value of CMAKE_CXX_FLAGS in the first line of
> llvm/CMakeLists.txt
>
>
>
> On Mon, Apr 13, 2015 at 11:55 AM Kaylor, Andrew <andrew.kaylor at intel.com>
> wrote:
>
> I'm not sure I understand.  Are you saying this problem only happens if
> you have CMAKE_CXX_FLAGS defined to include W3?
>
> I don't have CMAKE_CXX_FLAGS defined on my system and my version of CMake
> (3.0.1) isn't defining it on its own, which would explain why I'm not
> seeing the problem.  I guess your patch to strip W3 if it's there is OK,
> though it should probably also look for W1 and W2.
>
> On the other hand, if there isn't some non-user reason that W3 would be
> part of CMAKE_CXX_FLAGS I would say this fall under the "don't do that"
> category of fix.
>
> -Andy
>
> -----Original Message-----
> From: aaron.ballman at gmail.com [mailto:aaron.ballman at gmail.com] On Behalf
> Of Aaron Ballman
> Sent: Monday, April 13, 2015 11:44 AM
> To: Zachary Turner
> Cc: Kaylor, Andrew; llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm] r234343 - Enable W4 warnings by default for MSVC builds
>
> When I do that, the first line of output shows /W3:
>
>
> E:\llvm\x64>"D:\Program Files (x86)\Microsoft Visual Studio
> 12.0\VC\vcvarsall.bat" amd64
>
> E:\llvm\x64>cmake -G "Visual Studio 12" ..\llvm CMAKE_CXX_FLAGS =  /DWIN32
> /D_WINDOWS /W3 /GR /EHsc ....
>
> ~Aaron
>
> On Mon, Apr 13, 2015 at 2:37 PM, Zachary Turner <zturner at google.com>
> wrote:
> > Also what version of CMake are you using?  Maybe it's an issue with
> > CMake >= 3.0?  Try putting the following line at the top of your
> > CMakeLists.txt
> >
> > message("CMAKE_CXX_FLAGS = ${CMAKE_CXX_FLAGS}")
> >
> > If you don't see /W3 in there, you won't hit the problem.
> >
> > On Mon, Apr 13, 2015 at 11:36 AM Zachary Turner <zturner at google.com>
> wrote:
> >>
> >> Just realized maybe 2012 was a typo.  Are you actually still using 2012?
> >> I thought support was dropped.
> >>
> >> On Mon, Apr 13, 2015 at 11:33 AM Aaron Ballman
> >> <aaron at aaronballman.com>
> >> wrote:
> >>>
> >>> I do not get the warnings when building x64 or x86 on Windows 7,
> >>> MSVC 2012, debug build. This is when building LLVM, Clang, lld, and
> >>> clang tools extra
> >>>
> >>> ~Aaron
> >>>
> >>> On Mon, Apr 13, 2015 at 2:22 PM, Zachary Turner <zturner at google.com>
> >>> wrote:
> >>> > I'm building x64.  Can you try that?  Run "vcvarsall amd64" before
> >>> > building LLVM
> >>> >
> >>> > On Mon, Apr 13, 2015 at 11:21 AM Kaylor, Andrew
> >>> > <andrew.kaylor at intel.com>
> >>> > wrote:
> >>> >>
> >>> >> I haven’t seen that.  I’m not seeing any warnings right now.
> >>> >>
> >>> >>
> >>> >>
> >>> >> From: Zachary Turner [mailto:zturner at google.com]
> >>> >> Sent: Monday, April 13, 2015 11:17 AM
> >>> >> To: Kaylor, Andrew; llvm-commits at cs.uiuc.edu
> >>> >> Subject: Re: [llvm] r234343 - Enable W4 warnings by default for
> >>> >> MSVC builds
> >>> >>
> >>> >>
> >>> >>
> >>> >> Hi Andy, this is causing a huge slew of warning spam.  Is this
> >>> >> expected?
> >>> >> CMake passes /W3 by default, so  every single translation unit I
> >>> >> compile gives me the following warning:
> >>> >>
> >>> >>
> >>> >>
> >>> >> cl : Command line warning D9025 : overriding '/W3' with '/W4'
> >>> >>
> >>> >>
> >>> >>
> >>> >> Furthermore, since this is a command line warning and not a
> >>> >> compiler warning, it cannot be suppressed.  Unless you have any
> >>> >> better ideas on how to address this, I think this should be
> >>> >> reverted.
> >>> >>
> >>> >>
> >>> >>
> >>> >> On Tue, Apr 7, 2015 at 12:07 PM Andrew Kaylor
> >>> >> <andrew.kaylor at intel.com>
> >>> >> wrote:
> >>> >>
> >>> >> Author: akaylor
> >>> >> Date: Tue Apr  7 14:01:01 2015
> >>> >> New Revision: 234343
> >>> >>
> >>> >> URL: http://llvm.org/viewvc/llvm-project?rev=234343&view=rev
> >>> >> Log:
> >>> >> Enable W4 warnings by default for MSVC builds
> >>> >>
> >>> >> Modified:
> >>> >>     llvm/trunk/CMakeLists.txt
> >>> >>
> >>> >> Modified: llvm/trunk/CMakeLists.txt
> >>> >> URL:
> >>> >>
> >>> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/CMakeLists.txt?rev
> >>> >> =234343&r1=234342&r2=234343&view=diff
> >>> >>
> >>> >>
> >>> >> =================================================================
> >>> >> =============
> >>> >> --- llvm/trunk/CMakeLists.txt (original)
> >>> >> +++ llvm/trunk/CMakeLists.txt Tue Apr  7 14:01:01 2015
> >>> >> @@ -233,14 +233,7 @@ list(REMOVE_DUPLICATES LLVM_TARGETS_TO_B
> >>> >>  include(AddLLVMDefinitions)
> >>> >>
> >>> >>  option(LLVM_ENABLE_PIC "Build Position-Independent Code" ON)
> >>> >> -
> >>> >> -# MSVC has a gazillion warnings with this.
> >>> >> -if( MSVC )
> >>> >> -  option(LLVM_ENABLE_WARNINGS "Enable compiler warnings." OFF)
> >>> >> -else()
> >>> >> -  option(LLVM_ENABLE_WARNINGS "Enable compiler warnings." ON)
> >>> >> -endif()
> >>> >> -
> >>> >> +option(LLVM_ENABLE_WARNINGS "Enable compiler warnings." ON)
> >>> >>  option(LLVM_ENABLE_MODULES "Compile with C++ modules enabled."
> >>> >> OFF)  option(LLVM_ENABLE_CXX1Y "Compile with C++1y enabled." OFF)
> >>> >> option(LLVM_ENABLE_LIBCXX "Use libc++ if available." OFF)
> >>> >>
> >>> >>
> >>> >> _______________________________________________
> >>> >> llvm-commits mailing list
> >>> >> llvm-commits at cs.uiuc.edu
> >>> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>> >
> >>> >
> >>> > _______________________________________________
> >>> > llvm-commits mailing list
> >>> > llvm-commits at cs.uiuc.edu
> >>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >>> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150413/c8530ace/attachment.html>