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

Yaron Keren yaron.keren at gmail.com
Tue Apr 21 22:13:19 PDT 2015 

Oh, I see Andy already disabled C4324 in r235450 (thanks!) so I reverted
your #pragma warning patch in r235480. All done.


2015-04-22 7:55 GMT+03:00 Yaron Keren <yaron.keren at gmail.com>:

> In general I'm pro-warnings, but C4324 warning is not helpful...the
> compiler says it aligned what you specifically asked to align. Surprise.
>
> Go ahead and disable it,  you can remove r235219 which will become
> superfluous.
>
>
> 2015-04-22 0:57 GMT+03:00 Aaron Ballman <aaron at aaronballman.com>:
>
>> On Tue, Apr 21, 2015 at 4:58 PM, Yaron Keren <yaron.keren at gmail.com>
>> wrote:
>> > Well, building for Visual C++ 2013 Win 64 there is a flood of:
>>
>> Ah, I was building for x86, so this makes sense.
>>
>> >  tools\clang\include\clang/AST/Stmt.h(437): warning C4324:
>> 'clang::Stmt' :
>> > structure was padded due to __declspec(align())
>> >
>> > and also
>> >
>> >  lib\Support\CrashRecoveryContext.cpp(29): warning C4324:
>> > '`anonymous-namespace'::CrashRecoveryContextImpl' : structure was
>> padded due
>> > to __declspec(align())
>> >
>> > We could continue fixing these or just disable this warning.
>>
>> These I am far less concerned about fixing. I think we're fine
>> disabling the warning (for all MSVC targets, not just x64). What do
>> you think?
>>
>> ~Aaron
>>
>> >
>> >
>> >
>> > 2015-04-17 23:26 GMT+03:00 Yaron Keren <yaron.keren at gmail.com>:
>> >>
>> >> Thanks!
>> >>
>> >> 2015-04-17 22:40 GMT+03:00 Aaron Ballman <aaron at aaronballman.com>:
>> >>>
>> >>> On Thu, Apr 16, 2015 at 11:47 AM, Aaron Ballman <
>> aaron at aaronballman.com>
>> >>> wrote:
>> >>> > On Thu, Apr 16, 2015 at 11:43 AM, Yaron Keren <
>> yaron.keren at gmail.com>
>> >>> > wrote:
>> >>> >> This LGTM, with #ifdef _MSC_VER macro of course, maybe in
>> Compiler.h ?
>> >>> >
>> >>> > That was what I was thinking, yes.
>> >>> >
>> >>> >> I wouldn't mind disabling the warning completely either, I don't
>> find
>> >>> >> it
>> >>> >> helpful.
>> >>> >
>> >>> > Since this is the only instance of that warning, and we may want to
>> >>> > fix it to keep the published support headers compiling at /W4
>> cleanly,
>> >>> > I don't see a reason to disable the warning. This warning *could*
>> give
>> >>> > us a heads up on behavioral differences for binary formats due to
>> >>> > __declspec(align), but it's just as likely to trigger false
>> positives,
>> >>> > so I wouldn't argue too hard against disabling the warning, either.
>> >>>
>> >>> I have disabled this warning in AlignOf.h in r235219. I did not add a
>> >>> macro for it because MSVC doesn't support _Pragma from my testing.
>> >>>
>> >>> ~Aaron
>> >>>
>> >>> >
>> >>> > ~Aaron
>> >>> >
>> >>> >>
>> >>> >>
>> >>> >> 2015-04-16 18:12 GMT+03:00 Aaron Ballman <aaron at aaronballman.com>:
>> >>> >>>
>> >>> >>> Since AlignOf is part of our support headers, I would slightly
>> prefer
>> >>> >>> if we could fix it there. As an experiment, I put in the proper
>> >>> >>> #pragma incantation around the declaration of
>> AlignmentCalcImpl<T> in
>> >>> >>> AlignOf.h, and it resolved *all* of the warnings.
>> >>> >>>
>> >>> >>> #pragma warning(push)
>> >>> >>> #pragma warning(disable : 4324)
>> >>> >>> template <typename T>
>> >>> >>> struct AlignmentCalcImpl {
>> >>> >>>   char x;
>> >>> >>>   T t;
>> >>> >>> private:
>> >>> >>>   AlignmentCalcImpl() {} // Never instantiate.
>> >>> >>> };
>> >>> >>> #pragma warning(pop)
>> >>> >>>
>> >>> >>>
>> >>> >>> While the #pragma dance is ugly (and would need to be abstracted
>> away
>> >>> >>> via macros), it may be worth it since this is a support header
>> (and
>> >>> >>> we
>> >>> >>> cannot fix the padding issue because that's the whole point to
>> that
>> >>> >>> structure). How do others feel?
>> >>> >>>
>> >>> >>> ~Aaron
>> >>> >>>
>> >>> >>> On Thu, Apr 16, 2015 at 10:49 AM, Yaron Keren <
>> yaron.keren at gmail.com>
>> >>> >>> wrote:
>> >>> >>> > /W3 was probably cached from the previous build.
>> >>> >>> >
>> >>> >>> > So should we disable C4324 or fix (how?) the AlignOf?
>> >>> >>> >
>> >>> >>> >
>> >>> >>> > 2015-04-16 17:34 GMT+03:00 Aaron Ballman <
>> aaron at aaronballman.com>:
>> >>> >>> >>
>> >>> >>> >> It seems that in order to trigger this, you have to have done a
>> >>> >>> >> clean
>> >>> >>> >> cmake build, as I am now seeing the same warnings you are.
>> >>> >>> >>
>> >>> >>> >> ~Aaron
>> >>> >>> >>
>> >>> >>> >> On Thu, Apr 16, 2015 at 10:16 AM, Aaron Ballman
>> >>> >>> >> <aaron at aaronballman.com>
>> >>> >>> >> wrote:
>> >>> >>> >> > On Thu, Apr 16, 2015 at 10:03 AM, Yaron Keren
>> >>> >>> >> > <yaron.keren at gmail.com>
>> >>> >>> >> > wrote:
>> >>> >>> >> >> It is same in the IDE and command line, Release or Debug
>> >>> >>> >> >> (unthreaded),
>> >>> >>> >> >> 32
>> >>> >>> >> >> bit.
>> >>> >>> >> >> The warning is:
>> >>> >>> >> >>
>> >>> >>> >> >>   PPMacroExpansion.cpp
>> >>> >>> >> >> c:\llvm\include\llvm/Support/AlignOf.h(25): warning C4324:
>> >>> >>> >> >> 'llvm::AlignmentCalcImpl<T>' : structure was padded due to
>> >>> >>> >> >> __declspec(align())
>> >>> >>> >> >> [c:\llvm\msvc\tools\clang\lib\Lex\clangLex.vcxproj]
>> >>> >>> >> >>           with
>> >>> >>> >> >>           [
>> >>> >>> >> >>               T=clang::SrcMgr::ContentCache
>> >>> >>> >> >>           ] (c:\llvm\tools\clang\lib\Lex\PPCaching.cpp)
>> >>> >>> >> >>           c:\llvm\include\llvm/Support/AlignOf.h(40) : see
>> >>> >>> >> >> reference
>> >>> >>> >> >> to
>> >>> >>> >> >> class template instantiation 'llvm::AlignmentC
>> >>> >>> >> >>   alcImpl<T>' being compiled
>> >>> >>> >> >>           with
>> >>> >>> >> >>           [
>> >>> >>> >> >>               T=clang::SrcMgr::ContentCache
>> >>> >>> >> >>           ]
>> >>> >>> >> >>
>> >>> >>> >> >> c:\llvm\tools\clang\include\clang/Basic/SourceManager.h(225)
>> >>> >>> >> >> : see
>> >>> >>> >> >> reference to class template instantiation
>> >>> >>> >> >>    'llvm::AlignOf<clang::SrcMgr::ContentCache>' being
>> compiled
>> >>> >>> >> >>
>> >>> >>> >> >> referring to SourceManager.h(225):
>> >>> >>> >> >>
>> >>> >>> >> >>   static_assert(llvm::AlignOf<ContentCache>::Alignment >= 8,
>> >>> >>> >> >>                 "ContentCache must be 8-byte aligned.");
>> >>> >>> >> >>
>> >>> >>> >> >> And appears whenever SourceManager.h is included, a lot.
>> >>> >>> >> >>
>> >>> >>> >> >> The compiler is Microsoft Visual Studio Ultimate 2013,
>> Version
>> >>> >>> >> >> 12.0.31101.00 Update 4.
>> >>> >>> >> >>
>> >>> >>> >> >> I'll try threaded build now.
>> >>> >>> >> >
>> >>> >>> >> > I will do a clean rebuild from scratch; I wonder if the
>> reason
>> >>> >>> >> > I'm
>> >>> >>> >> > not
>> >>> >>> >> > seeing anything is because of some caching somewhere. Thank
>> you!
>> >>> >>> >> >
>> >>> >>> >> > ~Aaron
>> >>> >>> >
>> >>> >>> >
>> >>> >>
>> >>> >>
>> >>
>> >>
>> >
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150422/4eaf2988/attachment.html>

More information about the llvm-commits mailing list