[pthreads] Initialize TLS on workers in MINIMAL_RUNTIME mode. #24646
Conversation
TLS was not initialized on workers causing the tls_base address to be 0 and write to the wrong memory location. Fixes emscripten-core#21528 and is also needed for PR emscripten-core#24565
| @@ -66,10 +69,6 @@ function initRuntime(wasmExports) { | |||
| writeStackCookie(); | |||
| #endif | |||
|
|
|||
| #if PTHREADS | |||
| PThread.tlsInitFunctions.push(wasmExports['_emscripten_tls_init']); | |||
There was a problem hiding this comment.
Does this not remove it from the main thread as well? (And the new code only re-adds it for pthreads)
Or is this not needed on the main thread? But from libpthread.js it looks like it is run there.
There was a problem hiding this comment.
It does remove it from the main thread, which was not my intention. I'm amazed that worked. I guess there's not much testing for minimal+pthread.
Ideally this code would be in some shared spot that both minimal runtime + regular runtime would share, but I think that's going to be a bigger refactor.
There was a problem hiding this comment.
Sounds like a hole in testing, yeah... we do have pthreads tests for minimal runtime, but without TLS, I guess, until this PR.
| #include <assert.h> | ||
| #include <stdio.h> | ||
|
|
||
| static _Thread_local int thread_local = 0; |
There was a problem hiding this comment.
Would it perhaps make sense to do = 3 here and assert that thread_local starts with this value in moth main and in `thread_start?
| def test_pthread_c_thread_local(self): | ||
| if self.get_setting('MINIMAL_RUNTIME') and is_sanitizing(self.cflags): | ||
| self.skipTest('MINIMAL_RUNTIME + threads + asan does not work') | ||
| self.do_run_in_out_file_test('pthread/test_pthread_c_thread_local.c') |
There was a problem hiding this comment.
Surely we already have tests that cover this?
There was a problem hiding this comment.
It looks like test/pthread/test_pthread_tls.cpp is probably what we want. It happens to use C++ thread_local instead of the C's _Thread_local but I think it should the same thing from the POV of this PR.
- Convert to C - Run in core tests under node. - Remove redundant test added in emscripten-core#24646
- Convert to C - Run in core tests under node. - Remove redundant test added in emscripten-core#24646
- Convert to C - Run in core tests under node. - Remove redundant test added in #24646
TLS was not initialized on workers causing the tls_base address to be 0 and write to the wrong memory location.
Fixes #21528 and is also needed for PR #24565