LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 41367 - LLVM's ManagedStatic.h fails with VS 2019 <atomic> and clang-cl due to init-order-fiasco and constexpr confusion
Summary: LLVM's ManagedStatic.h fails with VS 2019 <atomic> and clang-cl due to init-o...
Status: RESOLVED FIXED
Alias: None
Product: clang
Classification: Unclassified
Component: C++ (show other bugs)
Version: unspecified
Hardware: PC Windows NT
: P enhancement
Assignee: Reid Kleckner
URL:
Keywords:
: 46108 49027 (view as bug list)
Depends on:
Blocks: release-10.0.1
  Show dependency tree
 
Reported: 2019-04-03 16:27 PDT by Reid Kleckner
Modified: 2021-02-04 12:09 PST (History)
13 users (show)

See Also:
Fixed By Commit(s): 46e5c5fe778b 28a6713e107


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Reid Kleckner 2019-04-03 16:27:04 PDT
I installed VS 2019 and tried to self-host clang with the new headers, but it didn't work. The symptom was this:

[1 processes, 17/531 @ 3.4/s : 4.984s ] Building AttributesCompatFunc.inc...
FAILED: lib/IR/AttributesCompatFunc.inc
cmd.exe /C "cd /D C:\src\llvm-project\build && C:\src\llvm-project\build\bin\llvm-tblgen.exe -gen-attrs -I C:/src/llvm-project/llvm/lib/IR -I C:/src/llvm-project/llvm/include C:/src/llvm-project/llvm/lib/IR/AttributesCompatFunc.td -o lib/IR/AttributesCompatFunc.inc -d lib/IR/AttributesCompatFunc.inc.d"
llvm-tblgen.exe: Unknown command line argument '-gen-attrs'.  Try: 'C:\src\llvm-project\build\bin\llvm-tblgen.exe -help'
llvm-tblgen.exe: Did you mean '-stats'?
ninja: build stopped: subcommand failed.

The issue here is that command line options aren't being registered. This is because TopLevelSubCommand is a ManagedStatic<SubCommand>. What happens is that the cl::opt initializers run first, then ManagedStatic is incorrectly *not* linker initialized, we run the ManagedStatic ctor, and we zero out the pointer to the previously constructed list of command line flags.

The issue is that ManagedStaticBase wants to be linker initialized, but the way it achieves that is by not having a constructor at all. It has members that look like this:

class ManagedStaticBase {
protected:
  // This should only be used as a static variable, which guarantees that this
  // will be zero initialized.
  mutable std::atomic<void *> Ptr;
  mutable void (*DeleterFn)(void*);
  mutable const ManagedStaticBase *Next;

With clang-cl, for some reason the default constructor for this class isn't considered to be constexpr. When I try to explicitly make it constexpr, clang complains:
C:\src\llvm-project\llvm\include\llvm/Support/ManagedStatic.h(47,3): error: defaulted definition of default constructor is not constexpr
  constexpr ManagedStaticBase() = default;
  ^
1 error generated.

So, clang thinks the defaulted one won't be constexpr, but it doesn't say why. Even if it's correct, it should probably point the user at the uninitialized members. If I initialize the members explicitly like this:

  mutable std::atomic<void *> Ptr{nullptr};
  mutable void (*DeleterFn)(void*) = nullptr;
  mutable const ManagedStaticBase *Next = nullptr;

Then the defaulted default ctor becomes constexpr and everything works as expected.

The atomic headers have some interesting ifdefs that seem to allude to this issue:

struct _Atomic_base : _Atomic_impl<_Bytes> {
  ...
#ifdef __clang__ // TRANSITION, VSO#406237
    constexpr _Atomic_base() _NOEXCEPT_COND(is_nothrow_default_constructible_v<_Ty>) : _My_val() {}
#else // ^^^ no workaround ^^^ // vvv workaround vvv
    _Atomic_base() = default;
#endif // VSO#406237

The odd thing is that if this constexpr default ctor didn't exist, we might be in business? Hard to say.

Another way of looking at this bug, if it's a clang bug, is, why is the following an error?

struct atomic_ptr {
  constexpr atomic_ptr() = default;
  void *Ptr = nullptr;
};
struct Foo {
  atomic_ptr Ptr;
  int y;
  constexpr Foo() = default; // why an error?
};
extern Foo obj;
Foo obj;

Just because 'y' has no initializer, Foo can't be constexpr, even if I ask for it to be?

In the end, I *think* this is a clang bug, but it might be an STL bug, so I added Billy and STL.

Anyway, there's an obvious workaround, which is to add a real constexpr constructor to ManagedStatic, assuming that works with all the toolchains we support. I'm going to try that.
Comment 1 Reid Kleckner 2019-04-03 16:57:41 PDT
After discussing this further here, we think this is definitely an LLVM bug, not a clang bug or a VC STL bug.
Comment 2 Billy O'Neal 2019-04-03 18:43:46 PDT
VSO#406237 is "C1XX does not do constant initialization where the standard requires". Basically, the compiler doesn't do constant initialization unless you are declaring an instance of a thing which is itself actually constexpr. This let us implement P0883 for clang but not for c1xx yet.

Billy O'Neal
Visual C++ Libraries
Comment 3 Simon Pilgrim 2019-04-04 03:38:34 PDT
Reid - after rL357655 I'm seeing a very similar llvm-tblgen error on VS2017 - I've only been able to repro on msbuild debug builds so far.
Comment 4 Simon Pilgrim 2019-04-04 04:14:18 PDT
(In reply to Simon Pilgrim from comment #3)
> Reid - after rL357655 I'm seeing a very similar llvm-tblgen error on VS2017
> - I've only been able to repro on msbuild debug builds so far.

Reverted in rL357685
Comment 5 Reid Kleckner 2019-04-04 10:35:48 PDT
(In reply to Billy O'Neal from comment #2)
> VSO#406237 is "C1XX does not do constant initialization where the standard
> requires". Basically, the compiler doesn't do constant initialization unless
> you are declaring an instance of a thing which is itself actually constexpr.
> This let us implement P0883 for clang but not for c1xx yet.
> 
> Billy O'Neal
> Visual C++ Libraries

(In reply to Simon Pilgrim from comment #3)
> Reid - after rL357655 I'm seeing a very similar llvm-tblgen error on VS2017
> - I've only been able to repro on msbuild debug builds so far.

So, if I understand correctly, my fix, rL357655, ran into exactly the issue that Billy is describing with the Visual C++ compiler from VS 2017. =/ I have yet to find a proper solution.
Comment 6 Reid Kleckner 2019-07-15 16:40:01 PDT
I relanded a fix back in April in r359135.
Comment 7 Denys Petrov 2020-05-22 09:57:54 PDT
There was a bug of constexpr ctor in VS2019 compiler until ver 16.5, that really caused the problem.
It is fixed now. https://developercommunity.visualstudio.com/content/problem/262083/compiler-emits-dynamic-initializer-for-variable-wi.html
I've provided a fix for LLVM https://reviews.llvm.org/D80433.
Comment 8 Simon Pilgrim 2020-05-22 11:09:14 PDT
Reopened to deal with the 16.5+ case
Comment 9 Denys Petrov 2020-05-25 07:10:51 PDT
The fix for MSVC v16.5+ loaded.
Comment 10 Simon Pilgrim 2020-05-27 05:36:08 PDT
This affects building the 10.0 release build with VS 16.6:

https://llvm.discourse.group/t/llvm-not-building-in-vusual-studio/1139

I'd like to propose we merge https://reviews.llvm.org/D80433 / rG46e5c5fe778b into the branch.
Comment 11 Denys Petrov 2020-05-27 06:03:40 PDT
Simon, I'm reletively a newcomer. Could you please explain how to do that?
Comment 12 Simon Pilgrim 2020-05-27 11:18:55 PDT
(In reply to Denys Petrov from comment #11)
> Simon, I'm reletively a newcomer. Could you please explain how to do that?

We just need to get a consensus on this ticket that rG46e5c5fe778b should be merged into the 10.X release branch and Tom will handle the actual merge (by blocking [Bug #45309] he has visibility on merge candidates).

Reid - any comments?
Comment 13 Eli Friedman 2020-05-27 14:23:15 PDT
*** Bug 46108 has been marked as a duplicate of this bug. ***
Comment 14 Tom Stellard 2020-06-11 10:22:44 PDT
Reid, is this OK to backport?
Comment 15 Tom Stellard 2020-06-23 15:56:39 PDT
This looks safe to me, so I'm going to merge this.
Comment 16 Tom Stellard 2020-06-23 16:05:15 PDT
Merged: 28a6713e107
Comment 17 Reid Kleckner 2021-02-04 12:09:59 PST
*** Bug 49027 has been marked as a duplicate of this bug. ***