Fix make THREAD_MODEL=posix
#311
Conversation
|
Should I abandon #303 now in favor of this? (seems like I should) |
Probably, yes... this contains your commit but rebased on the latest |
abrown
force-pushed
the
fix_pthread_build_rebased
branch
from
e1ef872 to
af251b9
Compare
In WebAssembly#311, it became apparent that duplicate symbol definitions were becoming unwieldy. This change merges all duplicates using `uniq`.
In #311, it became apparent that duplicate symbol definitions were becoming unwieldy. This change merges all duplicates using `uniq`.
This uses the `_REENTRANT` definition to indicate when the `lock` should be available.
In talking to @sunfishcode about `aio.h`, this functionality is not yet a primary concern (it was already disabled in the default, single-threaded mode). Additionally, this change adds expectation lines for the new symbols/includes added and removed by `pthread.h`. This change was reached by running: ```console $ git diff --no-index expected/wasm32-wasi sysroot/share/wasm32-wasi > patch.diff # replace "sysroot/share" with "expected" in `patch.diff` $ git apply patch.diff --reject # manually fix any rejections ```
The `-ftls-model` configuration can be removed once https://reviews.llvm.org/D130053 makes its way into an upstream release.
The symbol is still undefined, though.
@sbc100 wanted to retain the whitespace trailing after certain predefined macro lines. This change restores that whitespace from upstream and re-generates the POSIX version using the following command: ```console $ git diff --no-index expected/wasm32-wasi/posix/predefined-macros.txt sysroot/share/wasm32-wasi/predefined-macros.txt | sed 's/sysroot\/share\/wasm32-wasi/expected\/wasm32-wasi\/posix/' | git apply ```
There are other options here (e.g., always define the `pthread_*` symbols with stubs) but until we discuss that this is an intermediate working step.
abrown
force-pushed
the
fix_pthread_build_rebased
branch
from
b07263d to
0878de7
Compare
| @@ -0,0 +1,171 @@ | |||
| #include <__errno.h> | |||
There was a problem hiding this comment.
The naming of these new files seems a little odd.. since posix doesn't just mean threads. I would hope we could use pthreads here (or something like that), but that might mean changing the way we trigger the build. Something like make WITH_PTHREADS=1?
There was a problem hiding this comment.
Can we do a follow-up to that later? It's quite convenient and clear to use $(THREAD_MODEL) throughout the Makefile...
There was a problem hiding this comment.
I'd rather not rename these expectations file just to rename them again later.
Can you find a way to at least leave the existing files in place?
How about something like this:
#if THREAD_MODULE == posix
EXPECTATION_SUBDIR = '/pthreads'
#else
EXPECTATION_SUBDIR = ''
#endif
I know that isn't Makefile syntax but you get the idea. This way the old files stay in place, and the new files have more meaningful names.
There was a problem hiding this comment.
Ok, I tried this and the issue is that we are using diff recursively so a structure like the following doesn't really work:
- expected
--- wasm-32-wasi
----- defined-symbols.txt
----- ...
----- posix (or pthreads)
------- defined-symbols.txt
------- ...
The posix/pthreads directory gets in the way of the diff.
There was a problem hiding this comment.
It seems like we'll need two separate directory trees to diff with. Here are some options:
a. append /pthreads or /no-pthreads to the expected directory path; this at least means that the naming is a bit more accurate
b. append something, e.g., pthreads, to the target, so we would diff against either expected/wasm32-wasi or expected/wasm32-wasi-pthreads
|
Can you take care to clean up the the commit message when squashing/merging next time? That comment message was a little out of control. |
* Fixes for the THREAD_MODEL=posix build * Fix expected symbols from previous commit * Enable `lock` in `random.c` when threads are enabled This uses the `_REENTRANT` definition to indicate when the `lock` should be available. * Disable `aio.h` when compiling for threads In talking to @sunfishcode about `aio.h`, this functionality is not yet a primary concern (it was already disabled in the default, single-threaded mode). Additionally, this change adds expectation lines for the new symbols/includes added and removed by `pthread.h`. This change was reached by running: ```console $ git diff --no-index expected/wasm32-wasi sysroot/share/wasm32-wasi > patch.diff $ git apply patch.diff --reject ``` * Specify the TLS model until LLVM 15 is released The `-ftls-model` configuration can be removed once https://reviews.llvm.org/D130053 makes its way into an upstream release. * Rename `__wasi_libc_pthread_self` to `__wasilibc_pthread_self` The symbol is still undefined, though. * Add different sets of expected output based on THREAD_MODEL * Re-add trailing whitespace to `predefined-macros.txt` @sbc100 wanted to retain the whitespace trailing after certain predefined macro lines. This change restores that whitespace from upstream and re-generates the POSIX version using the following command: ```console $ git diff --no-index expected/wasm32-wasi/posix/predefined-macros.txt sysroot/share/wasm32-wasi/predefined-macros.txt | sed 's/sysroot\/share\/wasm32-wasi/expected\/wasm32-wasi\/posix/' | git apply ``` * Protect `preopens.c` against concurrent access * Only build thread-capable wasi-libc on latest version of Clang * Use `thrd_sleep` from MUSL instead of aliasing `nanosleep` * Define `pthread_setcancelstate` in `THREAD_MODEL=posix` builds There are other options here (e.g., always define the `pthread_*` symbols with stubs) but until we discuss that this is an intermediate working step. * Define a Wasm global to store `pthread_self` * Remove `g_needs_dynamic_alloc` global * Document the state of pthread support * review: de-duplicate symbols based on WebAssembly#314 * review: only define `__wasilibc_cwd_{un}lock` when needed * review: add #ifdefs to `__pthread_setcancelstate` * review: add additional #ifdefs to `pthread_self.c` * review: put lock definition behind #ifdef _REENTRANT * review: remove pthread_setcancelstate.c * review: re-fix indentation * review: alias __clock_nanosleep in bottom half * review: remove extra line Co-authored-by: Sam Clegg <[email protected]>
This adds my changes on top of #303 in order to have
make THREAD_MODEL=posixbuild successfully.Fixes: #209