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

Yaron Keren yaron.keren at gmail.com
Thu Apr 16 03:24:22 PDT 2015


This patch disables 4324, 'structure was padded due to __declspec(align())'.

OK to commit?


2015-04-15 15:13 GMT+03:00 NAKAMURA Takumi <geek4civic at gmail.com>:

> It'd be good idea for published headers to be W4-clean, even if they
> were ugly for us.
>
> 2015-04-15 21:10 GMT+09:00 Aaron Ballman <aaron at aaronballman.com>:
> > On Wed, Apr 15, 2015 at 3:49 AM, Yaron Keren <yaron.keren at gmail.com>
> wrote:
> >> Hi,
> >>
> >> Now with real /W4 in effect, there are new warnings
> >>
> >>  lib\support\Windows/TimeValue.inc(48): warning C4189: 'Error' : local
> >> variable is initialized but not referenced
> >
> > Thank you for fixing this one!
> >
> >>
> >>  lib\transforms\utils\lcssa.cpp(345): warning C4718: 'verifyLoop' :
> >> recursive call has no side effects, deleting
> >
> > I think this one has pointed out a possible issue, but likely a benign
> > one. It looks like the point to that recursive call was the assert,
> > but the code was commit with the assert commented out in r200067.
> >
> >>
> >> and tons of :
> >>
> >>  include\llvm/Support/AlignOf.h(25): warning C4324:
> >> 'llvm::AlignmentCalcImpl<T>' : structure was padded due to
> >> __declspec(align())
> >
> > I think this warning can be globally disabled, but it might be nice to
> > selectively enable it for headers we expose to the C API?
> >
> > ~Aaron
> >
> >>
> >> I fixed the first warning in r234982 but not sure if the other warnings
> >> should be disabled or the code fixed.
> >>
> >> Yaron
> >>
> >>
> >> 2015-04-14 2:46 GMT+03:00 Zachary Turner <zturner at google.com>:
> >>>
> >>> Will commit the patch to strip /W3 tomorrow morning if there's no
> further
> >>> objections.
> >>>
> >>> On Mon, Apr 13, 2015 at 12:25 PM Aaron Ballman <aaron at aaronballman.com
> >
> >>> wrote:
> >>>>
> >>>> On Mon, Apr 13, 2015 at 3:21 PM, Zachary Turner <zturner at google.com>
> >>>> wrote:
> >>>> > 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.
> >>>>
> >>>> If we're not regressing behavior with the patch you posted, then I
> >>>> think that patch is good. We definitely don't want a ton of warnings
> >>>> with Ninja.
> >>>>
> >>>> ~Aaron
> >>>>
> >>>> >
> >>>> > 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
> >>>> >> >>> >
> >>>
> >>>
> >>> _______________________________________________
> >>> 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/20150416/3b4379e0/attachment.html>
-------------- next part --------------
Index: cmake/modules/HandleLLVMOptions.cmake
===================================================================
--- cmake/modules/HandleLLVMOptions.cmake	(revision 235083)
+++ cmake/modules/HandleLLVMOptions.cmake	(working copy)
@@ -277,6 +277,7 @@
     -wd4258 # Suppress ''var' : definition from the for loop is ignored; the definition from the enclosing scope is used'
     -wd4267 # Suppress ''var' : conversion from 'size_t' to 'type', possible loss of data'
     -wd4291 # Suppress ''declaration' : no matching operator delete found; memory will not be freed if initialization throws an exception'
+    -wd4324 # Suppress 'structure was padded due to __declspec(align())'
     -wd4345 # Suppress 'behavior change: an object of POD type constructed with an initializer of the form () will be default-initialized'
     -wd4351 # Suppress 'new behavior: elements of array 'array' will be default initialized'
     -wd4355 # Suppress ''this' : used in base member initializer list'


More information about the llvm-commits mailing list