Revert 4.0.11 threads normal-mutex debug implementation#26635
Revert 4.0.11 threads normal-mutex debug implementation#26635slowriot wants to merge 6 commits intoemscripten-core:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR reverts Emscripten’s 4.0.11-era debug behavior changes for PTHREAD_MUTEX_NORMAL in musl, restoring the pre-4.0.11 fast-path semantics to address the reported allocator regression under -pthread + -sWASM_WORKERS with assertions/debug-enabled system libraries.
Changes:
- Revert musl pthread normal-mutex debug behavior to always use the historical fast path (and remove the debug deadlock assertion path).
- Disable
test_pthread_mutex_deadlockwhile preserving the underlying test source for potential future re-enable. - Add a browser regression test for the allocator crash and update pthreads codesize baselines.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
system/lib/libc/musl/src/thread/pthread_mutex_trylock.c |
Restores fast-path behavior for normal mutex trylock in debug builds. |
system/lib/libc/musl/src/thread/pthread_mutex_lock.c |
Restores fast-path behavior for normal mutex lock in debug builds. |
system/lib/libc/musl/src/thread/pthread_mutex_timedlock.c |
Removes Emscripten debug deadlock assertion and restores normal fast-path. |
test/wasm_worker/pthread_mutex_debug_allocator_regression.cpp |
Adds a wasm-worker allocator churn repro/regression test. |
test/test_browser.py |
Wires the new browser regression test into the wasm worker test suite. |
test/test_other.py |
Disables the deadlock test while documenting the rollback intent. |
test/other/test_pthread_mutex_deadlock.c |
Updates comments to reflect that deadlock-assert behavior is reverted/disabled. |
test/codesize/test_codesize_minimal_pthreads.json |
Updates expected code size baselines after the revert. |
test/codesize/test_codesize_minimal_pthreads_memgrowth.json |
Updates expected code size baselines (memgrowth variant). |
AUTHORS |
Adds a new author entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static void main_loop() { | ||
| static unsigned ticks; | ||
| static bool reported; | ||
| new std::uint8_t{0}; |
There was a problem hiding this comment.
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 ?)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I would rather have just the C version of the test. We tend to prefer C tests for various reasons.
Resolves #26619.
For motivation, please see description and discussion at #26622.
This PR simply reverts the changes between 4.0.10 and 4.0.11 around debug behaviour for
PTHREAD_MUTEX_NORMAL.This also disables the
test_pthread_mutex_deadlocktest which was testing this functionality, but preserves the code and test setup so it can be re-enabled in future if needed.Finally, this PR adds a test to detect the regression, as in #26622. The test passes with this fix.