Kill the LLVM global lock

[zturner]

Summary

This patch kills the LLVM global lock. This was being used in only two places, neither of which required exclusivity from each other, so they have been independently changed to use their own separate mutexes.

[majnemer]

include/llvm/Support/Threading.h

21 ↗

(On Diff #10410)

The & should bind to the function name, not the return type.

[zturner]

Gah, again. I need to start using clang-format. Thanks for catching this.

[zturner]

Fix the binding of the reference operator.

[chandlerc]

lib/Support/Threading.cpp

25 ↗

(On Diff #10411)

Is the constructor for a mutex threadsafe? Because we use VS versions which don't provide a thread safe initialization. Actually, even if the constructor is threadsafe, won't that still form a data race if we call this from two threads when built with MSVC?

I think you'll need this to be a static global ultimately... =/

[zturner]

lib/Support/Threading.cpp

25 ↗

(On Diff #10411)

I should probably put a comment here, but the problem with that is that static constructors can trigger instantiation of ManagedStatics. In this particular CL, that will be fine, because when a ManagedStatic is instantiated during a static constructor, llvm_is_multithreaded() will be false.

But after the rest goes through, multi-threading will always be on, and if the mutex is a global static we won't be able to guarantee that the mutex's constructor will have been called. By definining it in the function, we can enforce the ordering.

By the same logic though, since we call this from static constructors, we know that it will be initialized in a thread-safe fashion. But this is only by chance (since we know we use static constructors), so perhaps we should leave a comment indicating this so that if / when we get entirely rid of static constructors, we will know to go back and fix this.

[chandlerc]

lib/Support/Threading.cpp

25 ↗

(On Diff #10411)

I don't think that we can reasonably require that this public routine only be called from static constructors and thus already be serialized. Instead, we need to make this reliable in some way. I suspect that the right way is through a global pointer to a mutex and a pthread_once (or analogous) call to a function which allocates the mutex on the heap and sets the pointer to it. We can then "leak" the mutex freely because it isn't actually leaked at all and there will be only one...

But I feel like we're spending too much time on an abstraction that has no utility.

We can remove the usage of this from Timer.cpp by re-using the existing ManagedStatic-based mutex that file already has.

Now we just need ManagedStatic to bootstrap itself a mutex to use, and everything else can rely on it. My suggestion is to sink the pthread_once-style allocation of a single mutex completely into the ManagedStatic code. If necessary, we can even have a Once.inc in Unix/ and Windows/, but I see no reason to really surface the API to other users.

What I wouldn't give for thread-safe function statics...

[zturner]

Remove the global lock entirely, and bootstrap a call_once initialized' mutex in ManagedStatic that is safe for use during static construction.

[chandlerc]

This patch (attached to the email, and in Phab) still contains all the
changes to stop using multithreading as a runtime parameter?

[zturner]

Fourth time's a charm.

This patch splits the CL out so that the only thing it does is kill the global lock.

[zturner]

Fix the comment about static initialization.

[rnk]

lgtm

lib/Support/ManagedStatic.cpp

32 ↗

(On Diff #10511)

The new function naming convention is camelCase over StudlyCaps:
http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

The confusion is understandable, because the existing code is very inconsistent.

[zturner]

Thanks! Will wait for Chandler's lgtm as well before I push, since he's
had a bunch of comments on past iterations, want to make sure all of his
concerns are addressed as well.

[zturner]

rnk's recommendation offline is to go ahead and submit this, given that it seems like all of Chandler's concerns have been addressed. At the very least, this should be close enough to addressing all of Chandler's concerns that if there is anything outstanding, we can address it in a follow up.

[zturner]

Closed by commit rL211277 (authored by @zturner).

[chandlerc]

FYI, LGTM too.

llvm/trunk/docs/ProgrammersManual.rst

 invocation of ``llvm_shutdown()``, it implements a simple lazy initialization
 scheme.  Once ``llvm_start_multithreaded()`` returns, however, it uses
 double-checked locking to implement thread-safe lazy initialization.
 
 Note that, because no other threads are allowed to issue LLVM API calls before
 ``llvm_start_multithreaded()`` returns, it is possible to have
 ``ManagedStatic``\ s of ``llvm::sys::Mutex``\ s.
 
-The ``llvm_acquire_global_lock()`` and ``llvm_release_global_lock`` APIs provide
-access to the global lock used to implement the double-checked locking for lazy
-initialization.  These should only be used internally to LLVM, and only if you
-know what you're doing!
-
 .. _llvmcontext:
 
 Achieving Isolation with ``LLVMContext``
 ----------------------------------------
 
 ``LLVMContext`` is an opaque class in the LLVM API which clients can use to
 operate multiple, isolated instances of LLVM concurrently within the same
 address space.  For instance, in a hypothetical compile-server, the compilation

llvm/trunk/include/llvm/Support/Threading.h

namespace llvm {
   /// safe for multithreading.
   /// THIS MUST EXECUTE IN ISOLATION FROM ALL OTHER LLVM API CALLS.
   void llvm_stop_multithreaded();
 
   /// llvm_is_multithreaded - Check whether LLVM is executing in thread-safe
   /// mode or not.
   bool llvm_is_multithreaded();
 
-  /// acquire_global_lock - Acquire the global lock.  This is a no-op if called
-  /// before llvm_start_multithreaded().
-  void llvm_acquire_global_lock();
-
-  /// release_global_lock - Release the global lock.  This is a no-op if called
-  /// before llvm_start_multithreaded().
-  void llvm_release_global_lock();
-
   /// 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);
 }
 
 #endif

llvm/trunk/lib/Support/ManagedStatic.cpp

 // This file implements the ManagedStatic class and llvm_shutdown().
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Config/config.h"
 #include "llvm/Support/Atomic.h"
 #include <cassert>
+#include <mutex>
 using namespace llvm;
 
 static const ManagedStaticBase *StaticList = nullptr;
 
+// ManagedStatics can get created during execution of static constructors.  As a
+// result, we cannot use a global static std::mutex object for the lock since it
+// may not have been constructed.  Instead, we do a call-once initialization of
+// a pointer to a mutex.
+static std::once_flag MutexInitializationFlag;
+static std::recursive_mutex* ManagedStaticMutex = nullptr;
+
+// Not all supported platforms (in particular VS2012) have thread-safe function
+// static initialization, so roll our own.
+static std::recursive_mutex& GetManagedStaticMutex() {
+  std::call_once(MutexInitializationFlag,
+      []() { ManagedStaticMutex = new std::recursive_mutex(); } );
+
+  return *ManagedStaticMutex;
+}
+
 void ManagedStaticBase::RegisterManagedStatic(void *(*Creator)(),
                                               void (*Deleter)(void*)) const {
   assert(Creator);
   if (llvm_is_multithreaded()) {
-    llvm_acquire_global_lock();
+    std::lock_guard<std::recursive_mutex> Lock(GetManagedStaticMutex());
 
     if (!Ptr) {
       void* tmp = Creator();
 
       TsanHappensBefore(this);
       sys::MemoryFence();
 
       // This write is racy against the first read in the ManagedStatic
       // accessors. The race is benign because it does a second read after a
       // memory fence, at which point it isn't possible to get a partial value.
       TsanIgnoreWritesBegin();
       Ptr = tmp;
       TsanIgnoreWritesEnd();
       DeleterFn = Deleter;
       
       // Add to list of managed statics.
       Next = StaticList;
       StaticList = this;
     }
-
-    llvm_release_global_lock();
   } else {
     assert(!Ptr && !DeleterFn && !Next &&
            "Partially initialized ManagedStatic!?");
     Ptr = Creator();
     DeleterFn = Deleter;
   
     // Add to list of managed statics.
     Next = StaticList;
void ManagedStaticBase::destroy() const {
   
   // Cleanup.
   Ptr = nullptr;
   DeleterFn = nullptr;
 }
 
 /// llvm_shutdown - Deallocate and destroy all ManagedStatic variables.
 void llvm::llvm_shutdown() {
+  std::lock_guard<std::recursive_mutex> Lock(GetManagedStaticMutex());
+
   while (StaticList)
     StaticList->destroy();
 
   if (llvm_is_multithreaded()) llvm_stop_multithreaded();
 }

llvm/trunk/lib/Support/Threading.cpp

 #include "llvm/Support/Atomic.h"
 #include "llvm/Support/Mutex.h"
 #include <cassert>
 
 using namespace llvm;
 
 static bool multithreaded_mode = false;
 
-static sys::Mutex* global_lock = nullptr;
-
 bool llvm::llvm_start_multithreaded() {
 #if LLVM_ENABLE_THREADS != 0
   assert(!multithreaded_mode && "Already multithreaded!");
   multithreaded_mode = true;
-  global_lock = new sys::Mutex(true);
 
   // We fence here to ensure that all initialization is complete BEFORE we
   // return from llvm_start_multithreaded().
   sys::MemoryFence();
   return true;
 #else
   return false;
 #endif
 }
 
 void llvm::llvm_stop_multithreaded() {
 #if LLVM_ENABLE_THREADS != 0
   assert(multithreaded_mode && "Not currently multithreaded!");
 
   // We fence here to insure that all threaded operations are complete BEFORE we
   // return from llvm_stop_multithreaded().
   sys::MemoryFence();
 
   multithreaded_mode = false;
-  delete global_lock;
 #endif
 }
 
 bool llvm::llvm_is_multithreaded() {
   return multithreaded_mode;
 }
 
-void llvm::llvm_acquire_global_lock() {
-  if (multithreaded_mode) global_lock->acquire();
-}
-
-void llvm::llvm_release_global_lock() {
-  if (multithreaded_mode) global_lock->release();
-}
-
 #if LLVM_ENABLE_THREADS != 0 && defined(HAVE_PTHREAD_H)
 #include <pthread.h>
 
 struct ThreadInfo {
   void (*UserFn)(void *);
   void *UserData;
 };
 static void *ExecuteOnThread_Dispatch(void *Arg) {

llvm/trunk/lib/Support/Timer.cpp

 #include "llvm/Support/Timer.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/ManagedStatic.h"
 #include "llvm/Support/Mutex.h"
+#include "llvm/Support/MutexGuard.h"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/raw_ostream.h"
 using namespace llvm;
 
 // CreateInfoOutputFile - Return a file stream to print our output on.
 namespace llvm { extern raw_ostream *CreateInfoOutputFile(); }
 
 // getLibSupportInfoOutputFilename - This ugly hack is brought to you courtesy

 
 
 static TimerGroup *DefaultTimerGroup = nullptr;
 static TimerGroup *getDefaultTimerGroup() {
   TimerGroup *tmp = DefaultTimerGroup;
   sys::MemoryFence();
   if (tmp) return tmp;
   
-  llvm_acquire_global_lock();
+  sys::SmartScopedLock<true> Lock(*TimerLock);
   tmp = DefaultTimerGroup;
   if (!tmp) {
     tmp = new TimerGroup("Miscellaneous Ungrouped Timers");
     sys::MemoryFence();
     DefaultTimerGroup = tmp;
   }
-  llvm_release_global_lock();
 
   return tmp;
 }
 
 //===----------------------------------------------------------------------===//
 // Timer Implementation
 //===----------------------------------------------------------------------===//