Removing the static destructor from ManagedStatic.cpp by controlling the allocation and de-allocaation of the mutex.

[beanz]

Summary

This is part of the ongoing work to remove static constructors and destructors.

[chandlerc]

lib/Support/ManagedStatic.cpp

35

Uh... How is this safe? How does this bootstrap when it is called while acquiring a lock (and thus concurrently without any other synchronization)?

[beanz]

lib/Support/ManagedStatic.cpp

35

It isn't, but since MSVC doesn't implement thread-safe static local initialization I figured this wasn't any worse off.

Do you have an alternate suggestion? I know in other places in LLVM we avoid the static destructor by just leaking the pointer (which I don't like). I could also static init the pointer, and still control deleting it, but I'm not sure that gets us much.

Thoughts?

[chandlerc]

lib/Support/ManagedStatic.cpp

35

I don't know how we've not been bitten by this before. It certainly was always unsafe on Windows, but historically was safe on Linux and Mac. =/

I think the correct fix here is to just platform specific code here. Rewrite this as an initialization function, and then use pthreads if we have it or C++11 if not (so MinGW's defective C++11 <mutex> and <thread> implementation can be coped with) to do a 'run_once' or 'once_init' or however it is spelled in a threadsafe way.

I'm happy to "leak" the single mutex provided we initialize it in a thread safe way.

[rnk]

lib/Support/ManagedStatic.cpp

35

The problematic platform is MinGW with native win32 threads. This platform has neither pthreads nor C++11 <mutex>. The only way to implement initialization here is to use the native win32 one-time initialization:
http://msdn.microsoft.com/en-us/library/windows/desktop/ms686934(v=vs.85).aspx

I think I have the right bits to test this configuration now. If you send a patch I can fill in the blanks and send it back.

[beanz]

This revision of the patch handles the thread safe initialization concerns that were raised by Chandler and Reid. The approach to handling the thread safe initialization is the implementation of a new LLVM_CALL_ONCE macro which has implementations using C++11 call_once, pthread_once, and sys::CompareAndSwap.

The LLVM_CALL_ONCE macro could alternatively be implemented as a function taking two pointer parameters, one to the function to execute, and the other to the flag.

[rnk]

configure

9628 ↗

(On Diff #14532)

Did you modify autoconf/configure.ac to produce this, or write it out yourself based on the pthread_mutex_init code above? We should keep our changes in the .ac file and regenerate this with the appropriate autoconf. Eric can do that if you don't have the right bits.

include/llvm/Support/Threading.h

18

We can only include llvm/Config/config.h from .cpp files, not .h files.

24

We take a function pointer as a template parameter instead of using macros. We'll get different static data members / static locals between different instantiations that way, which is what the macros appear to be solving.

47

I don't think we want to commit this double checked locking. I think we can simplify this to:

if Windows:
  use InitOnceExecuteOnce in lib/Support/Windows/Threading.inc
else:
  use a C++11 thread-safe static local in lib/Support/Unix/Threading.inc

lib/Support/ManagedStatic.cpp

99–101

This is a semantic change, but it can only break in ways that we probably don't support, ie racily shutting down LLVM while someone is trying to access a ManagedStatic. Seems fine. :)

[beanz]

I'm working on patch revisions, but I do have some comments inline below.

configure

9628 ↗

(On Diff #14532)

I'll have Eric review these patches. My autoconf foo is severely lacking.

include/llvm/Support/Threading.h

18

Really? We do include it in headers like Unix.h and WindowsSupport.h. I'm not really sure how to implement this without access to the HAVE_PTHREAD_ONCE define.

24

This would absolutely be a cleaner way to do this.

47

I'm not sure this is the right breakdown. I believe MSVC has std::call_once, so I was hoping to do a breakdown based on the availability of <mutex>, and pthread_once.

[beanz]

This update has the following changes:

• Based on discussions about the avaialability of <mutex> I've removed the pthread implementation and all the autoconf/cmake associated changes.
• Moved away from macros to a template implementation
• All platforms other than MingW32 use std::call_once, MingW32 uses the hand-rolled solution.

[rnk]

Thanks, I think this is almost done.

include/llvm/Support/Threading.h

18

I should've said .h files in llvm/include/, since those are exported to users, while config.h is not installed. The .h files in llvm/lib/ like Unix.h can use config.h.

47

That won't cover the mingw64 with no pthread emulation case, though. I think we can use thread-safe statics in that case, though.

51–69

mingw64 should have thread safe statics, so I think this can just be:

struct InitOnceWrapper {
  InitOnceWrapper() { UserFn(); }
};
static InitOnceWrapper InitOnceVar;

I don't actually understand memory barriers, so I'd rather rely on standard primitives.

[beanz]

include/llvm/Support/Threading.h

51–69

Doesn't MingW64 have <mutex>? I think we only need to fall back to the custom code on MingW32.

[beanz]

Updated to have all non-MingW platforms use std::call_once, and MingW platforms use thread safe static initialization.

[rnk]

Thanks! lgtm

[chandlerc]

include/llvm/Support/Threading.h

44

You really need some documentation for how to use this correctly...

45

I would remove the extraneous blank lines here.

Closed by commit rL219640 (authored by cbieneman).

include/llvm/Support/Threading.h

 // This file declares helper functions for running LLVM in a multi-threaded
 // environment.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_THREADING_H
 #define LLVM_SUPPORT_THREADING_H
 
+#if !defined(__MINGW32__)
+#include <mutex>
+#else
+#include "llvm/Support/Atomic.h"
+#include "llvm/Support/Valgrind.h"
+#endif
+
 namespace llvm {
   /// Returns true if LLVM is compiled with support for multi-threading, and
   /// false otherwise.
   bool llvm_is_multithreaded();
 
   /// llvm_execute_on_thread - Execute the given \p UserFn on a separate
   /// thread, passing it the provided \p UserData.
   ///
   /// This function does not guarantee that the code will actually be executed
   /// on a separate thread or honoring the requested stack size, but tries to do
   /// so where system support is available.
   ///
   /// \param UserFn - The callback to execute.
   /// \param UserData - An argument to pass to the callback function.
   /// \param RequestedStackSize - If non-zero, a requested size (in bytes) for
   /// the thread stack.
   void llvm_execute_on_thread(void (*UserFn)(void*), void *UserData,
                               unsigned RequestedStackSize = 0);
+
+template <void (*UserFn)(void)> void llvm_call_once() {
+
+#if !defined(__MINGW32__)
+  static std::once_flag flag;
+  std::call_once(flag, UserFn);
+
+#else
+  static volatile sys::cas_flag initialized = 0;
+  sys::cas_flag old_val = sys::CompareAndSwap(&initialized, 1, 0);
+  if (old_val == 0) {
+    UserFn();
+    sys::MemoryFence();
+    TsanIgnoreWritesBegin();
+    TsanHappensBefore(&initialized);
+    initialized = 2;
+    TsanIgnoreWritesEnd();
+  } else {
+    sys::cas_flag tmp = initialized;
+    sys::MemoryFence();
+    while (tmp != 2) {
+      tmp = initialized;
+      sys::MemoryFence();
+    }
+  }
+  TsanHappensAfter(&initialized);
+#endif
+}
 }
 
 #endif

lib/Support/ManagedStatic.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Config/config.h"
 #include "llvm/Support/Atomic.h"
 #include "llvm/Support/Mutex.h"
 #include "llvm/Support/MutexGuard.h"
+#include "llvm/Support/Threading.h"
 #include <cassert>
 using namespace llvm;
 
 static const ManagedStaticBase *StaticList = nullptr;
+static sys::Mutex *ManagedStaticMutex = nullptr;
 
-static sys::Mutex& getManagedStaticMutex() {
+static void initializeMutex() {
+  ManagedStaticMutex = new sys::Mutex();
+}
+
+static sys::Mutex* getManagedStaticMutex() {
   // We need to use a function local static here, since this can get called
   // during a static constructor and we need to guarantee that it's initialized
   // correctly.
-  static sys::Mutex ManagedStaticMutex;
+  llvm_call_once<initializeMutex>();
   return ManagedStaticMutex;
 }
 
 void ManagedStaticBase::RegisterManagedStatic(void *(*Creator)(),
                                               void (*Deleter)(void*)) const {
   assert(Creator);
   if (llvm_is_multithreaded()) {
-    MutexGuard Lock(getManagedStaticMutex());
+    MutexGuard Lock(*getManagedStaticMutex());
 
     if (!Ptr) {
       void* tmp = Creator();
 
       TsanHappensBefore(this);
       sys::MemoryFence();
 
       // This write is racy against the first read in the ManagedStatic
void ManagedStaticBase::destroy() const {
   
   // Cleanup.
   Ptr = nullptr;
   DeleterFn = nullptr;
 }
 
 /// llvm_shutdown - Deallocate and destroy all ManagedStatic variables.
 void llvm::llvm_shutdown() {
-  MutexGuard Lock(getManagedStaticMutex());
+  {
+    MutexGuard Lock(*getManagedStaticMutex());
 
     while (StaticList)
       StaticList->destroy();
   }
+
+  delete ManagedStaticMutex;
+  ManagedStaticMutex = nullptr;
+}