-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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; | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of this check, perhaps we could ensure that emscripten/system/lib/libc/musl/src/thread/pthread_create.c Lines 162 to 163 in 4d864df
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem is that we have at least one test that calls There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 emscripten/system/lib/libc/musl/src/thread/pthread_key_create.c Lines 22 to 24 in 8192916
Though, the current approach would also be sufficient. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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) { | ||||||
|
@@ -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 | ||||||
|
@@ -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. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. indentation looks wrong here
Suggested change
|
||||||
__main_pthread.robust_list.head = &__main_pthread.robust_list; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This would avoid depending on There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
|
||||||
// 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 |
---|---|---|
|
@@ -14,7 +14,6 @@ M | |
N | ||
O | ||
P | ||
Q | ||
t | ||
u | ||
v | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
16031 | ||
16084 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
emscripten/system/lib/libc/musl/src/thread/pthread_key_create.c
Line 24 in b752bd6
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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