pthread: 'pthread_join()' improvements

Make pthread_join() work in more situations and support passing the thread
return value.

Fixes #2892

repos/libports/src/lib/pthread/thread.cc

@@ -13,6 +13,7 @@
  */
 
 #include <base/log.h>
+#include <base/sleep.h>
 #include <base/thread.h>
 #include <os/timed_semaphore.h>
 #include <util/list.h>
@@ -26,24 +27,6 @@
 
 using namespace Genode;
 
-/*
- * Structure to handle self-destructing pthreads.
- */
-struct thread_cleanup : List<thread_cleanup>::Element
-{
-	pthread_t thread;
-
-	thread_cleanup(pthread_t t) : thread(t) { }
-
-	~thread_cleanup() {
-		if (thread)
-			delete thread;
-	}
-};
-
-static Lock pthread_cleanup_list_lock;
-static List<thread_cleanup> pthread_cleanup_list;
-
 
 void * operator new(__SIZE_TYPE__ size) { return malloc(size); }
 void operator delete (void * p) { return free(p); }
@@ -72,10 +55,43 @@ void pthread::Thread_object::entry()
 	_stack_addr = (void *)info.base;
 	_stack_size = info.top - info.base;
 
-	void *exit_status = _start_routine(_arg);
+	pthread_exit(_start_routine(_arg));
+}
+
+
+void pthread::join(void **retval)
+{
+	struct Check : Libc::Suspend_functor
+	{
+		bool retry { false };
+
+		pthread &_thread;
+
+		Check(pthread &thread) : _thread(thread) { }
+		
+		bool suspend() override
+		{
+			retry = !_thread._exiting;
+			return retry;
+ 		}
+	} check(*this);
+
+	do {
+		Libc::suspend(check);
+	} while (check.retry);
+
+	_join_lock.lock();
+
+	if (retval)
+		*retval = _retval;
+}
+
+
+void pthread::cancel()
+{
 	_exiting = true;
 	Libc::resume_all();
-	pthread_exit(exit_status);
+	_join_lock.unlock();
 }
 
 
@@ -136,20 +152,9 @@ extern "C" {
 
 	int pthread_join(pthread_t thread, void **retval)
 	{
-		struct Check : Libc::Suspend_functor
-		{
-			bool suspend() override {
-				return true;
-			}
-		} check;
+		thread->join(retval);
 
-		while (!thread->exiting()) {
-			Libc::suspend(check);
-		}
-
-
-		thread->join();
-		*((int **)retval) = 0;
+		delete thread;
 
 		return 0;
 	}
@@ -178,38 +183,17 @@ extern "C" {
 	}
 
 
-	void pthread_cleanup()
-	{
-		{
-			Lock_guard<Lock> lock_guard(pthread_cleanup_list_lock);
-
-			while (thread_cleanup * t = pthread_cleanup_list.first()) {
-				pthread_cleanup_list.remove(t);
-				delete t;
-			}
-		}
-	}
-
 	int pthread_cancel(pthread_t thread)
 	{
-		/* cleanup threads which tried to self-destruct */
-		pthread_cleanup();
-
-		if (pthread_equal(pthread_self(), thread)) {
-			Lock_guard<Lock> lock_guard(pthread_cleanup_list_lock);
-			pthread_cleanup_list.insert(new thread_cleanup(thread));
-		} else
-			delete thread;
-
+		thread->cancel();
 		return 0;
 	}
 
+
 	void pthread_exit(void *value_ptr)
 	{
-		pthread_cancel(pthread_self());
-
-		Lock lock;
-		while (true) lock.lock();
+		pthread_self()->exit(value_ptr);
+		Genode::sleep_forever();
 	}
 
 

repos/libports/src/lib/pthread/thread.h

@@ -60,8 +60,6 @@ extern "C" {
 	 * defined as 'struct pthread*' in '_pthreadtypes.h'
 	 */
 	struct pthread;
-
-	void pthread_cleanup();
 }
 
 
@@ -104,7 +102,6 @@ struct pthread : Genode::Noncopyable, Genode::Thread::Tls::Base
 		{
 			start_routine_t _start_routine;
 			void           *_arg;
-			bool            _exiting = false;
 
 			void          *&_stack_addr;
 			size_t         &_stack_size;
@@ -149,6 +146,23 @@ struct pthread : Genode::Noncopyable, Genode::Thread::Tls::Base
 			pthread_registry().insert(this);
 		}
 
+		bool _exiting = false;
+
+		/*
+		 * The join lock is needed because 'Libc::resume_all()' uses a
+		 * 'Signal_transmitter' which holds a reference to a signal context
+		 * capability, which needs to be released before the thread can be
+		 * destroyed.
+		 *
+		 * Also, we cannot use 'Genode::Thread::join()', because it only
+		 * returns when the thread entry function returns, which does not
+		 * happen with 'pthread_cancel()'.
+		 */
+		Genode::Lock _join_lock { Genode::Lock::LOCKED };
+
+		/* return value for 'pthread_join()' */
+		void *_retval = PTHREAD_CANCELED;
+
 		/* attributes for 'pthread_attr_get_np()' */
 		void   *_stack_addr = nullptr;
 		size_t  _stack_size = 0;
@@ -191,12 +205,20 @@ struct pthread : Genode::Noncopyable, Genode::Thread::Tls::Base
 		}
 
 		void start() { _thread.start(); }
-		bool exiting() const
-		{
-			return _thread_object->_exiting;
-		}
 
-		void join() { _thread.join(); }
+		void join(void **retval);
+
+		/*
+		 * Inform the thread calling 'pthread_join()' that this thread can be
+		 * destroyed.
+		 */
+		void cancel();
+
+		void exit(void *retval)
+		{
+			_retval = retval;
+			cancel();
+		}
 
 		void   *stack_addr() const { return _stack_addr; }
 		size_t  stack_size() const { return _stack_size; }

repos/libports/src/lib/pthread/thread_create.cc

@@ -25,9 +25,6 @@ extern "C"
 	int pthread_create(pthread_t *thread, const pthread_attr_t *attr,
 	                   void *(*start_routine) (void *), void *arg)
 	{
-		/* cleanup threads which tried to self-destruct */
-		pthread_cleanup();
-
 		size_t const stack_size = (attr && *attr && (*attr)->stack_size)
 		                        ? (*attr)->stack_size
 		                        : Libc::Component::stack_size();

repos/libports/src/test/pthread/main.cc

@@ -13,6 +13,7 @@
 
 #include <pthread.h>
 #include <semaphore.h>
+#include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
 
@@ -52,7 +53,44 @@ void *thread_func(void *arg)
 	return 0;
 }
 
-void *thread_func_self_destruct(void *arg) { return 0; }
+/*
+ * Test self-destructing threads with 'pthread_join()', both when created and
+ * joined by the main thread and when created and joined by a pthread.
+ */
+
+void test_self_destruct(void *(*start_routine)(void*), uintptr_t num_iterations)
+{
+	for (uintptr_t i = 0; i < num_iterations; i++) {
+
+		pthread_t  t;
+		void      *retval;
+
+		if (pthread_create(&t, 0, start_routine, (void*)i) != 0) {
+			printf("error: pthread_create() failed\n");
+			exit(-1);
+		}
+
+		pthread_join(t, &retval);
+		
+		if (retval != (void*)i) {
+			printf("error: return value does not match\n");
+			exit(-1);
+		}
+	}
+}
+
+void *thread_func_self_destruct2(void *arg)
+{
+	return arg;
+}
+
+void *thread_func_self_destruct(void *arg)
+{
+	test_self_destruct(thread_func_self_destruct2, 2);
+
+	return arg;
+}
+
 
 static inline void compare_semaphore_values(int reported_value, int expected_value)
 {
@@ -120,13 +158,25 @@ int main(int argc, char **argv)
 	for (int i = 0; i < NUM_THREADS; i++)
 		if (thread[i].thread_args.thread_id_self != thread[i].thread_id_create) {
 			printf("error: thread IDs don't match\n");
+			return -1;
 		}
 
 	printf("main thread: destroying the threads\n");
 
-	for (int i = 0; i < NUM_THREADS; i++)
+	for (int i = 0; i < NUM_THREADS; i++) {
+
+		void *retval;
+
 		pthread_cancel(thread[i].thread_id_create);
 
+		pthread_join(thread[i].thread_id_create, &retval);
+
+		if (retval != PTHREAD_CANCELED) {
+			printf("error: return value is not PTHREAD_CANCELED\n");
+			return -1;
+		}
+	}
+
 	printf("main thread: destroying the semaphores\n");
 
 	for (int i = 0; i < NUM_THREADS; i++)
@@ -134,13 +184,7 @@ int main(int argc, char **argv)
 
 	printf("main thread: create pthreads which self de-struct\n");
 
-	for (unsigned i = 0 ; i < 100; i++) {
-		pthread_t t;
-		if (pthread_create(&t, 0, thread_func_self_destruct, 0) != 0) {
-			printf("error: pthread_create() failed\n");
-			return -1;
-		}
-	}
+	test_self_destruct(thread_func_self_destruct, 100);
 
 	printf("--- returning from main ---\n");
 	return 0;

repos/ports/src/virtualbox/thread.cc

@@ -116,9 +116,6 @@ static int create_thread(pthread_t *thread, const pthread_attr_t *attr,
 extern "C" int pthread_create(pthread_t *thread, const pthread_attr_t *attr,
                               void *(*start_routine) (void *), void *arg)
 {
-	/* cleanup threads which tried to self-destruct */
-	pthread_cleanup();
-
 	PRTTHREADINT rtthread = reinterpret_cast<PRTTHREADINT>(arg);
 
 	/* retry thread creation once after CPU session upgrade */