WASIp3 Cooperative Threading#799
Conversation
|
Ok I've extracted out a number of issues as follow-up from this PR:
Otherwise this is now green in wasi-sdk so I'm going to merge this PR here. More work is going to be required for coop threads fully working, and even wasip3 without coop threads isn't quite production ready just yet. That being said I think everything's in a reasonable enough state to at least start testing externally in the ecosystem. |
This is all written for this repo and managed here, no need to keep this in sync with other sources. Keep it up to the same style as the rest of the repo.
It's a bit confusing now with coop threads
The previous implementation would have a sem_wait waiter go back to sleep in the presence of multiple waiters because the count would still be negative. The interpretation of the `__count` field is now "number of permits to hand out" and can't go negative, meaning that when a waiter is woken it'll continue if it sees the increment or otherwise goes back to sleep.
The previous implementation of pthread_cond_wait did a normal `pthread_mutex_unlock` before enqueueing the current thread on the waiter list. This implicitly performs a yield, however, meaning that another thread could run, send a signal, and then when the original thread enqueues itself the signal is lost. This is fixed by adding an auxiliary function and parameter `int yield` which is set to 0 on this path which only unsuspends a thread without yielding.
Use the same helper function that non-coop threads use to register when a lock is acquired.
No need to re-acquire it in `make_absolute` because it's already held within the sole caller.
When a thread is woken it might further modify the list of threads that are being woken, so instead make this operation "atomic" by taking the entire list at the beginning and then iterating over it locally without looking at modifications to `list`
Once joined the thread is free'd and can't further be modified.
Remove `once_state.control` as it was only written, never read, and was otherwise tricky state to keep track of. Additionally leave a comment saying that while the current implementation is a bit sub-optimal it should be sufficient for correctness for now.
Also ignore the test that required this fix because it's just UB
The joiner will deallocate a thread's stack, meaning that when the original thread resumed it might be a UAF of its own stack.
|
Ok I've gone through a few iterations of llm-assisted review now which discovered a number of bugs. Everything should be fixed/tested now. Minor things still remain, but within the realm of "not really that consequential". I'll note that I've filed a few more issues as I went along (the list above expanded) and @TartanLlama you should do a once-over of my commits when you get back. Finally, I also disabled a few more open-posix-testsuite tests due to them exercising UB. I'm a bit suspicious that a nontrivial number of tests in this open test suite seem to do UB (e.g. join a detached thread, detach a joined thread, etc), but alas. Better than nothing! In the meantime I'm still going to merge this to unblock a wasi-sdk release which will then, circularly, unblock this repository updating to test all of this in CI. |
2 participants
Currently a draft while the LLVM patches merge and I get all of this ready for review