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

[ASDenysPetrov]

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.

[ASDenysPetrov]

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

[RKSimon]

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]

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]

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]

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]

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__)

[ASDenysPetrov]

Updated

[RKSimon]

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]

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

• VC++ used to have a constexpr constructor compiler bug.
• The MSVC STL worked around it in std::atomic, this was tracked by the internal number VSO-406237
• C1XX fixed the constexpr bug
• The MSVC STL removed the work around in this github pull request: https://github.com/microsoft/STL/issues/336 / diff https://github.com/microsoft/STL/pull/390/commits/92d1e1f4d9b2c0c52b145adb598f0e27ff0203d2

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]

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.

[ASDenysPetrov]

@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]

LGTM - cheers

llvm/include/llvm/Support/ManagedStatic.h

 // ManagedStatic must be initialized to zero, and it must *not* have a dynamic
 // initializer because managed statics are often created while running other
 // dynamic initializers. In standard C++11, the best way to accomplish this is
 // with a constexpr default constructor. However, different versions of the
 // Visual C++ compiler have had bugs where, even though the constructor may be
 // constexpr, a dynamic initializer may be emitted depending on optimization
 // settings. For the affected versions of MSVC, use the old linker
 // initialization pattern of not providing a constructor and leaving the fields
-// uninitialized.
-#if !defined(_MSC_VER) || defined(__clang__)
+// uninitialized. See http://llvm.org/PR41367 for details.
+#if !defined(_MSC_VER) || (_MSC_VER >= 1925) || defined(__clang__)
 #define LLVM_USE_CONSTEXPR_CTOR
 #endif
 
 /// ManagedStaticBase - Common base class for ManagedStatic instances.
 class ManagedStaticBase {
 protected:
 #ifdef LLVM_USE_CONSTEXPR_CTOR
   mutable std::atomic<void *> Ptr{};
   mutable void (*DeleterFn)(void *) = nullptr;
   mutable const ManagedStaticBase *Next = nullptr;
 #else