Page MenuHomePhabricator

[clang-tblgen][CommandLine][ManagedStatic] Fix build errors with clang-tblgen in Debug mode using MSVC 2019 v16.6
ClosedPublic

Authored by ASDenysPetrov on May 22 2020, 5:32 AM.

Details

Summary

I'm using Windows. After updating MSVS19 from v16.4 to v16.6 I faced with a build errors compiling in Debug mode.
It complains on clang-tblgen.exe and llvm-tblgen.exe cmd line args, e.g.

D:\llvm-project\buildvs\Debug\bin>clang-tblgen.exe --help
clang-tblgen.exe: Unknown command line argument '--help'.  Try: 'clang-tblgen.exe --help'
clang-tblgen.exe: Did you mean '--color'?

After some debugging it turned out that VS compiler had a bug. It dynamically creates an object with constexpr ctor in Debug mode. This bug has been fixed in VS2019 v16.5.
https://developercommunity.visualstudio.com/content/problem/262083/compiler-emits-dynamic-initializer-for-variable-wi.html
But a workaround was implemented for that and everithing works until v16.5 comes.
The workaround became irrelevant since v16.5 and caused build errors.
So I disabled the workaround for VS2019 v16.5 and higher.

Now the build compiles without errors and works correctly.

Diff Detail

Event Timeline

ASDenysPetrov created this revision.May 22 2020, 5:32 AM

@lattner, @rnk, @stoklund, @bkramer, please, review the change.

RKSimon added a subscriber: RKSimon.
RKSimon added inline comments.
llvm/include/llvm/Support/ManagedStatic.h
47

Won't this no longer fire on gcc builds? Maybe this:

#if !defined(_MSC_VER) || defined(__clang__) || (defined(_MSC_VER) && (_MSC_VER >= 1925))
ASDenysPetrov marked an inline comment as done.May 22 2020, 7:50 AM
ASDenysPetrov added inline comments.
llvm/include/llvm/Support/ManagedStatic.h
47

Thank you. I somehow missed this case.
It'd be even better this:

#if !defined(_MSC_VER) || (_MSC_VER >= 1925)
aganea added a subscriber: aganea.May 22 2020, 8:09 AM
aganea added inline comments.
llvm/include/llvm/Support/ManagedStatic.h
47

@ASDenysPetrov You need the defined(__clang__) part because users can set _MSC_VER with -fmsc-version= or through auto-detection (so you could have _MSC_VER < 1925), and yet that should use the LLVM_USE_CONSTEXPR_CTOR codepath.

RKSimon added inline comments.May 22 2020, 8:10 AM
llvm/include/llvm/Support/ManagedStatic.h
47

You need the clang case to support clang-cl builds.

#if !defined(_MSC_VER) || (_MSC_VER >= 1925) || defined(__clang__)
ASDenysPetrov marked an inline comment as done.May 22 2020, 8:40 AM
ASDenysPetrov added inline comments.
llvm/include/llvm/Support/ManagedStatic.h
47

OK. Thanks for clarification. I'll take this option:

#if !defined(_MSC_VER) || (_MSC_VER >= 1925) || defined(__clang__)
lattner resigned from this revision.May 22 2020, 9:11 AM
RKSimon added inline comments.May 22 2020, 12:15 PM
llvm/include/llvm/Support/ManagedStatic.h
46

I'm not sure what the style rules are about exceeding the column-80 limit for URLs. Maybe just refer people to http://llvm.org/PR41367 ?

rnk added a comment.May 23 2020, 9:37 AM

To restate what I know, here is what I think is going on:

Once that is done, the default constructor of std::atomic<void*> becomes constexpr, but it is no longer trivial (it does zero init). ManagedStaticBase's constructor, however, cannot be constexpr unless it initializes all fields. So, if it is not constexpr, and it is not trivial, then it requires dynamic initialization, which was never desired. :( 💥

This annoyingly breaks our code, but strictly speaking, our workaround wasn't ever supposed to work (std::atomic wasn't supposed to be trivially default constructible). I suspect we are not alone, but such is life, the fix looks good to me.

llvm/include/llvm/Support/ManagedStatic.h
46

I agree with @RKSimon, this comment seems like too much detail. The original comment already explains why the code is written the way it is. The only new information is that "affected versions" are now known to be _MSC_VER < 1925, which is clearly expressed by the code. I think you can remove the comment edits. Whenever we drop support for pre-1925 versions of MSVC, someone will come back here and remove all this. The VS dev community links are also probably not super stable.

Re: URLs in comments, yes, they generally are the most common exception to the line limit style rule, but I don't think they add much value in this case.

ASDenysPetrov marked an inline comment as done.May 25 2020, 3:36 AM

here is what I think is going on:

@rnk , actually I've observed some another case:

  • In CommandLine.h some amount of arbitrary static cl::opt is registered while constructing in ManagedStatic<SubCommand> TopLevelSubCommand , that is lazy-creates an instance of ManagedStatic<SubCommand>::Ptr at first call
  • Then ManagedStatic<SubCommand> TopLevelSubCommand is actually constructed as a regular global variable, because its turn has come, thus re-init its members, particularly lost the list of previously registered cl::opt.
llvm/include/llvm/Support/ManagedStatic.h
46

Maybe just refer people to http://llvm.org/PR41367 ?
this comment seems like too much detail.

Make sense. I'll refer to PR41367. Thanks.

@RKSimon, @rnk ,
Updated according to your comments. Please, review and approve if you are OK with it. I don't want this bug block me and other people for a long.

RKSimon accepted this revision.May 25 2020, 4:47 AM

LGTM - cheers

This revision is now accepted and ready to land.May 25 2020, 4:47 AM
This revision was automatically updated to reflect the committed changes.