Add implementation for wasi_thread_spawn() #1786
Conversation
The parameter should be of type integer (i) instead of buffer; the native code should never access the data pointed by the pointer, therefore having the argument as a pointer requires additional unnecessary conversion.
loganek
force-pushed
the
loganek/thread-implementation
branch
from
ac51ca0
to
52fc95b
Compare
core/iwasm/libraries/lib-wasi-threads/lib_wasi_threads_wrapper.c
Outdated
Show resolved
Hide resolved
core/iwasm/libraries/lib-wasi-threads/lib_wasi_threads_wrapper.c
Outdated
Show resolved
Hide resolved
core/iwasm/libraries/lib-wasi-threads/lib_wasi_threads_wrapper.c
Outdated
Show resolved
Hide resolved
core/iwasm/libraries/lib-wasi-threads/lib_wasi_threads_wrapper.c
Outdated
Show resolved
Hide resolved
loganek
force-pushed
the
loganek/thread-implementation
branch
2 times, most recently
from
c0ba6ef
to
4705265
Compare
core/iwasm/libraries/lib-wasi-threads/lib_wasi_threads_wrapper.c
Outdated
Show resolved
Hide resolved
core/iwasm/libraries/lib-wasi-threads/lib_wasi_threads_wrapper.c
Outdated
Show resolved
Hide resolved
| goto thread_spawn_fail; | ||
| } | ||
|
|
||
| thread_start_arg->thread_id = thread_id = allocate_thread_id(); |
There was a problem hiding this comment.
Shall we check the integer overflow in allocate_thread_id? The system may run for long time and keep creating threads and eventually integer overflow occurs.
There was a problem hiding this comment.
We're planning to improve thread id allocation strategy in the upcoming PRs, I left it like that for simplicity.
There was a problem hiding this comment.
besides that, wasi-libc doesn't seem to use this id for anything important (yet)
loganek
force-pushed
the
loganek/thread-implementation
branch
from
4705265
to
99b932d
Compare
| @@ -444,6 +450,9 @@ wasm_native_init() | |||
| #endif | |||
|
|
|||
| #if WASM_ENABLE_LIB_WASI_THREADS != 0 | |||
| if (!lib_wasi_threads_init()) | |||
There was a problem hiding this comment.
Should lib_wasi_threads_destroy() in the handling of fail if lib_wasi_threads_init() is called, e.g. L468, L476? And we should modify L483 to goto fail?
There was a problem hiding this comment.
lib_wasi_threads_destroy() is called in wasm_native_destroy() so I think that's fine. I agree that L483 should goto fail instead of returning, will update that.
This is a simple implementation for now; going forward we most likely won't need to rely on thread manager as it doesn't provide much value in this scenario.
loganek
force-pushed
the
loganek/thread-implementation
branch
from
99b932d
to
e8842ee
Compare
| @@ -35,12 +38,12 @@ wasi_thread_start(int thread_id, int *start_arg) | |||
| int | |||
| main(int argc, char **argv) | |||
| { | |||
| shared_t data = { 0, 52 }; | |||
| __wasi_errno_t err; | |||
| shared_t data = { 0, 52, -1 }; | |||
There was a problem hiding this comment.
| shared_t data = { 0, 52, -1 }; | |
| shared_t data = { .th_ready = 0, .value = 52, .thread_id = -1 }; |
There was a problem hiding this comment.
iirc, we intentionally avoid designated initializers for some reasons. (old compilers?)
There was a problem hiding this comment.
yes, I think this is only available in >= c99
For now this implementation uses thread manager. Not sure whether thread manager is needed in that case. In the future there'll be likely another syscall added (for pthread_exit) and for that we might need some kind of thread management - with that in mind, we keep thread manager for now and will refactor this later if needed.
For now this implementation uses thread manager.
I wasn't sure whether thread manager is needed in that case. In the future there'll be likely another syscall added (for pthread_exit) and for that we might need some kind of thread management - with that in mind, I keep thread manager for now and we'll refactor this later if needed.