Refactor stub pthread library and enable unconditionally #602
Conversation
|
I went through quite a lot of effort to ensure that the symbols and macros exported by the |
There was a problem hiding this comment.
Thanks for this! Just a minor comment about assert messages, but otherwise looks all good. I agree it's ok to defer sem_* and other APIs to future PRs if necessary.
I think it'd be nice to have tests here somehow, but #518 was landed without tests which is where all the "meat" comes from so I don't think it's reasonable to block this on tests.
Out of curiosity though, have you tested this in conjunction with the wasi-sdk PR to see if it suits your needs? (e.g. is able to build a working LLVM)
libc-top-half/musl/include/pthread.h
Outdated
| #define pthread_create(...) ({ _Static_assert(0, "This function is not available without -pthread"); 0;}) | ||
| #define pthread_detach(...) ({ _Static_assert(0, "This function is not available without -pthread"); 0;}) | ||
| #define pthread_join(...) ({ _Static_assert(0, "This function is not available without -pthread"); 0;}) |
There was a problem hiding this comment.
Since this error only shows up with -D_WASI_STRICT_PTHREAD and that's non-default, while the message here is technically correct could it instead refer to -D_WASI_STRICT_PTHREAD as that's probably the "true" fix in the sense that -pthread is a heavy hammer to fix this as it might enable threads when that wasn't intended.
There was a problem hiding this comment.
How about
This function is not available on a single-threaded target; switch to a multi-threaded target with -pthread or enable a stub implementation by undefining _WASI_STRICT_PTHREAD
Right. I think #518 was pretty raw and I was somewhat surprised that it landed given that it's not quite up to the standard of the rest of wasi-sdk code, and I've taken some effort to clean it up and bring it up to par. Hopefully that's assurance enough. I acknowledge that having tests would be good but it was a bit of a rush to get this in before a release and I'd really rather not continue to be in a situation where a wasi-sdk release is blocked on me. Of course if the PR turns out to be buggy that would create problems for me too, since I have several codebases where I'd like to apply it right away.
Yes! I submitted draft PRs at first and only un-drafted them after I became confident that everything works as intended via testing. I haven't built LLVM (I think I'll do that next), rather I've built Slang and Yosys-Slang and then linked all that into Yosys. Yosys-Slang uses some mutexes during compilation, which tested that, and the whole stack obviously links and runs fine, which tests that there aren't any unresolved references. |
This commit changes the layout of the stub pthread library to match
that of libc-top-half: splits it into (mostly) one file per function,
and ensures the same API is exposed from both libraries, adding stub
functions as necessary.
This commit also removes `-lwasi-emulated-pthread` as well as
`-D_WASI_EMULATED_PTHREAD` and enables this functionality
unconditionally, seeing as this is necessary for building libc++ with
threading enabled.
It adds a flag `-D_WASI_STRICT_PTHREAD` that causes thread creation
functions (`pthread_create`, `pthread_detach`, `pthread_join`,
`pthread_tryjoin_np`, `pthread_timedjoin_np`) to be defined as a macro
causing a compile error, which can be used to locate the parts of
a codebase requiring changes for the single thread model.
Specifically, after this commit, the two targets `wasm32-wasip1` and
`wasm32-wasip1-threads` differ as follows (cleaned up for clarity):
--- expected/wasm32-wasip1/defined-symbols.txt
+++ expected/wasm32-wasip1-threads/defined-symbols.txt
+flockfile
+ftrylockfile
+funlockfile
+sem_destroy
+sem_getvalue
+sem_init
+sem_post
+sem_timedwait
+sem_trywait
+sem_wait
+wasi_thread_start
--- expected/wasm32-wasip1/predefined-macros.txt
+++ expected/wasm32-wasip1-threads/predefined-macros.txt
+#define _REENTRANT 1
+#define SEM_NSEMS_MAX 256
+#define SEM_VALUE_MAX 0x7fffffff
+#define __wasm_atomics__ 1
+#define __wasm_bulk_memory__ 1
|
Should be ready to merge now. |
This commit enables thread support in libc++ for all thread models, enabling C++ applications that use threading APIs like `<atomic>` but do not spawn threads (e.g. Clang) to be built with minimal changes. Fixes #546. Depends on WebAssembly/wasi-libc#602.
Thanks to recent wasi-libc changes, wasi-libc and libc++ in wasi-sdk now provides `std::atomic`, `std::mutex`, and other thread-related APIs even for wasm32-wasip1 target. * WebAssembly/wasi-libc#602 * WebAssembly/wasi-sdk#548
This PR changes the layout of the stub pthread library to match that of libc-top-half: splits it into (mostly) one file per function, and ensures the same API is exposed from both libraries, adding stub functions as necessary.
This PR also removes
-lwasi-emulated-pthreadas well as-D_WASI_EMULATED_PTHREADand enables this functionality unconditionally, seeing as this is necessary for building libc++ with threading enabled.It adds a flag
-D_WASI_STRICT_PTHREADthat causes thread creation functions (pthread_create,pthread_detach,pthread_join) to be defined as a macro causing a compile error, which can be used to locate the parts of a codebase requiring changes for the single thread model.Specifically, after this commit, the two targets
wasm32-wasip1andwasm32-wasip1-threadsdiffer as follows (cleaned up for clarity):