Simplify thread clean / exit / terminate logic. NFC

Mostly inspired by musl.

Author: kleisauke

system/lib/libc/musl/src/thread/pthread_detach.c

@@ -4,30 +4,37 @@
 static int __pthread_detach(pthread_t t)
 {
 #ifdef __EMSCRIPTEN__
-	// XXX EMSCRIPTEN: Add check for invalid (already joined) thread.  Again
+	// XXX Emscripten: Add check for invalid (already joined) thread.  Again
 	// for the benefit of the conformance tests.
-	if (!_emscripten_thread_is_valid(t))
-		return ESRCH;
-#endif
-	/* If the cas fails, detach state is either already-detached
-	 * or exiting/exited, and pthread_join will trap or cleanup. */
-#ifdef __EMSCRIPTEN__
-	int old_state = a_cas(&t->detach_state, DT_JOINABLE, DT_DETACHED);
-	if (old_state != DT_JOINABLE) {
+	if (!_emscripten_thread_is_valid(t)) return ESRCH;
+
+	// Note that we don't use pthread_join here, to avoid
+	// returning EDEADLK when attempting to detach itself.
+	switch (a_cas(&t->detach_state, DT_JOINABLE, DT_DETACHED)) {
+	case DT_EXITED:
+	case DT_EXITING:
+		_emscripten_thread_cleanup(t);
+		return 0;
+	case DT_JOINABLE:
+		return 0;
+	case DT_DETACHED: // already-detached
+	default: // >= DT_DETACHED
 		// Even though the man page says this is undefined behaviour to attempt to
 		// detach an already-detached thread we have several tests in the posixtest
 		// suite that depend on this (pthread_join.c)
-		if (old_state == DT_DETACHED)
-			return EINVAL;
+		return EINVAL;
+	}
 #else
+	/* If the cas fails, detach state is either already-detached
+	 * or exiting/exited, and pthread_join will trap or cleanup. */
 	if (a_cas(&t->detach_state, DT_JOINABLE, DT_DETACHED) != DT_JOINABLE) {
-#endif
 		int cs;
 		__pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
 		__pthread_join(t, 0);
 		__pthread_setcancelstate(cs, 0);
 	}
 	return 0;
+#endif
 }
 
 weak_alias(__pthread_detach, pthread_detach);

system/lib/libc/musl/src/thread/pthread_join.c

@@ -13,11 +13,8 @@ static int __pthread_timedjoin_np(pthread_t t, void **res, const struct timespec
 	// Attempt to join a thread which does not point to a valid thread, or
 	// does not exist anymore.
 	if (!_emscripten_thread_is_valid(t)) return ESRCH;
-	// Thread is attempting to join to itself.  Already detached threads are
-	// handled below by returning EINVAL instead.
-	// TODO: The detached check here is just to satisfy the
-	// `other.test_{proxy,main}_pthread_join_detach` tests.
-	if (t->detach_state != DT_DETACHED && __pthread_self() == t) return EDEADLK;
+	// Thread is attempting to join to itself.
+	if (__pthread_self() == t) return EDEADLK;
 #endif
 	int state, cs, r = 0;
 	__pthread_testcancel();

system/lib/pthread/library_pthread.c

@@ -117,7 +117,7 @@ void emscripten_main_thread_process_queued_calls() {
 }
 
 int _emscripten_thread_is_valid(pthread_t thread) {
-  return thread->self == thread;
+  return thread->tid;
 }
 
 static void *dummy_tsd[1] = { 0 };
@@ -126,8 +126,7 @@ weak_alias(dummy_tsd, __pthread_tsd_main);
 // See system/lib/README.md for static constructor ordering.
 __attribute__((constructor(48)))
 void _emscripten_init_main_thread(void) {
-  // The pthread struct has a field that points to itself - this is used as
-  // a magic ID to detect whether the pthread_t structure is 'alive'.
+  // The pthread struct has a field that points to itself.
   __main_pthread.self = &__main_pthread;
   __main_pthread.detach_state = DT_JOINABLE;
   // Main thread ID is always 1.  It can't be 0 because musl assumes

system/lib/pthread/pthread_create.c

@@ -173,9 +173,10 @@ int __pthread_create(pthread_t* restrict res,
   new->map_base = block;
   new->map_size = size;
 
-  // The pthread struct has a field that points to itself - this is used as a
-  // magic ID to detect whether the pthread_t structure is 'alive'.
+  // The pthread struct has a field that points to itself.
   new->self = new;
+
+  // Thread ID, this becomes zero when the thread is no longer available.
   new->tid = next_tid++;
 
   // pthread struct robust_list head should point to itself.
@@ -280,18 +281,21 @@ void _emscripten_thread_free_data(pthread_t t) {
   // A thread can never free its own thread data.
   assert(t != pthread_self());
 #ifndef NDEBUG
-  if (t->profilerBlock) {
-    emscripten_builtin_free(t->profilerBlock);
+  intptr_t old_block = __c11_atomic_exchange((_Atomic intptr_t*)&t->profilerBlock, 0, __ATOMIC_SEQ_CST);
+  if (old_block) {
+    emscripten_builtin_free((void*)old_block);
   }
 #endif
+  // The tid may be reused.  Clear it to prevent inadvertent use
+  // and inform functions that would use it that it's no longer
+  // available.
+  t->tid = 0;
 
-  // Free all the enture thread block (called map_base because
+  // Free the entire thread block (called map_base because
   // musl normally allocates this using mmap).  This region
   // includes the pthread structure itself.
   unsigned char* block = t->map_base;
   dbg("_emscripten_thread_free_data thread=%p map_base=%p", t, block);
-  // To aid in debugging, set the entire region to zero.
-  memset(block, 0, sizeof(struct pthread));
   emscripten_builtin_free(block);
 }
 

test/other/test_pthread_self_join_detach.c

@@ -21,17 +21,11 @@ int main() {
   pthread_t self = pthread_self();
 
   /*
-   * Attempts to join the current thread will either generate
-   * EDEADLK or EINVAL depending on whether has already been
-   * detached
+   * Attempts to join the current thread will generate EDEADLK.
    */
   int ret = pthread_join(self, NULL);
   printf("pthread_join -> %s\n", strerror(ret));
-  if (is_detached) {
-    assert(ret == EINVAL);
-  } else {
-    assert(ret == EDEADLK);
-  }
+  assert(ret == EDEADLK);
 
   /*
    * Attempts to detach the main thread will either succeed
@@ -45,10 +39,6 @@ int main() {
     assert(ret == 0);
   }
 
-  ret = pthread_join(self, NULL);
-  printf("pthread_join -> %s\n", strerror(ret));
-  assert(ret == EINVAL);
-
   puts("passed");
 
   return 0;