[Support] Add a way to run a function on a detached thread

[Dmitry.Kozhevnikov]

Summary

This roughly mimics std::thread(...).detach() except it allows to customize the stack size. Required for https://reviews.llvm.org/D50993.

I've decided against reusing the existing llvm_execute_on_thread because it's not obvious what to do with the ownership of the passed function/arguments:

1. If we pass possibly owning functions data to llvm_execute_on_thread, we'll lose the ability to pass small non-owning non-allocating functions for the joining case (as it's used now). Is it important enough?
2. If we use the non-owning interface in the new use case, we'll force clients to transfer ownership to the spawned thread manually, but similar code would still have to exist inside llvm_execute_on_thread(_async) anyway (as we can't just pass the same non-owning pointer to pthreads and Windows implementations, and would be forced to wrap it in some structure, and deal with its ownership.

[zturner]

I'm almost wondering if we should just make a replacement for std::thread that allows to set the stack size in the constructor. The approach here isn't wrong per se, but I think if we could just say llvm::thread(Func, StackSize); it would be useful in more places.

lib/Support/Threading.cpp

112

Shouldn't the last parameter be set to true here?

[Dmitry.Kozhevnikov]

lib/Support/Threading.cpp

112

Oops, sorry, last-minute typo.

[Dmitry.Kozhevnikov]

Added a smoke test for blocking llvm_execute_on_thread as well

[jfb]

include/llvm/Support/Threading.h

98

Seems like you want an optional<> for stack size. Can you also change the name so it contains the unit? i.e. Requested StackSizeInBytes.

lib/Support/Unix/Threading.inc

70

Please change ShouldJoin to an enum class instead of a bool. Ditto on the return value.

83

So any failure to init attr, set stack size, or create the thread just... silently fail? I know this is pre-existing, but that's terrible! Is there ever a situation when we want to execute something on a thread, or just not execute it at all? It seems like we should abort if any of these fail?

[Dmitry.Kozhevnikov]

Addressed review comments:

• Any failure when spawning a thread now causes a fatal error
• Replaced a bool flag with an enum
• Replaced unsigned RequestedStackSize with Optional<unsigned> StackSizeInBytes

[jfb]

include/llvm/Support/Threading.h

53

Can you remove the llvm_execute_on_thread while you're here?

lib/Support/Threading.cpp

119

llvm::make_unique instead of new.

122

I'd replace get above with release, and not have the release here, since you're kinda "moving" ownership into a void*.

lib/Support/Unix/Threading.inc

65

Here and below, pthread doesn't set errno: it returns the error directly. You should pass the result in and call strerror.

[Dmitry.Kozhevnikov]

include/llvm/Support/Threading.h

53

Should I? It's used twice: at CrashRecoveryContext::RunSafelyOnThread, and, which is probably worse, exposed via the libclang (tools/clang/include/clang-c/Index.h, clang_executeOnThread). My understanding was this API is meant to be stable? That was one of the reasons I haven't touched its interface at all.

Also, while looking for this, I've found the fallback implementation of llvm_execute_on_thread, in lib/Support/Threading.cpp, which just runs the callback synchronously. Sorry, I haven't updated its signature, will do. I've probably should also add the new function there, that would just raise a fatal error since it's meaningless to call it with multithreading disabled.

lib/Support/Threading.cpp

122

I think in the current state, I can just drop unique_ptr and just pass a raw pointer returned by new directly.

lib/Support/Unix/Threading.inc

65

Sorry, will do.

[jfb]

include/llvm/Support/Threading.h

53

I mean the comment start. Documentation like this is the old LLVM style, and as we update code we also update the docs:

http://www.llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

[Dmitry.Kozhevnikov]

include/llvm/Support/Threading.h

53

I see, thank you for the info, will do.

[Dmitry.Kozhevnikov]

Address review comments:

• use error code returned from pthread functions to report errors
• use naked new instead of unique_ptr (it's always released anyway)
• tune comments
• add matching function stubs when threading implementation is unavailable or unknown

[jfb]

Unix stuff looks good. I didn't look at Windows.

[Dmitry.Kozhevnikov]

In D51103#1214852, @jfb wrote:

Unix stuff looks good. I didn't look at Windows.

Thank you!

Should I invite someone else for the review?

[jfb]

In D51103#1214855, @Dmitry.Kozhevnikov wrote:

In D51103#1214852, @jfb wrote:

Unix stuff looks good. I didn't look at Windows.

Thank you!

Should I invite someone else for the review?

Ain't no party like a Windows review party!

I mean, yes :)

[ilya-biryukov]

IIUC, someone needs to review the Windows bits here.
I volunteer to do this tomorrow unless someone else want to beat me to this :-)

[jfb]

In D51103#1496948, @ilya-biryukov wrote:

IIUC, someone needs to review the Windows bits here.
I volunteer to do this tomorrow unless someone else want to beat me to this :-)

Maybe @rnk can help.

[rnk]

Windows parts look good! Sorry for the delay.

[ilya-biryukov]

@Dmitry.Kozhevnikov, is this good to go or do you plan more changes?

[Dmitry.Kozhevnikov]

• moved to monorepo, rebased
• I've added an std::thread-based implementation of llvm_execute_on_thread_async when threading is on, but it's non-win32 non-pthread implementation (otherwise we should expose this detail for clients so they could decide if they could use it).

[Dmitry.Kozhevnikov]

In D51103#1504940, @ilya-biryukov wrote:

@Dmitry.Kozhevnikov, is this good to go or do you plan more changes?

I've added the last changes I wanted to make today.

[sammccall]

We realized today this never got landed, and I tried to do so. Unfortunately the callers weren't updated for the unsigned -> optional change.
Stack sizes seem to get passed around through lots of functions. I can update the immediate callers only and reland.

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
 
+#include "llvm/ADT/FunctionExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Config/llvm-config.h" // for LLVM_ON_UNIX
 #include "llvm/Support/Compiler.h"
 #include <ciso646> // So we can check the C++ standard lib macros.
 #include <functional>
 
 #if defined(_MSC_VER)
 // MSVC's call_once implementation worked since VS 2015, which is the minimum

 
 namespace llvm {
 class Twine;
 
 /// 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 and waits for thread
 /// completion.
 ///
 /// 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);
+/// \param StackSizeInBytes - A requested size (in bytes) for the thread stack
+/// (or None for default)
+void llvm_execute_on_thread(
+    void (*UserFn)(void *), void *UserData,
+    llvm::Optional<unsigned> StackSizeInBytes = llvm::None);
+
+/// Schedule the given \p Func for execution on a separate thread, then return
+/// to the caller immediately. Roughly equivalent to
+/// `std::thread(Func).detach()`, except it allows requesting a specific stack
+/// size.
+///
+/// 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 Func - The callback to execute.
+/// \param StackSizeInBytes - A requested size (in bytes) for the thread stack
+/// (or None for default)
+void llvm_execute_on_thread_async(
+    llvm::unique_function<void()> Func,
+    llvm::Optional<unsigned> StackSizeInBytes = llvm::None);
 
 #if LLVM_THREADING_USE_STD_CALL_ONCE
 
   typedef std::once_flag once_flag;
 
 #else
 
   enum InitStatus { Uninitialized = 0, Wait = 1, Done = 2 };
 
   /// The llvm::once_flag structure
   ///
   /// This type is modeled after std::once_flag to use with llvm::call_once.
   /// This structure must be used as an opaque object. It is a struct to force
   /// autoinitialization and behave like std::once_flag.
   struct once_flag {
     volatile sys::cas_flag status = Uninitialized;
   };
 
 #endif
 
   /// Execute the function specified as a parameter once.
   ///
   /// Typical usage:

lib/Support/Threading.cpp

 //===-- llvm/Support/Threading.cpp- Control multithreading mode --*- C++ -*-==//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file defines helper functions for running LLVM in a multi-threaded
 // environment.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/Threading.h"
+#include "llvm/ADT/Optional.h"
 #include "llvm/Config/config.h"
 #include "llvm/Support/Host.h"
 
 #include <cassert>
 #include <errno.h>
 #include <stdlib.h>
 #include <string.h>
 
if (sched_getaffinity(0, sizeof(Set), &Set))
     return CPU_COUNT(&Set);
 #endif
   // Guard against std::thread::hardware_concurrency() returning 0.
   if (unsigned Val = std::thread::hardware_concurrency())
     return Val;
   return 1;
 }
 
+namespace {
+struct SyncThreadInfo {
+  void (*UserFn)(void *);
+  void *UserData;
+};
+
+using AsyncThreadInfo = llvm::unique_function<void()>;
+
+enum class JoiningPolicy { Join, Detach };
+} // namespace
+
 // Include the platform-specific parts of this class.
 #ifdef LLVM_ON_UNIX
 #include "Unix/Threading.inc"
 #endif
 #ifdef _WIN32
 #include "Windows/Threading.inc"
 #endif
 
+void llvm::llvm_execute_on_thread(void (*Fn)(void *), void *UserData,
+                                  llvm::Optional<unsigned> StackSizeInBytes) {
+
+  SyncThreadInfo Info = {Fn, UserData};
+  llvm_execute_on_thread_impl(threadFuncSync, &Info, StackSizeInBytes,
+                              JoiningPolicy::Join);
+}
+
+void llvm::llvm_execute_on_thread_async(
+    llvm::unique_function<void()> Func,
+    llvm::Optional<unsigned> StackSizeInBytes) {
+  std::unique_ptr<AsyncThreadInfo> Info(new AsyncThreadInfo(std::move(Func)));
+  llvm_execute_on_thread_impl(&threadFuncAsync, Info.get(), StackSizeInBytes,
+                              JoiningPolicy::Detach);
+  Info.release();
+}
+
 #endif

lib/Support/Unix/Threading.inc

 //===- Unix/Threading.inc - Unix Threading Implementation ----- -*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file provides the Unix specific implementation of Threading functions.
 //
 //===----------------------------------------------------------------------===//
 
+#include "Unix.h"
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/Twine.h"
 
 #if defined(__APPLE__)
 #include <mach/mach_init.h>
 #include <mach/mach_port.h>
 #endif
 

 #include <lwp.h> // For _lwp_self()
 #endif
 
 #if defined(__linux__)
 #include <sys/syscall.h> // For syscall codes
 #include <unistd.h>      // For syscall()
 #endif
 
-namespace {
-  struct ThreadInfo {
-    void(*UserFn)(void *);
-    void *UserData;
-  };
+static void *threadFuncSync(void *Arg) {
+  SyncThreadInfo *TI = static_cast<SyncThreadInfo *>(Arg);
+  TI->UserFn(TI->UserData);
+  return nullptr;
 }
 
-static void *ExecuteOnThread_Dispatch(void *Arg) {
-  ThreadInfo *TI = reinterpret_cast<ThreadInfo*>(Arg);
-  TI->UserFn(TI->UserData);
+static void *threadFuncAsync(void *Arg) {
+  std::unique_ptr<AsyncThreadInfo> Info(static_cast<AsyncThreadInfo *>(Arg));
+  (*Info)();
   return nullptr;
 }
 
-void llvm::llvm_execute_on_thread(void(*Fn)(void*), void *UserData,
-  unsigned RequestedStackSize) {
-  ThreadInfo Info = { Fn, UserData };
+static void
+llvm_execute_on_thread_impl(void *(*ThreadFunc)(void *), void *Arg,
+                            llvm::Optional<unsigned> StackSizeInBytes,
+                            JoiningPolicy JP) {
+  // Construct the attributes object.
   pthread_attr_t Attr;
-  pthread_t Thread;
+  if (::pthread_attr_init(&Attr) != 0) {
+    ReportErrnoFatal("pthread_attr_init failed");
+  }
 
-  // Construct the attributes object.
-  if (::pthread_attr_init(&Attr) != 0)
-    return;
+  auto AttrGuard = llvm::make_scope_exit([&] {
+    if (::pthread_attr_destroy(&Attr) != 0) {
+      ReportErrnoFatal("pthread_attr_destroy failed");
+    }
+  });
 
   // Set the requested stack size, if given.
-  if (RequestedStackSize != 0) {
-    if (::pthread_attr_setstacksize(&Attr, RequestedStackSize) != 0)
-      goto error;
+  if (StackSizeInBytes) {
+    if (::pthread_attr_setstacksize(&Attr, *StackSizeInBytes) != 0) {
+      ReportErrnoFatal("pthread_attr_setstacksize failed");
+    }
   }
 
   // Construct and execute the thread.
-  if (::pthread_create(&Thread, &Attr, ExecuteOnThread_Dispatch, &Info) != 0)
-    goto error;
-
-  // Wait for the thread and clean up.
-  ::pthread_join(Thread, nullptr);
+  pthread_t Thread;
+  if (::pthread_create(&Thread, &Attr, ThreadFunc, Arg) != 0)
+    ReportErrnoFatal("pthread_create failed");
 
-error:
-  ::pthread_attr_destroy(&Attr);
+  if (JP == JoiningPolicy::Join) {
+    // Wait for the thread
+    if (::pthread_join(Thread, nullptr) != 0) {
+      ReportErrnoFatal("pthread_join failed");
+    }
+  }
 }
-
 
 uint64_t llvm::get_threadid() {
 #if defined(__APPLE__)
   // Calling "mach_thread_self()" bumps the reference count on the thread
   // port, so we need to deallocate it. mach_task_self() doesn't bump the ref
   // count.
   thread_port_t Self = mach_thread_self();
   mach_port_deallocate(mach_task_self(), Self);

lib/Support/Unix/Unix.h

 //===----------------------------------------------------------------------===//
 //=== WARNING: Implementation here must contain only generic UNIX code that
 //===          is guaranteed to work on all UNIX variants.
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Config/config.h" // Get autoconf configuration settings
 #include "llvm/Support/Chrono.h"
 #include "llvm/Support/Errno.h"
+#include "llvm/Support/ErrorHandling.h"
 #include <algorithm>
 #include <assert.h>
 #include <cerrno>
 #include <cstdio>
 #include <cstdlib>
 #include <cstring>
 #include <string>
 #include <sys/types.h>
static inline bool MakeErrMsg(
   if (!ErrMsg)
     return true;
   if (errnum == -1)
     errnum = errno;
   *ErrMsg = prefix + ": " + llvm::sys::StrError(errnum);
   return true;
 }
 
+// Include StrError() in a fatal error message.
+LLVM_ATTRIBUTE_NORETURN static inline void ReportErrnoFatal(const char *Msg) {
+  std::string ErrMsg;
+  MakeErrMsg(&ErrMsg, Msg);
+  llvm::report_fatal_error(ErrMsg);
+}
+
 namespace llvm {
 namespace sys {
 
 /// Convert a struct timeval to a duration. Note that timeval can be used both
 /// as a time point and a duration. Be sure to check what the input represents.
 inline std::chrono::microseconds toDuration(const struct timeval &TV) {
   return std::chrono::seconds(TV.tv_sec) +
          std::chrono::microseconds(TV.tv_usec);

lib/Support/Windows/Process.inc

 }
 
 const char *Process::ResetColor() {
   if (UseANSI) return "\033[0m";
   SetConsoleTextAttribute(GetStdHandle(STD_OUTPUT_HANDLE), defaultColors());
   return 0;
 }
 
-// Include GetLastError() in a fatal error message.
-static void ReportLastErrorFatal(const char *Msg) {
-  std::string ErrMsg;
-  MakeErrMsg(&ErrMsg, Msg);
-  report_fatal_error(ErrMsg);
-}
-
 unsigned Process::GetRandomNumber() {
   HCRYPTPROV HCPC;
   if (!::CryptAcquireContextW(&HCPC, NULL, NULL, PROV_RSA_FULL,
                               CRYPT_VERIFYCONTEXT))
     ReportLastErrorFatal("Could not acquire a cryptographic context");
 
   ScopedCryptContext CryptoProvider(HCPC);
   unsigned Ret;
   if (!::CryptGenRandom(CryptoProvider, sizeof(Ret),
                         reinterpret_cast<BYTE *>(&Ret)))
     ReportLastErrorFatal("Could not generate a random number");
   return Ret;
 }

lib/Support/Windows/Threading.inc

 #include "Windows/WindowsSupport.h"
 #include <process.h>
 
 // Windows will at times define MemoryFence.
 #ifdef MemoryFence
 #undef MemoryFence
 #endif
 
-namespace {
-  struct ThreadInfo {
-    void(*func)(void*);
-    void *param;
-  };
+static unsigned __stdcall threadFuncSync(void *Arg) {
+  SyncThreadInfo *TI = static_cast<SyncThreadInfo *>(Arg);
+  TI->UserFn(TI->UserData);
+  return 0;
 }
 
-static unsigned __stdcall ThreadCallback(void *param) {
-  struct ThreadInfo *info = reinterpret_cast<struct ThreadInfo *>(param);
-  info->func(info->param);
-
+static unsigned __stdcall threadFuncAsync(void *Arg) {
+  std::unique_ptr<AsyncThreadInfo> Info(static_cast<AsyncThreadInfo *>(Arg));
+  (*Info)();
   return 0;
 }
 
-void llvm::llvm_execute_on_thread(void(*Fn)(void*), void *UserData,
-  unsigned RequestedStackSize) {
-  struct ThreadInfo param = { Fn, UserData };
-
-  HANDLE hThread = (HANDLE)::_beginthreadex(NULL,
-    RequestedStackSize, ThreadCallback,
-    &param, 0, NULL);
-
-  if (hThread) {
-    // We actually don't care whether the wait succeeds or fails, in
-    // the same way we don't care whether the pthread_join call succeeds
-    // or fails.  There's not much we could do if this were to fail. But
-    // on success, this call will wait until the thread finishes executing
-    // before returning.
-    (void)::WaitForSingleObject(hThread, INFINITE);
-    ::CloseHandle(hThread);
+static void
+llvm_execute_on_thread_impl(_beginthreadex_proc_type ThreadFunc, void *Arg,
+                            llvm::Optional<unsigned> StackSizeInBytes,
+                            JoiningPolicy JP) {
+  HANDLE hThread = (HANDLE)::_beginthreadex(
+      NULL, StackSizeInBytes.getValueOr(0), ThreadFunc, Arg, 0, NULL);
+
+  if (!hThread) {
+    ReportLastErrorFatal("_beginthreadex failed");
+  }
+
+  if (JP == JoiningPolicy::Join) {
+    if (::WaitForSingleObject(hThread, INFINITE) == WAIT_FAILED) {
+      ReportLastErrorFatal("WaitForSingleObject failed");
+    }
+  }
+  if (::CloseHandle(hThread) == FALSE) {
+    ReportLastErrorFatal("CloseHandle failed");
   }
 }
 
 uint64_t llvm::get_threadid() {
   return uint64_t(::GetCurrentThreadId());
 }
 
 uint32_t llvm::get_max_thread_name_length() { return 0; }

lib/Support/Windows/WindowsSupport.h

 
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Config/config.h" // Get build system configuration settings
 #include "llvm/Support/Chrono.h"
 #include "llvm/Support/Compiler.h"
+#include "llvm/Support/ErrorHandling.h"
 #include <cassert>
 #include <string>
 #include <system_error>
 #include <windows.h>
 
 // Must be included after windows.h
 #include <wincrypt.h>
 
inline bool MakeErrMsg(std::string *ErrMsg, const std::string &prefix) {
   else
     *ErrMsg = prefix + ": Unknown error";
   *ErrMsg += " (0x" + llvm::utohexstr(LastError) + ")";
 
   LocalFree(buffer);
   return R != 0;
 }
 
+// Include GetLastError() in a fatal error message.
+LLVM_ATTRIBUTE_NORETURN inline void ReportLastErrorFatal(const char *Msg) {
+  std::string ErrMsg;
+  MakeErrMsg(&ErrMsg, Msg);
+  llvm::report_fatal_error(ErrMsg);
+}
+
 template <typename HandleTraits>
 class ScopedHandle {
   typedef typename HandleTraits::handle_type handle_type;
   handle_type Handle;
 
   ScopedHandle(const ScopedHandle &other); // = delete;
   void operator=(const ScopedHandle &other); // = delete;
 public:

unittests/Support/Threading.cpp

 //===- unittests/Threading.cpp - Thread tests -----------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/thread.h"
 #include "gtest/gtest.h"
 
+#include <atomic>
+
 using namespace llvm;
 
 namespace {
 
+class Notification {
+public:
+  void notify() {
+    {
+      std::lock_guard<std::mutex> Lock(M);
+      Notified = true;
+    }
+    CV.notify_all();
+  }
+
+  bool wait() {
+    std::unique_lock<std::mutex> Lock(M);
+    using steady_clock = std::chrono::steady_clock;
+    auto Deadline = steady_clock::now() +
+                    std::chrono::duration_cast<steady_clock::duration>(
+                        std::chrono::duration<double>(5));
+    return CV.wait_until(Lock, Deadline, [this] { return Notified; });
+  }
+
+private:
+  bool Notified = false;
+  mutable std::condition_variable CV;
+  mutable std::mutex M;
+};
+
 TEST(Threading, PhysicalConcurrency) {
   auto Num = heavyweight_hardware_concurrency();
   // Since Num is unsigned this will also catch us trying to
   // return -1.
   ASSERT_LE(Num, thread::hardware_concurrency());
 }
 
+TEST(Threading, RunOnThreadSyncAsync) {
+  Notification ThreadStarted, ThreadAdvanced, ThreadFinished;
+
+  auto ThreadFunc = [&] {
+    ThreadStarted.notify();
+    ASSERT_TRUE(ThreadAdvanced.wait());
+    ThreadFinished.notify();
+  };
+
+  llvm::llvm_execute_on_thread_async(ThreadFunc);
+  ASSERT_TRUE(ThreadStarted.wait());
+  ThreadAdvanced.notify();
+  ASSERT_TRUE(ThreadFinished.wait());
+}
+
+TEST(Threading, RunOnThreadSync) {
+  std::atomic_bool Executed(false);
+  llvm::llvm_execute_on_thread(
+      [](void *Arg) { *static_cast<std::atomic_bool *>(Arg) = true; },
+      &Executed);
+  ASSERT_EQ(Executed, true);
+}
+
 } // end anon namespace