Skip to content

Move most of Pthread.initRuntime into native code. NFC #14775

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.

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

Merged
merged 1 commit into from
Jul 28, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 0 additions & 1 deletion emcc.py
Original file line number Diff line number Diff line change
Expand Up @@ -1863,7 +1863,6 @@ def default_setting(name, new_default):
'_emscripten_get_global_libc',
'_emscripten_main_browser_thread_id',
'_emscripten_main_thread_process_queued_calls',
'_emscripten_register_main_browser_thread_id',
'_emscripten_run_in_main_runtime_thread_js',
'_emscripten_stack_set_limits',
'_emscripten_sync_run_in_main_thread_2',
Expand Down
46 changes: 5 additions & 41 deletions src/library_pthread.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,8 @@
var LibraryPThread = {
$PThread__postset: 'if (!ENVIRONMENT_IS_PTHREAD) PThread.initMainThreadBlock();',
$PThread__deps: ['_emscripten_thread_init',
'emscripten_register_main_browser_thread_id',
'emscripten_futex_wake', '$killThread',
'$cancelThread', '$cleanupThread',
#if USE_ASAN || USE_LSAN
, '$withBuiltinMalloc'
#endif
],
$PThread: {
// Contains all Workers that are idle/unused and not currently hosting an
Expand All @@ -37,33 +33,7 @@ var LibraryPThread = {
}
#endif
},
initRuntime: function() {
#if USE_ASAN || USE_LSAN
// When sanitizers are enabled, malloc is normally instrumented to call
// sanitizer code that checks some things about pthreads. As we are just
// setting up the main thread here, and are not ready for such calls,
// call malloc directly.
withBuiltinMalloc(function () {
#endif

var tb = _malloc({{{ C_STRUCTS.pthread.__size__ }}});

for (var i = 0; i < {{{ C_STRUCTS.pthread.__size__ }}}/4; ++i) HEAPU32[tb/4+i] = 0;

// 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'.
{{{ makeSetValue('tb', C_STRUCTS.pthread.self, 'tb', 'i32') }}};

// pthread struct robust_list head should point to itself.
var headPtr = tb + {{{ C_STRUCTS.pthread.robust_list }}};
{{{ makeSetValue('headPtr', 0, 'headPtr', 'i32') }}};

// Allocate memory for thread-local storage.
var tlsMemory = _malloc({{{ cDefine('PTHREAD_KEYS_MAX') * 4 }}});
for (var i = 0; i < {{{ cDefine('PTHREAD_KEYS_MAX') }}}; ++i) HEAPU32[tlsMemory/4+i] = 0;
Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.tsd }}} ) >> 2, tlsMemory); // Init thread-local-storage memory array.
Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.tid }}} ) >> 2, tb); // Main thread ID.
Copy link
Member

Choose a reason for hiding this comment

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

I understand where the code above for tb.self etc. has moved to (C), but what now handles the TLS init?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed I probably should have explained that. musl uses statically allocated tsd for the main thread.

It gets assigned on first use here:

if (!self->tsd) self->tsd = __pthread_tsd_main;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So we can avoid malloc completely!

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, thanks... and nice to avoid malloc!

So this is for pthread_key* - what about C/C++ level TLS? Is that handled in another way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, C/C++ TLS variables are handled separately to the dynamically created TLS keys in pthread_key_create


initRuntime: function(tb) {
#if PTHREADS_PROFILING
PThread.createProfilerBlock(tb);
PThread.setThreadName(tb, "Browser main thread");
Expand All @@ -74,14 +44,9 @@ var LibraryPThread = {
// globals which act as a form of TLS. Global constructors trying
// to access this value will read the wrong value, but that is UB anyway.
__emscripten_thread_init(tb, /*isMainBrowserThread=*/!ENVIRONMENT_IS_WORKER, /*isMainRuntimeThread=*/1);
_emscripten_register_main_browser_thread_id(tb);
#if ASSERTIONS
PThread.mainRuntimeThread = true;
#endif

#if USE_ASAN || USE_LSAN
});
#endif
},
initWorker: function() {
#if USE_CLOSURE_COMPILER
Expand Down Expand Up @@ -568,6 +533,7 @@ var LibraryPThread = {
pthread.worker.postMessage({ 'cmd': 'cancel' });
},

$spawnThread__deps: ['$zeroMemory'],
$spawnThread: function(threadParams) {
if (ENVIRONMENT_IS_PTHREAD) throw 'Internal Error! spawnThread() can only ever be called from main application thread!';

Expand All @@ -582,10 +548,8 @@ var LibraryPThread = {
PThread.runningWorkers.push(worker);

// Allocate memory for thread-local storage and initialize it to zero.
var tlsMemory = _malloc({{{ cDefine('PTHREAD_KEYS_MAX') }}} * 4);
for (var i = 0; i < {{{ cDefine('PTHREAD_KEYS_MAX') }}}; ++i) {
{{{ makeSetValue('tlsMemory', 'i*4', 0, 'i32') }}};
}
var tlsMemory = _malloc({{{ cDefine('PTHREAD_KEYS_MAX') * 4 }}});
zeroMemory(tlsMemory, {{{ cDefine('PTHREAD_KEYS_MAX') * 4 }}});

var stackHigh = threadParams.stackBase + threadParams.stackSize;

Expand Down Expand Up @@ -822,7 +786,7 @@ var LibraryPThread = {
// Allocate thread block (pthread_t structure).
var threadInfoStruct = _malloc({{{ C_STRUCTS.pthread.__size__ }}});
// zero-initialize thread structure.
for (var i = 0; i < {{{ C_STRUCTS.pthread.__size__ }}} >> 2; ++i) HEAPU32[(threadInfoStruct>>2) + i] = 0;
zeroMemory(threadInfoStruct, {{{ C_STRUCTS.pthread.__size__ }}});
{{{ makeSetValue('pthread_ptr', 0, 'threadInfoStruct', 'i32') }}};

// The pthread struct has a field that points to itself - this is used as a
Expand Down
4 changes: 4 additions & 0 deletions system/lib/libc/musl/src/thread/pthread_getspecific.c
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@
static void *__pthread_getspecific(pthread_key_t k)
{
struct pthread *self = __pthread_self();
// XXX EMSCRIPTEN: self->tsd can be NULL in the case of the
// main thread where pthread_key_create is not called.
// See __pthread_key_create for where it get assigned.
if (!self->tsd) return NULL;
Copy link
Collaborator

@kleisauke kleisauke Jul 28, 2021

Choose a reason for hiding this comment

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

Instead of this check, perhaps we could ensure that self->tsd is always initialized? This also matches musl's convention, which seems to always assign it to __pthread_tsd_main once the first thread is created:

static void *dummy_tsd[1] = { 0 };
weak_alias(dummy_tsd, __pthread_tsd_main);

self->tsd = (void **)__pthread_tsd_main;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is that we have at least one test that calls pthread_getspecific before any threads are created and expects it to work: posixtest.test_pthread_key_create_2_1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or at least the test expects it to return NULL and not crash

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah I see, that test looks odd. When compiling with GCC it also complains that key is not initialized.

conformance/interfaces/pthread_key_create/2-1.c:33:14: error: ‘key’ is used uninitialized [-Werror=uninitialized]
   33 |         rc = pthread_getspecific(key);
      |              ^~~~~~~~~~~~~~~~~~~~~~~~
conformance/interfaces/pthread_key_create/2-1.c:29:23: note: ‘key’ declared here
   29 |         pthread_key_t key;
      |                       ^~~
cc1: all warnings being treated as errors

But running it locally and on Alpine (which also uses musl) can be done without any problems.

$ cd ./tests/third_party/posixtestsuite
$ docker run -v $(pwd):/app -w /app -it alpine:latest \
      apk add build-base && \
      POSIX_TARGET=conformance/interfaces/pthread_key_create make LDFLAGS='-pthread -Wno-error=uninitialized'
...
conformance/interfaces/pthread_key_create/2-1: build: PASS
conformance/interfaces/pthread_key_create/2-1: link: PASS
...
conformance/interfaces/pthread_key_create/2-1: execution: PASS

Do you think it makes sense to initialize tsd in __emscripten_pthread_data_constructor? Similar to the logic in pthread_key_create.

/* This can only happen in the main thread before
* pthread_create has been called. */
if (!self->tsd) self->tsd = __pthread_tsd_main;

Though, the current approach would also be sufficient.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, my test command for Alpine was wrong (it ran the test on my own system). This also seems to segfault on Alpine.

$ docker run -v $(pwd):/app -w /app -it alpine:latest sh -c "\
    apk add build-base && \
    POSIX_TARGET=conformance/interfaces/pthread_key_create make LDFLAGS='-pthread -Wno-error=uninitialized'"
$ cat logfile
conformance/interfaces/pthread_key_create/1-1: build: PASS
conformance/interfaces/pthread_key_create/1-1: link: PASS
conformance/interfaces/pthread_key_create/1-2: build: PASS
conformance/interfaces/pthread_key_create/1-2: link: PASS
conformance/interfaces/pthread_key_create/2-1: build: PASS
conformance/interfaces/pthread_key_create/2-1: link: PASS
conformance/interfaces/pthread_key_create/3-1: build: PASS
conformance/interfaces/pthread_key_create/3-1: link: PASS
conformance/interfaces/pthread_key_create/speculative/5-1: build: PASS
conformance/interfaces/pthread_key_create/speculative/5-1: link: PASS
conformance/interfaces/pthread_key_create/1-1: execution: PASS
conformance/interfaces/pthread_key_create/1-2: execution: PASS
conformance/interfaces/pthread_key_create/2-1: execution: INTERRUPTED: Output: 
Segmentation fault (core dumped)
conformance/interfaces/pthread_key_create/3-1: execution: PASS
conformance/interfaces/pthread_key_create/speculative/5-1: execution: FAILED: Output: 
Test FAILED: EAGAIN was returned before the key limit was exceeded

Perhaps we should report this upstream.

return self->tsd[k];
}

Expand Down
26 changes: 15 additions & 11 deletions system/lib/pthread/library_pthread.c
Original file line number Diff line number Diff line change
Expand Up @@ -405,15 +405,10 @@ EMSCRIPTEN_RESULT emscripten_wait_for_call_i(
return res;
}

static pthread_t main_browser_thread_id_ = 0;

void emscripten_register_main_browser_thread_id(
pthread_t main_browser_thread_id) {
main_browser_thread_id_ = main_browser_thread_id;
}
static struct pthread __main_pthread;

pthread_t emscripten_main_browser_thread_id() {
return main_browser_thread_id_;
return &__main_pthread;
}

int _emscripten_do_dispatch_to_thread(pthread_t target_thread, em_queued_call* call) {
Expand Down Expand Up @@ -886,8 +881,8 @@ int _emscripten_call_on_thread(int forceAsync, pthread_t targetThread, EM_FUNC_S
// the main thread is waiting, we wake it up before waking up any workers.
EMSCRIPTEN_KEEPALIVE void* _emscripten_main_thread_futex;

EM_JS(void, initPthreadsJS, (void), {
PThread.initRuntime();
EM_JS(void, initPthreadsJS, (void* tb), {
PThread.initRuntime(tb);
})

// We must initialize the runtime at the proper time, which is after memory is
Expand All @@ -898,6 +893,15 @@ EM_JS(void, initPthreadsJS, (void), {
EMSCRIPTEN_KEEPALIVE
__attribute__((constructor(48)))
void __emscripten_pthread_data_constructor(void) {
initPthreadsJS();
pthread_self()->locale = &libc.global_locale;
initPthreadsJS(&__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;
// pthread struct robust_list head should point to itself.
Copy link
Member

Choose a reason for hiding this comment

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

indentation looks wrong here

Suggested change
// pthread struct robust_list head should point to itself.
// pthread struct robust_list head should point to itself.

__main_pthread.robust_list.head = &__main_pthread.robust_list;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
__main_pthread.robust_list.head = &__main_pthread.robust_list;
__main_pthread.robust_list.head = &__main_pthread.robust_list.head;

This would avoid depending on head being the initial field, or is that bad for some reason?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Honestly I was just trying to mimic exactly what the JS code was doing. I don't really understand the purpose of this assignment.

Copy link
Member

Choose a reason for hiding this comment

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

Me either, but the actual address is identical in both cases - just that writing .head would still work if we reorder the struct in the future?


// Main thread ID.
__main_pthread.tid = (long)&__main_pthread;

__main_pthread.locale = &libc.global_locale;
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ M
N
O
P
Q
t
u
v
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ $emscripten_current_thread_process_queued_calls
$emscripten_get_global_libc
$emscripten_main_thread_process_queued_calls
$emscripten_proxy_main
$emscripten_register_main_browser_thread_id
$emscripten_run_in_main_runtime_thread_js
$emscripten_stack_set_limits
$emscripten_sync_run_in_main_thread
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
16031
16084