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

sbc100 · 2021-07-28T05:25:54Z

This also avoids the use of malloc both for the main pthread struct and
for its TLS vars. This in turn avoids the use of withBuiltinMalloc
since we no longer use malloc here.

kleisauke

LGTM! I added a small inline comment, which might also be resolved in a separate PR.

kleisauke · 2021-07-28T07:39:39Z

system/lib/libc/musl/src/thread/pthread_getspecific.c

+	// 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;


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:

emscripten/system/lib/libc/musl/src/thread/pthread_create.c

Lines 162 to 163 in 4d864df

static void *dummy_tsd[1] = { 0 };

weak_alias(dummy_tsd, __pthread_tsd_main);

emscripten/system/lib/libc/musl/src/thread/pthread_create.c

Line 201 in 4d864df

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

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

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

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.

emscripten/system/lib/libc/musl/src/thread/pthread_key_create.c

Lines 22 to 24 in 8192916

/* 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.

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.

This also avoids the use of malloc both for the main pthread struct and for its TLS vars. This in turn avoids the use of `withBuiltinMalloc` since we no longer use malloc here.

kripken · 2021-07-28T16:59:54Z

system/lib/pthread/library_pthread.c

+  // 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.


indentation looks wrong here

Suggested change

// pthread struct robust_list head should point to itself.

// pthread struct robust_list head should point to itself.

kripken · 2021-07-28T17:01:23Z

system/lib/pthread/library_pthread.c

+  // 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.
+  __main_pthread.robust_list.head = &__main_pthread.robust_list;


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?

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

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?

kripken · 2021-07-28T17:02:54Z

src/library_pthread.js

-      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.


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

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

It gets assigned on first use here:

emscripten/system/lib/libc/musl/src/thread/pthread_key_create.c

Line 24 in b752bd6

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

So we can avoid malloc completely!

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?

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

kripken · 2021-07-28T17:22:25Z

Oops, once again I approved a PR without noticing it had automerge, so there are still some comments from me and @kleisauke . Sorry about that, I really have to be careful about looking for automerge all the time...

sbc100 · 2021-07-28T18:53:16Z

Sorry @kripken I will avoid using auto-merge so much in future. And I will address all the comments here.

kripken · 2021-07-28T19:55:00Z

No, I think it's fine to use auto-merge... I just need to get used to it.

sbc100 requested review from kripken and kleisauke July 28, 2021 05:28

kleisauke approved these changes Jul 28, 2021

View reviewed changes

Move most of Pthread.initRuntime into native code. NFC

09c8871

This also avoids the use of malloc both for the main pthread struct and for its TLS vars. This in turn avoids the use of `withBuiltinMalloc` since we no longer use malloc here.

sbc100 force-pushed the pthead_avoid_malloc branch from 3637f1e to 09c8871 Compare July 28, 2021 16:22

sbc100 enabled auto-merge (squash) July 28, 2021 16:26

kripken reviewed Jul 28, 2021

View reviewed changes

sbc100 merged commit b752bd6 into main Jul 28, 2021

sbc100 deleted the pthead_avoid_malloc branch July 28, 2021 17:18

sbc100 mentioned this pull request Sep 16, 2021

Use musl implementation of pthread API where possible #15061

Closed

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

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

	// 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;
	__main_pthread.robust_list.head = &__main_pthread.robust_list.head;

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

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

Uh oh!

Conversation

sbc100 commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kleisauke left a comment

Choose a reason for hiding this comment

Uh oh!

kleisauke Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Jul 28, 2021

Uh oh!

sbc100 commented Jul 28, 2021

Uh oh!

kripken commented Jul 28, 2021

Uh oh!

Uh oh!

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

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

sbc100 commented Jul 28, 2021 •

edited

Loading

kleisauke Jul 28, 2021 •

edited

Loading