[lld] 564481a - [Support] ThreadPoolExecutor fixes for Windows/MinGW

[Andrew Ng via llvm-commits]

Author: Andrew Ng
Date: 2020-01-10T12:44:01Z
New Revision: 564481aebe18a723c9cfe9ea9ca5808771f7e9d8

URL: https://github.com/llvm/llvm-project/commit/564481aebe18a723c9cfe9ea9ca5808771f7e9d8
DIFF: https://github.com/llvm/llvm-project/commit/564481aebe18a723c9cfe9ea9ca5808771f7e9d8.diff

LOG: [Support] ThreadPoolExecutor fixes for Windows/MinGW

Changed ThreadPoolExecutor to no longer use detached threads and instead
to join threads on destruction. This is to prevent intermittent crashing
on Windows when doing a normal full exit, e.g. via exit().

Changed ThreadPoolExecutor to be a ManagedStatic so that it can be
stopped on llvm_shutdown(). Without this, it would only be stopped in
the destructor when doing a full exit. This is required to avoid
intermittent crashing on Windows due to a race condition between the
ThreadPoolExecutor starting up threads and the process doing a fast
exit, e.g. via _exit().

The Windows crashes appear to only occur with the MSVC static runtimes
and are more frequent with the debug static runtime.

These changes also prevent intermittent deadlocks on exit with the MinGW
runtime.

Differential Revision: https://reviews.llvm.org/D70447

Added: 
    

Modified: 
    lld/Common/ErrorHandler.cpp
    llvm/lib/Support/Parallel.cpp

Removed: 
    


################################################################################
diff  --git a/lld/Common/ErrorHandler.cpp b/lld/Common/ErrorHandler.cpp
index 91ea6799907e..b6066b557cbf 100644
--- a/lld/Common/ErrorHandler.cpp
+++ b/lld/Common/ErrorHandler.cpp
@@ -56,9 +56,10 @@ void lld::exitLld(int val) {
   if (errorHandler().outputBuffer)
     errorHandler().outputBuffer->discard();
 
-  // Dealloc/destroy ManagedStatic variables before calling
-  // _exit(). In a non-LTO build, this is a nop. In an LTO
-  // build allows us to get the output of -time-passes.
+  // Dealloc/destroy ManagedStatic variables before calling _exit().
+  // In an LTO build, allows us to get the output of -time-passes.
+  // Ensures that the thread pool for the parallel algorithms is stopped to
+  // avoid intermittent crashes on Windows when exiting.
   llvm_shutdown();
 
   lld::outs().flush();

diff  --git a/llvm/lib/Support/Parallel.cpp b/llvm/lib/Support/Parallel.cpp
index 355c64b7d079..523665d14b02 100644
--- a/llvm/lib/Support/Parallel.cpp
+++ b/llvm/lib/Support/Parallel.cpp
@@ -8,14 +8,17 @@
 
 #include "llvm/Support/Parallel.h"
 #include "llvm/Config/llvm-config.h"
+#include "llvm/Support/ManagedStatic.h"
 
 #if LLVM_ENABLE_THREADS
 
 #include "llvm/Support/Threading.h"
 
 #include <atomic>
+#include <future>
 #include <stack>
 #include <thread>
+#include <vector>
 
 namespace llvm {
 namespace parallel {
@@ -36,30 +39,53 @@ class Executor {
 ///   in filo order.
 class ThreadPoolExecutor : public Executor {
 public:
-  explicit ThreadPoolExecutor(unsigned ThreadCount = hardware_concurrency())
-      : Done(ThreadCount) {
+  explicit ThreadPoolExecutor(unsigned ThreadCount = hardware_concurrency()) {
     // Spawn all but one of the threads in another thread as spawning threads
     // can take a while.
-    std::thread([&, ThreadCount] {
-      for (size_t i = 1; i < ThreadCount; ++i) {
-        std::thread([=] { work(); }).detach();
+    Threads.reserve(ThreadCount);
+    Threads.resize(1);
+    std::lock_guard<std::mutex> Lock(Mutex);
+    Threads[0] = std::thread([&, ThreadCount] {
+      for (unsigned i = 1; i < ThreadCount; ++i) {
+        Threads.emplace_back([=] { work(); });
+        if (Stop)
+          break;
       }
+      ThreadsCreated.set_value();
       work();
-    }).detach();
+    });
   }
 
-  ~ThreadPoolExecutor() override {
-    std::unique_lock<std::mutex> Lock(Mutex);
-    Stop = true;
-    Lock.unlock();
+  void stop() {
+    {
+      std::lock_guard<std::mutex> Lock(Mutex);
+      if (Stop)
+        return;
+      Stop = true;
+    }
     Cond.notify_all();
-    // Wait for ~Latch.
+    ThreadsCreated.get_future().wait();
   }
 
+  ~ThreadPoolExecutor() override {
+    stop();
+    std::thread::id CurrentThreadId = std::this_thread::get_id();
+    for (std::thread &T : Threads)
+      if (T.get_id() == CurrentThreadId)
+        T.detach();
+      else
+        T.join();
+  }
+
+  struct Deleter {
+    static void call(void *Ptr) { ((ThreadPoolExecutor *)Ptr)->stop(); }
+  };
+
   void add(std::function<void()> F) override {
-    std::unique_lock<std::mutex> Lock(Mutex);
-    WorkStack.push(F);
-    Lock.unlock();
+    {
+      std::lock_guard<std::mutex> Lock(Mutex);
+      WorkStack.push(F);
+    }
     Cond.notify_one();
   }
 
@@ -75,19 +101,39 @@ class ThreadPoolExecutor : public Executor {
       Lock.unlock();
       Task();
     }
-    Done.dec();
   }
 
   std::atomic<bool> Stop{false};
   std::stack<std::function<void()>> WorkStack;
   std::mutex Mutex;
   std::condition_variable Cond;
-  parallel::detail::Latch Done;
+  std::promise<void> ThreadsCreated;
+  std::vector<std::thread> Threads;
 };
 
 Executor *Executor::getDefaultExecutor() {
-  static ThreadPoolExecutor exec;
-  return &exec;
+  // The ManagedStatic enables the ThreadPoolExecutor to be stopped via
+  // llvm_shutdown() which allows a "clean" fast exit, e.g. via _exit(). This
+  // stops the thread pool and waits for any worker thread creation to complete
+  // but does not wait for the threads to finish. The wait for worker thread
+  // creation to complete is important as it prevents intermittent crashes on
+  // Windows due to a race condition between thread creation and process exit.
+  //
+  // The ThreadPoolExecutor will only be destroyed when the static unique_ptr to
+  // it is destroyed, i.e. in a normal full exit. The ThreadPoolExecutor
+  // destructor ensures it has been stopped and waits for worker threads to
+  // finish. The wait is important as it prevents intermittent crashes on
+  // Windows when the process is doing a full exit.
+  //
+  // The Windows crashes appear to only occur with the MSVC static runtimes and
+  // are more frequent with the debug static runtime.
+  //
+  // This also prevents intermittent deadlocks on exit with the MinGW runtime.
+  static ManagedStatic<ThreadPoolExecutor, object_creator<ThreadPoolExecutor>,
+                       ThreadPoolExecutor::Deleter>
+      ManagedExec;
+  static std::unique_ptr<ThreadPoolExecutor> Exec(&(*ManagedExec));
+  return Exec.get();
 }
 } // namespace