[clementperon]

With dynamic linking, _emscripten_tls_init allocates a TLS block for each side module on every new thread, but _emscripten_tls_free is never called. Every thread that exits leaks the TLS of every loaded side module (4 KiB per thread for a side module with a 4 KiB thread_local).

Side modules now export _emscripten_tls_free, registerTLSInit collects them, and _emscripten_thread_exit runs them after the thread's TSD destructors (so C++ thread_local destructors still see their data).

The new test_pthread_dylink_tls_free checks heap growth over 100 threads after dlopen() of such a side module; it fails on main and passes with this change. Over 1000 threads the heap grew by 4,108,000 bytes before and 0 after.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces a mechanism to free Thread Local Storage (TLS) blocks allocated by side modules when a thread exits, preventing memory leaks in dynamic linking scenarios. It adds _emscripten_thread_free_tls to clean up these blocks and integrates it into the thread exit sequence. The review feedback correctly identifies potential runtime TypeError issues in non-MAIN_MODULE builds where PThread.tlsFreeFunctions is undefined. It is recommended to wrap both the registration and execution of these TLS free functions in #if MAIN_MODULE preprocessor guards to ensure safety.

[gemini-code-assist]

Code Review

This pull request implements the freeing of side module thread-local storage (TLS) blocks upon thread exit in Emscripten. It registers the _emscripten_tls_free functions of side modules and invokes them during thread cleanup. Feedback on the changes highlights potential runtime TypeError crashes when MAIN_MODULE is disabled, as PThread.tlsFreeFunctions is only defined under that preprocessor guard. It is recommended to add checks or guards when pushing to and iterating over tlsFreeFunctions.

I am having trouble creating individual review comments. Click here to see my feedback.

src/lib/libpthread.js (701-704)

If MAIN_MODULE is disabled, PThread.tlsFreeFunctions is not defined (as its declaration on line 116 is guarded by #if MAIN_MODULE). In this scenario, if tlsFreeFunc is somehow resolved to a truthy value, attempting to call .push() on PThread.tlsFreeFunctions will throw a TypeError. Adding a guard check for PThread.tlsFreeFunctions ensures robustness.

    var tlsFreeFunc = moduleExports?.['_emscripten_tls_free'];
    if (tlsFreeFunc && PThread.tlsFreeFunctions) {
      PThread.tlsFreeFunctions.push(tlsFreeFunc);
    }

src/lib/libpthread.js (1229)

Since PThread.tlsFreeFunctions is only defined when MAIN_MODULE is enabled, calling _emscripten_thread_free_tls when MAIN_MODULE is disabled will result in a TypeError: Cannot read properties of undefined (reading 'forEach'). We should guard the body of _emscripten_thread_free_tls with #if MAIN_MODULE to prevent runtime crashes in non-main-module builds.

  _emscripten_thread_free_tls: () => {
#if MAIN_MODULE
    PThread.tlsFreeFunctions.forEach((f) => f());
#endif
  },

[clementperon]

@sbc100 leak found when brute forcing side module

[clementperon]

@sbc100 Thanks for the review, I have commited the test for proof but IMO we should no add such kind of test no? We should more compile with ASAN / TSAN and just run a normal usage.
Else we can advocate to add one test per allocation which doesn't make sense.
What do you think?

[sbc100]

@sbc100 Thanks for the review, I have commited the test for proof but IMO we should no add such kind of test no? We should more compile with ASAN / TSAN and just run a normal usage. Else we can advocate to add one test per allocation which doesn't make sense. What do you think?

We do support ASAN and we run the whole test_core.py suite in asan mode, but sadly emscripten does not (yet) support TSAN.

[clementperon]

@sbc100 Thanks for the review, I have commited the test for proof but IMO we should no add such kind of test no? We should more compile with ASAN / TSAN and just run a normal usage. Else we can advocate to add one test per allocation which doesn't make sense. What do you think?

We do support ASAN and we run the whole test_core.py suite in asan mode, but sadly emscripten does not (yet) support TSAN.

I have dig a bit into it and currently it can't detect it, because the side-module TLS is allocated with emscripten_builtin_memalign, which aliases raw dlmalloc and sits below the sanitizer allocator :(

[sbc100]

Yes, that makes sense to me. These allocations a very low level, and maybe cannot be tracked like normal allocation using ASan.

I'm happy to land this with the tests you wrote ? Or maybe just one of them ?

[clementperon]

I tried to challenge those tests but they guard two different bugs.
tls_free checks whether side-module TLS gets freed (the leak). tls_free_robust checks when a robust mutex can live in that TLS, and freeing it before the thread-exit robust-list walk corrupts the heap.

Neither catches the other.

[sbc100]

lgtm % comment