-
Notifications
You must be signed in to change notification settings - Fork 239
Fix races around pthread exit and join #409
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 |
|---|---|---|
|
|
@@ -164,14 +164,6 @@ static void __pthread_exit(void *result) | |
| self->prev->next = self->next; | ||
| self->prev = self->next = self; | ||
|
|
||
| #ifndef __wasilibc_unmodified_upstream | ||
| /* On Linux, the thread is created with CLONE_CHILD_CLEARTID, | ||
| * and this lock will unlock by kernel when this thread terminates. | ||
| * So we should unlock it here in WebAssembly. | ||
| * See also set_tid_address(2) */ | ||
| __tl_unlock(); | ||
| #endif | ||
|
|
||
| #ifdef __wasilibc_unmodified_upstream | ||
| if (state==DT_DETACHED && self->map_base) { | ||
| /* Detached threads must block even implementation-internal | ||
|
|
@@ -190,9 +182,6 @@ static void __pthread_exit(void *result) | |
| } | ||
| #else | ||
| if (state==DT_DETACHED && self->map_base) { | ||
| // __syscall(SYS_exit) would unlock the thread, list | ||
| // do it manually here | ||
| __tl_unlock(); | ||
| free(self->map_base); | ||
| // Can't use `exit()` here, because it is too high level | ||
| return; | ||
|
|
@@ -212,10 +201,15 @@ static void __pthread_exit(void *result) | |
| #ifdef __wasilibc_unmodified_upstream | ||
| for (;;) __syscall(SYS_exit, 0); | ||
| #else | ||
| // __syscall(SYS_exit) would unlock the thread, list | ||
| // do it manually here | ||
| __tl_unlock(); | ||
| // Can't use `exit()` here, because it is too high level | ||
|
|
||
| /* On Linux, the thread is created with CLONE_CHILD_CLEARTID, | ||
| * and the lock (__thread_list_lock) will be unlocked by kernel when | ||
| * this thread terminates. | ||
| * See also set_tid_address(2) | ||
| * | ||
| * In WebAssembly, we leave it to wasi_thread_start instead. | ||
| */ | ||
|
Member
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. Could we instead call In the first case we are unlocking right before notifying the joiner that they can call free and in the later case we are unlocking right before we call (I'm suggesting this so that we can avoid re-implementing
Member
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. Actually, what was wrong with the single
Contributor
Author
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.
this PR doesn't re-implement
Contributor
Author
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.
Member
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. I agree the double unlock thing looks like a bug. But the joiner is not waiting on the
Contributor
Author
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 joiner uses |
||
| #endif | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,4 +28,21 @@ wasi_thread_start: | |
| local.get 1 # start_arg | ||
| call __wasi_thread_start_C | ||
|
|
||
| # Unlock thread list. (as CLONE_CHILD_CLEARTID would do for Linux) | ||
| # | ||
| # Note: once we unlock the thread list, our "map_base" can be freed | ||
| # by a joining thread. It's safe as we are in ASM and no longer use | ||
| # our C stack or pthread_t. It's impossible to do this safely in C | ||
| # because there is no way to tell the C compiler not to use C stack. | ||
| i32.const __thread_list_lock | ||
| i32.const 0 | ||
| i32.atomic.store 0 | ||
| # As an optimization, we can check tl_lock_waiters here. | ||
| # But for now, simply wake up unconditionally as | ||
| # CLONE_CHILD_CLEARTID does. | ||
| i32.const __thread_list_lock | ||
| i32.const 1 | ||
| memory.atomic.notify 0 | ||
| drop | ||
|
Member
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. How about putting all this new code a new local function called Otherwise this lgtm now.
Contributor
Author
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. i added a comment. |
||
|
|
||
| end_function | ||
Uh oh!
There was an error while loading. Please reload this page.