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

[Yaron Keren]

That's a good question, maybe build definitions? I use:

cmake -G "Visual Studio 12" -DLLVM_INCLUDE_EXAMPLES=ON
-DLLVM_INCLUDE_TESTS=ON -DLLVM_INCLUDE_DOCS=OFF -DLLVM_ENABLE_ASSERTIONS=ON
-DLLVM_ENABLE_THREADS:BOOL=OFF  -DLLVM_USE_CRT_DEBUG=MTd ..
"c:\Program Files (x86)\MSBuild\12.0\Bin\msbuild.exe" llvm.sln
/target:ALL_BUILD /property:Configuration=Release

I'll try to to see what may be relevant to the C4324 warnings. They appear
a lot.



2015-04-16 15:15 GMT+03:00 Aaron Ballman <aaron at aaronballman.com>:

> On Thu, Apr 16, 2015 at 6:24 AM, Yaron Keren <yaron.keren at gmail.com>
> wrote:
> > This patch disables 4324, 'structure was padded due to
> __declspec(align())'.
> >
> > OK to commit?
>
> Patch itself looks good, but I do have two questions before the LGTM.
> I am not seeing any of these warnings in my MSVC build for some
> reason, so that's a bit concerning. Takumi's builders are also showing
> up clean. So why are you seeing warnings that others are not? My other
> question is, how many of these C4324 warnings are with published
> headers? We should fix the code in those cases, if it's not overly
> onerous, because having our published headers build cleanly is
> important for out of tree projects.
>
> Thanks!
>
> ~Aaron
>
> >
> >
> > 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/f569c2a4/attachment.html>