Skip to content

Revert 4.0.11 threads normal-mutex debug implementation #26635

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
slowriot wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Revert 4.0.11 threads normal-mutex debug implementation #26635

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6f7004e
add unit test for regression #26619
slowriot Apr 6, 2026
a3c997b
revert 4.0.11 debug deadlock slow patch changes, resolves #26619
slowriot Apr 6, 2026
0c28704
use btest_exit for allocator regression test
slowriot Apr 7, 2026
a77220e
remove self.set_setting('ASSERTIONS')
slowriot Apr 7, 2026
40e8bfa
restore original test comment
slowriot Apr 7, 2026
47fa005
Merge branch 'main' into revert-pthread-mutex-debug-deadlock-detection
slowriot Apr 7, 2026
01b389c
replace C++ test with C equivalent
slowriot Apr 13, 2026
dd8d0da
Merge branch 'revert-pthread-mutex-debug-deadlock-detection' of https…
slowriot Apr 13, 2026
4dda4be
Merge branch 'main' into revert-pthread-mutex-debug-deadlock-detection
slowriot Apr 13, 2026
48cff0d
use C reproducer in test_other for mutex regression
slowriot Apr 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,3 +602,4 @@ a license to everyone to use it as detailed in LICENSE.)
* Christian Lloyd <clloyd@teladochealth.com> (copyright owned by Teladoc Health, Inc.)
* Sean Morris <sean@seanmorr.is>
* Mitchell Wills <mwills@google.com> (copyright owned by Google, Inc.)
* Eugene Hopkinson <slowriot@armchair.software>
3 changes: 0 additions & 3 deletions system/lib/libc/musl/src/thread/pthread_mutex_lock.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,9 @@

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);
}
Expand Down
12 changes: 0 additions & 12 deletions system/lib/libc/musl/src/thread/pthread_mutex_timedlock.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
#include "pthread_impl.h"

#ifdef __EMSCRIPTEN__
#include <assert.h>
#endif

#ifndef __EMSCRIPTEN__
#define IS32BIT(x) !((x)+0x80000000ULL>>32)
#define CLAMP(x) (int)(IS32BIT(x) ? (x) : 0x7fffffffU+((0ULL+(x))>>63))
Expand Down Expand Up @@ -61,12 +57,9 @@ 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;
Expand All @@ -89,11 +82,6 @@ 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;
Expand Down
9 changes: 0 additions & 9 deletions system/lib/libc/musl/src/thread/pthread_mutex_trylock.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,12 +53,6 @@ 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 purposes).
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;
Expand All @@ -77,11 +71,8 @@ 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);
}

Expand Down
8 changes: 4 additions & 4 deletions test/codesize/test_codesize_minimal_pthreads.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"a.out.js": 7363,
"a.out.js.gz": 3604,
"a.out.nodebug.wasm": 19003,
"a.out.nodebug.wasm.gz": 8786,
"total": 26366,
"total_gz": 12390,
"a.out.nodebug.wasm": 18991,
"a.out.nodebug.wasm.gz": 8771,
"total": 26354,
"total_gz": 12375,
"sent": [
"a (memory)",
"b (exit)",
Expand Down
8 changes: 4 additions & 4 deletions test/codesize/test_codesize_minimal_pthreads_memgrowth.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"a.out.js": 7765,
"a.out.js.gz": 3810,
"a.out.nodebug.wasm": 19004,
"a.out.nodebug.wasm.gz": 8787,
"total": 26769,
"total_gz": 12597,
"a.out.nodebug.wasm": 18992,
"a.out.nodebug.wasm.gz": 8772,
"total": 26757,
"total_gz": 12582,
"sent": [
"a (memory)",
"b (exit)",
Expand Down
5 changes: 2 additions & 3 deletions test/other/test_pthread_mutex_deadlock.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,8 @@ int main() {
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.
Comment thread
slowriot marked this conversation as resolved.
// Attempt to lock a second time. In debug builds with the #24607 behavior
// enabled this should hit an assertion. Without that behavior it deadlocks.
pthread_mutex_lock(&m);
printf("should never get here\n");
assert(false);
Expand Down
5 changes: 5 additions & 0 deletions test/test_browser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5243,6 +5243,11 @@ def test_wasm_worker_proxied_function(self):
# Test that code does not crash in ASSERTIONS-disabled builds
self.btest('wasm_worker/proxied_function.c', expected='0', cflags=['--js-library', test_file('wasm_worker/proxied_function.js'), '-sWASM_WORKERS', '-sASSERTIONS=0'])

def test_wasm_worker_pthread_mutex_debug_allocator_regression(self):
Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a link to https://github.com/emscripten-core/emscripten/issues/26619 here?

(And also in the test source code).

Copy link
Copy Markdown
Contributor Author

@slowriot slowriot Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in latest (in case this branch is still potentially useful)

self.set_setting('ASSERTIONS')
Comment thread
slowriot marked this conversation as resolved.
Outdated
self.btest('wasm_worker/pthread_mutex_debug_allocator_regression.cpp',
expected='0', cflags=['-pthread', '-sWASM_WORKERS'])
Comment thread
slowriot marked this conversation as resolved.
Outdated

@no_firefox('no 4GB support yet')
@no_2gb('uses MAXIMUM_MEMORY')
@no_4gb('uses MAXIMUM_MEMORY')
Expand Down
4 changes: 4 additions & 0 deletions test/test_other.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11721,6 +11721,10 @@ def test_pthread_reuse(self):
def test_pthread_hello(self, args):
self.do_other_test('test_pthread_hello.c', args)

# Preserved for future reinstatement of the debug deadlock check from #24607.
# This rollback intentionally removes that behavior and returns debug builds to
# the pre-4.0.11 fast-path behavior for PTHREAD_MUTEX_NORMAL.
@disabled('disabled by revert-pthread-mutex-debug-deadlock-detection rollback')
@crossplatform
@requires_pthreads
def test_pthread_mutex_deadlock(self):
Expand Down
25 changes: 25 additions & 0 deletions test/wasm_worker/pthread_mutex_debug_allocator_regression.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
#include <cstdint>
#include <emscripten.h>
#include <emscripten/wasm_worker.h>

static void worker_loop() {
for (;;) {
delete new std::uint8_t{0};
Comment thread
slowriot marked this conversation as resolved.
Outdated
}
}

static void main_loop() {
static unsigned ticks;
static bool reported;
new std::uint8_t{0};
Comment thread
slowriot marked this conversation as resolved.
Outdated
Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The the bug still reproduce if you convert this from .cpp to .c and use malloc and free?

(You might need to add --no-builtin maybe ?)

Copy link
Copy Markdown
Contributor Author

@slowriot slowriot Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this test is basically equivalent to the CPP and C repros I provided in the issue description at #26619. Only the C++ path is tested here because I understand them to be identical. --no-builtin shouldn't be necessary as the tests are built with -O0 by default which should prevent those calls being optimised away. Let me know if you'd like me to add a pure C test as well just to be belt-and-braces about it, but I wouldn't normally bother because from previous stack traces (as per the issue report) we can see that malloc and free are always the culprit calls behind the scenes when using new and delete.

Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather have just the C version of the test. We tend to prefer C tests for various reasons.

Copy link
Copy Markdown
Contributor Author

@slowriot slowriot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbc100 C++ test replaced with the C equivalent in the latest version. Please let me know if you'd like any other changes.

Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you able to reproduce the issue using that C version on the main branch. For me, it doesn't reproduce with the C version on main.

Copy link
Copy Markdown
Contributor Author

@slowriot slowriot Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbc100 just catching up on those now, will confirm fully shortly, but it seems that my original repros detailed in #26619 (comment) no longer crash on main, at least. It'll take me a while to test against the full version of my prod code, but at least those two small code samples now run cleanly.

Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can can still repro the crash with the C++ version on main:

$ ./emcc -g test.cc -pthread -sWASM_WORKERS && node ./a.out.js
start
i=0
i=1000000
i=2000000
i=3000000
i=4000000
i=5000000
i=6000000
i=7000000
i=8000000
i=9000000
i=10000000
i=11000000
i=12000000
i=13000000
i=14000000
i=15000000
Aborted(native code called abort())
/usr/local/google/home/sbc/dev/wasm/emscripten/a.out.js:169
    throw toThrow;
    ^

RuntimeError: Aborted(native code called abort())
    at abort (/usr/local/google/home/sbc/dev/wasm/emscripten/a.out.js:887:11)
    at __abort_js (/usr/local/google/home/sbc/dev/wasm/emscripten/a.out.js:1880:7)
    at a.out.wasm.abort (wasm://wasm/a.out.wasm-001433aa:wasm-function[230]:0x657b)
    at a.out.wasm.emscripten_builtin_malloc (wasm://wasm/a.out.wasm-001433aa:wasm-function[238]:0x7719)
    at a.out.wasm.operator_new_impl(unsigned long) (wasm://wasm/a.out.wasm-001433aa:wasm-function[260]:0x9b26)
    at a.out.wasm.operator new(unsigned long) (wasm://wasm/a.out.wasm-001433aa:wasm-function[259]:0x9afb)
    at a.out.wasm.main::$_1::operator()() const (wasm://wasm/a.out.wasm-001433aa:wasm-function[31]:0xa47)
    at a.out.wasm.main::$_1::__invoke() (wasm://wasm/a.out.wasm-001433aa:wasm-function[29]:0x94d)
    at callUserCallback (/usr/local/google/home/sbc/dev/wasm/emscripten/a.out.js:1539:16)
    at Object.runIter (/usr/local/google/home/sbc/dev/wasm/emscripten/a.out.js:2251:9)

Node.js v22.22.0

Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 Apr 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

#include <cstdint>
#include <cstdio>
#include <emscripten.h>
#include <emscripten/wasm_worker.h>

auto main() -> int {
  emscripten_wasm_worker_post_function_v(emscripten_malloc_wasm_worker(1024 * 1024), []{
    printf("start\n");
    for (int i = 0; i < 70'000'000; ++i) {
      if ((i % 1000000) == 0) {
        printf("i=%d\n", i);
      }
      delete new uint8_t{0};
    }
    printf("done\n");
  });
  emscripten_set_main_loop([]{new uint8_t{0};}, 0, false);
  return EXIT_SUCCESS;
}

if (!reported && ++ticks == 120) {
reported = true;
REPORT_RESULT(0);
}
}

int main() {
emscripten_wasm_worker_post_function_v(emscripten_malloc_wasm_worker(1024 * 1024), worker_loop);
emscripten_set_main_loop(main_loop, 0, false);
return 0;
}
Loading