[llvm] r234343 - Enable W4 warnings by default for MSVC builds
Aaron Ballman
aaron at aaronballman.com
Thu Apr 16 05:15:22 PDT 2015
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
>> >>>
>> >>
>
>
More information about the llvm-commits
mailing list