LibThreading: Overhaul thread behavior with ThreadState

This replaces all state-related variables with a single ThreadState.
These are simplified over what the Kernel has, but capture all
userspace-available thread state.

Locking the state behind an atomic and using proper atomic operations
also gets rid of quite some deadlocks and race conditions that have
existed around m_tid and others beforehand.

In terms of behavior, this introduces the following changes:
- All thread state mishandling (e.g. joining a detached thread) crashes
  the program. Mishandling thread state is a severe kind of concurrency
  bug that might also be indeterministic, so letting it silently
  disappear with the return value of pthread_ APIs is a bad idea. The
  thread state can always be checked beforehand to ensure that no crash
  happens.
- Destructing a still-running thread will crash in AK/Function, so the
  Thread destructor issues its own warning for debugging purposes.
- Thread issues warnings before crashes in many places to aid
  concurrency debugging (the most difficult kind of debugging).
- Joining dead but not detached threads is legal, as per POSIX APIs.
- The thread ID is never reset to 0 after the thread has been started
  and subsequently been assigned a valid thread ID. The thread's exit
  state is still obtainable.
- Detaching threads that are about to exit is considered a programming
  bug and will often (not always, as we can't catch all execution
  sequences involved in such a situation) crash the program on purpose.
  If you want to detach a thread that will definitely exit on its own,
  you have to prevent it from exiting before detach() was called (e.g.
  with an "exit requested" flag).

Userland/Libraries/LibThreading/Thread.cpp

@@ -25,10 +25,12 @@ Thread::Thread(Function<intptr_t()> action, StringView thread_name)
 
 Thread::~Thread()
 {
-    if (m_tid && !m_detached) {
-        dbgln("Destroying thread \"{}\"({}) while it is still running!", m_thread_name, m_tid);
+    if (needs_to_be_joined()) {
+        dbgln("Destroying {} while it is still running undetached!", *this);
         [[maybe_unused]] auto res = join();
     }
+    if (m_state == ThreadState::Detached)
+        dbgln("Bug! {} in state {} is being destroyed; AK/Function will crash shortly!", *this, m_state.load());
 }
 
 ErrorOr<void> Thread::set_priority(int priority)
@@ -56,17 +58,52 @@ DeprecatedString Thread::thread_name() const { return m_thread_name; }
 
 pthread_t Thread::tid() const { return m_tid; }
 
-bool Thread::is_started() const { return m_started; }
+ThreadState Thread::state() const { return m_state; }
+
+bool Thread::is_started() const { return m_state != ThreadState::Startable; }
+
+bool Threading::Thread::needs_to_be_joined() const
+{
+    auto state = m_state.load();
+    return state == ThreadState::Running || state == ThreadState::Exited;
+}
+
+bool Threading::Thread::has_exited() const
+{
+    auto state = m_state.load();
+    return state == ThreadState::Joined || state == ThreadState::Exited || state == ThreadState::DetachedExited;
+}
 
 void Thread::start()
 {
+    VERIFY(!is_started());
+
+    // Set this first so that the other thread starts out seeing m_state == Running.
+    m_state = Threading::ThreadState::Running;
+
     int rc = pthread_create(
         &m_tid,
+        // FIXME: Use pthread_attr_t to start a thread detached if that was requested by the user before the call to start().
         nullptr,
         [](void* arg) -> void* {
             Thread* self = static_cast<Thread*>(arg);
             auto exit_code = self->m_action();
-            self->m_tid = 0;
+
+            auto expected = Threading::ThreadState::Running;
+            // This code might race with a call to detach().
+            if (!self->m_state.compare_exchange_strong(expected, Threading::ThreadState::Exited)) {
+                // If the original state was Detached, we need to set to DetachedExited instead.
+                if (expected == Threading::ThreadState::Detached) {
+                    if (!self->m_state.compare_exchange_strong(expected, Threading::ThreadState::DetachedExited)) {
+                        dbgln("Thread logic bug: Found thread state {} while trying to set ExitedDetached state!", expected);
+                        VERIFY_NOT_REACHED();
+                    }
+                } else {
+                    dbgln("Thread logic bug: Found thread state {} while trying to set Exited state!", expected);
+                    VERIFY_NOT_REACHED();
+                }
+            }
+
             return reinterpret_cast<void*>(exit_code);
         },
         static_cast<void*>(this));
@@ -78,18 +115,24 @@ void Thread::start()
         VERIFY(rc == 0);
     }
 #endif
-    dbgln("Started thread \"{}\", tid = {}", m_thread_name, m_tid);
-    m_started = true;
+    dbgln("Started {}", *this);
 }
 
 void Thread::detach()
 {
-    VERIFY(!m_detached);
+    auto expected = Threading::ThreadState::Running;
+    // This code might race with the other thread exiting.
+    if (!m_state.compare_exchange_strong(expected, Threading::ThreadState::Detached)) {
+        // Always report a precise error before crashing. These kinds of bugs are hard to reproduce.
+        if (expected == Threading::ThreadState::Exited)
+            dbgln("Thread logic bug: {} is being detached after having exited!", this);
+        else
+            dbgln("Thread logic bug: trying to detach {} which is not in the Started state, but state {}!", this, expected);
+        VERIFY_NOT_REACHED();
+    }
 
     int rc = pthread_detach(m_tid);
     VERIFY(rc == 0);
-
-    m_detached = true;
 }
 
 }

Userland/Libraries/LibThreading/Thread.h

@@ -7,6 +7,7 @@
 
 #pragma once
 
+#include <AK/Assertions.h>
 #include <AK/DeprecatedString.h>
 #include <AK/DistinctNumeric.h>
 #include <AK/Function.h>
@@ -49,35 +50,46 @@ public:
     ErrorOr<void> set_priority(int priority);
     ErrorOr<int> get_priority() const;
 
+    // Only callable in the Startable state.
     void start();
+    // Only callable in the Running state.
     void detach();
 
+    // Only callable in the Running or Exited states.
     template<typename T = void>
     Result<T, ThreadError> join();
 
     DeprecatedString thread_name() const;
     pthread_t tid() const;
+    ThreadState state() const;
     bool is_started() const;
+    bool needs_to_be_joined() const;
+    bool has_exited() const;
 
 private:
     explicit Thread(Function<intptr_t()> action, StringView thread_name = {});
     Function<intptr_t()> m_action;
     pthread_t m_tid { 0 };
     DeprecatedString m_thread_name;
-    bool m_detached { false };
-    bool m_started { false };
+    Atomic<ThreadState> m_state { ThreadState::Startable };
 };
 
 template<typename T>
 Result<T, ThreadError> Thread::join()
 {
+    VERIFY(needs_to_be_joined());
+
     void* thread_return = nullptr;
     int rc = pthread_join(m_tid, &thread_return);
     if (rc != 0) {
         return ThreadError { rc };
     }
 
-    m_tid = 0;
+    // The other thread has now stopped running, so a TOCTOU bug is not possible.
+    // (If you call join from two different threads, you're doing something *very* wrong anyways.)
+    VERIFY(m_state == ThreadState::Exited);
+    m_state = ThreadState::Joined;
+
     if constexpr (IsVoid<T>)
         return {};
     else
@@ -98,7 +110,7 @@ template<>
 struct AK::Formatter<Threading::ThreadState> : AK::Formatter<FormatString> {
     ErrorOr<void> format(FormatBuilder& builder, Threading::ThreadState state)
     {
-        String name = "";
+        DeprecatedString name = "";
         switch (state) {
         case Threading::ThreadState::Detached:
             name = "Detached";