From 42fb00c9e1ef5a43c47f46deb86b3404ce75df1c Mon Sep 17 00:00:00 2001 From: Sam Clegg Date: Wed, 18 Jun 2025 16:56:03 -0700 Subject: [PATCH 1/2] Add pthread mutex deadlock detection for debug builds This would really have helped in at least detecting the deadlock that was found in #24570 / #24565 --- .../libc/musl/src/thread/pthread_mutex_lock.c | 3 +++ .../musl/src/thread/pthread_mutex_timedlock.c | 12 +++++++++++ .../musl/src/thread/pthread_mutex_trylock.c | 9 +++++++++ system/lib/pthread/library_pthread.c | 9 +++++---- .../test_codesize_minimal_pthreads.size | 2 +- ...t_codesize_minimal_pthreads_memgrowth.size | 2 +- test/other/test_pthread_mutex_deadlock.c | 20 +++++++++++++++++++ test/test_other.py | 6 ++++++ 8 files changed, 57 insertions(+), 6 deletions(-) create mode 100644 test/other/test_pthread_mutex_deadlock.c diff --git a/system/lib/libc/musl/src/thread/pthread_mutex_lock.c b/system/lib/libc/musl/src/thread/pthread_mutex_lock.c index 638d4b8697d84..02269d366c858 100644 --- a/system/lib/libc/musl/src/thread/pthread_mutex_lock.c +++ b/system/lib/libc/musl/src/thread/pthread_mutex_lock.c @@ -2,9 +2,12 @@ int __pthread_mutex_lock(pthread_mutex_t *m) { +#if !defined(__EMSCRIPTEN__) || defined(NDEBUG) + /* XXX EMSCRIPTEN always take the slow path in debug builds so we can trap rather than deadlock */ if ((m->_m_type&15) == PTHREAD_MUTEX_NORMAL && !a_cas(&m->_m_lock, 0, EBUSY)) return 0; +#endif return __pthread_mutex_timedlock(m, 0); } diff --git a/system/lib/libc/musl/src/thread/pthread_mutex_timedlock.c b/system/lib/libc/musl/src/thread/pthread_mutex_timedlock.c index 8a8952b67e8ae..232c9b321a8e2 100644 --- a/system/lib/libc/musl/src/thread/pthread_mutex_timedlock.c +++ b/system/lib/libc/musl/src/thread/pthread_mutex_timedlock.c @@ -1,5 +1,9 @@ #include "pthread_impl.h" +#ifdef __EMSCRIPTEN__ +#include +#endif + #ifndef __EMSCRIPTEN__ #define IS32BIT(x) !((x)+0x80000000ULL>>32) #define CLAMP(x) (int)(IS32BIT(x) ? (x) : 0x7fffffffU+((0ULL+(x))>>63)) @@ -57,9 +61,12 @@ static int pthread_mutex_timedlock_pi(pthread_mutex_t *restrict m, const struct int __pthread_mutex_timedlock(pthread_mutex_t *restrict m, const struct timespec *restrict at) { +#if !defined(__EMSCRIPTEN__) || defined(NDEBUG) + /* XXX EMSCRIPTEN always take the slow path in debug builds so we can trap rather than deadlock */ if ((m->_m_type&15) == PTHREAD_MUTEX_NORMAL && !a_cas(&m->_m_lock, 0, EBUSY)) return 0; +#endif int type = m->_m_type; int r, t, priv = (type & 128) ^ 128; @@ -82,6 +89,11 @@ int __pthread_mutex_timedlock(pthread_mutex_t *restrict m, const struct timespec if ((type&3) == PTHREAD_MUTEX_ERRORCHECK && own == __pthread_self()->tid) return EDEADLK; +#if defined(__EMSCRIPTEN__) && !defined(NDEBUG) + // Extra check for deadlock in debug builds, but only if we would block + // forever (at == NULL). + assert(at || own != __pthread_self()->tid && "pthread mutex deadlock detected"); +#endif a_inc(&m->_m_waiters); t = r | 0x80000000; diff --git a/system/lib/libc/musl/src/thread/pthread_mutex_trylock.c b/system/lib/libc/musl/src/thread/pthread_mutex_trylock.c index 9c7d8496a8c10..1f34686dd48ee 100644 --- a/system/lib/libc/musl/src/thread/pthread_mutex_trylock.c +++ b/system/lib/libc/musl/src/thread/pthread_mutex_trylock.c @@ -53,6 +53,12 @@ int __pthread_mutex_trylock_owner(pthread_mutex_t *m) } #endif +#if defined(__EMSCRIPTEN__) || !defined(NDEBUG) + // We can get here for normal mutexes too, but only in debug builds + // (where we track ownership purely for debug purposed). + if ((type & 15) == PTHREAD_MUTEX_NORMAL) return 0; +#endif + next = self->robust_list.head; m->_m_next = next; m->_m_prev = &self->robust_list.head; @@ -71,8 +77,11 @@ int __pthread_mutex_trylock_owner(pthread_mutex_t *m) int __pthread_mutex_trylock(pthread_mutex_t *m) { +#if !defined(__EMSCRIPTEN__) || defined(NDEBUG) + /* XXX EMSCRIPTEN always take the slow path in debug builds so we can trap rather than deadlock */ if ((m->_m_type&15) == PTHREAD_MUTEX_NORMAL) return a_cas(&m->_m_lock, 0, EBUSY) & EBUSY; +#endif return __pthread_mutex_trylock_owner(m); } diff --git a/system/lib/pthread/library_pthread.c b/system/lib/pthread/library_pthread.c index a90de5ff40114..cd0c0ba1a71ad 100644 --- a/system/lib/pthread/library_pthread.c +++ b/system/lib/pthread/library_pthread.c @@ -138,13 +138,9 @@ 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) { - _emscripten_init_main_thread_js(&__main_pthread); - // 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'. __main_pthread.self = &__main_pthread; - __main_pthread.stack = (void*)emscripten_stack_get_base(); - __main_pthread.stack_size = emscripten_stack_get_base() - emscripten_stack_get_end(); __main_pthread.detach_state = DT_JOINABLE; // pthread struct robust_list head should point to itself. __main_pthread.robust_list.head = &__main_pthread.robust_list.head; @@ -157,6 +153,11 @@ void _emscripten_init_main_thread(void) { __main_pthread.next = __main_pthread.prev = &__main_pthread; __main_pthread.tsd = (void **)__pthread_tsd_main; + _emscripten_init_main_thread_js(&__main_pthread); + + __main_pthread.stack = (void*)emscripten_stack_get_base(); + __main_pthread.stack_size = emscripten_stack_get_base() - emscripten_stack_get_end(); + _emscripten_thread_mailbox_init(&__main_pthread); _emscripten_thread_mailbox_await(&__main_pthread); } diff --git a/test/other/codesize/test_codesize_minimal_pthreads.size b/test/other/codesize/test_codesize_minimal_pthreads.size index 4e30e5860f7e3..3a243e798e99c 100644 --- a/test/other/codesize/test_codesize_minimal_pthreads.size +++ b/test/other/codesize/test_codesize_minimal_pthreads.size @@ -1 +1 @@ -19526 +19540 diff --git a/test/other/codesize/test_codesize_minimal_pthreads_memgrowth.size b/test/other/codesize/test_codesize_minimal_pthreads_memgrowth.size index 6f51e4c567243..fb23a9262b601 100644 --- a/test/other/codesize/test_codesize_minimal_pthreads_memgrowth.size +++ b/test/other/codesize/test_codesize_minimal_pthreads_memgrowth.size @@ -1 +1 @@ -19527 +19541 diff --git a/test/other/test_pthread_mutex_deadlock.c b/test/other/test_pthread_mutex_deadlock.c new file mode 100644 index 0000000000000..d26d2c045bc46 --- /dev/null +++ b/test/other/test_pthread_mutex_deadlock.c @@ -0,0 +1,20 @@ +#include +#include +#include +#include + +pthread_mutex_t m = PTHREAD_MUTEX_INITIALIZER; + +int main() { + printf("in main\n"); + int rtn = pthread_mutex_lock(&m); + assert(rtn == 0); + + // Attempt to lock a second time. In debug builds this should + // hit an assertion. In release builds this will deadlock and + // never return. + pthread_mutex_lock(&m); + printf("should never get here\n"); + assert(false); + return 0; +} diff --git a/test/test_other.py b/test/test_other.py index a42a2ba95c2cc..78c44891e3b5d 100644 --- a/test/test_other.py +++ b/test/test_other.py @@ -12947,6 +12947,12 @@ def test_pthread_reuse(self): def test_pthread_hello(self, args): self.do_other_test('test_pthread_hello.c', args) + @crossplatform + @node_pthreads + def test_pthread_mutex_deadlock(self): + self.do_runf('other/test_pthread_mutex_deadlock.c', 'pthread mutex deadlock detected', + cflags=['-g'], assert_returncode=NON_ZERO) + @node_pthreads def test_pthread_relocatable(self): self.do_run_in_out_file_test('hello_world.c', cflags=['-sRELOCATABLE']) From d7fa225f6b351e2a79e9b163a1e5c5462d6dc937 Mon Sep 17 00:00:00 2001 From: Sam Clegg Date: Wed, 9 Jul 2025 11:17:04 -0700 Subject: [PATCH 2/2] Update system/lib/libc/musl/src/thread/pthread_mutex_trylock.c Co-authored-by: Alon Zakai --- system/lib/libc/musl/src/thread/pthread_mutex_trylock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/lib/libc/musl/src/thread/pthread_mutex_trylock.c b/system/lib/libc/musl/src/thread/pthread_mutex_trylock.c index 1f34686dd48ee..37dd28fdd5b1a 100644 --- a/system/lib/libc/musl/src/thread/pthread_mutex_trylock.c +++ b/system/lib/libc/musl/src/thread/pthread_mutex_trylock.c @@ -55,7 +55,7 @@ int __pthread_mutex_trylock_owner(pthread_mutex_t *m) #if defined(__EMSCRIPTEN__) || !defined(NDEBUG) // We can get here for normal mutexes too, but only in debug builds - // (where we track ownership purely for debug purposed). + // (where we track ownership purely for debug purposes). if ((type & 15) == PTHREAD_MUTEX_NORMAL) return 0; #endif