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

Yaron Keren yaron.keren at gmail.com
Tue Apr 21 21:55:07 PDT 2015


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/8a6e28c2/attachment.html>


More information about the llvm-commits mailing list