-
Notifications
You must be signed in to change notification settings - Fork 223
Refactor how compiler-rt is managed in CI and locally #591
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
Refactor how compiler-rt is managed in CI and locally #591
Conversation
* If `BUILTINS_LIB` is defined then use that. * If `BUILTINS_LIB` is not defined then download a copy from wasi-sdk. Once a `BUILTINS_LIB` is acquired the `*.so` rules are then refactored as well: * Both now pass `EXTRA_CFLAGS` at the end to allow overriding things. * Both now pass `LDFLAGS`. * The `-resource-dir` argument is dropped for `libc.so` as it's not used.
aee232d to
da7f96b
Compare
|
@abrown @sunfishcode ok this is I believe the result of the changes we discussed today |
|
WebAssembly/wasi-sdk#540 is the wasi-sdk side of things and I think that's passing CI, just one being retried currently after a spurious failure |
abrown
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me! I think the one missing piece to this is the README.md and test/README.md: they currently explain, in many words, the need for libclang_rt.builtins-wasm32.a. We probably want to keep some of that explanation in case users run into this again ("if you run into errors like undefined symbol: __muloti4..."), but we may want to explain the download-by-default change.
| # For Clang linking to work correctly, we need to place Clang's runtime | ||
| # library for `wasm32-wasi` in the right location (i.e., the `mkdir` and | ||
| # `cp` below). | ||
| run: | | ||
| cd test | ||
| mkdir resource-dir | ||
| export WASI_DIR=./resource-dir/lib/wasi/ | ||
| export WASIP1_DIR=./resource-dir/lib/wasip1/ | ||
| export WASIP2_DIR=./resource-dir/lib/wasip2/ | ||
| mkdir -p $WASI_DIR $WASIP1_DIR $WASIP2_DIR | ||
| cp build/download/libclang_rt.builtins-wasm32.a $WASI_DIR | ||
| cp build/download/libclang_rt.builtins-wasm32.a $WASIP1_DIR | ||
| cp build/download/libclang_rt.builtins-wasm32.a $WASIP2_DIR | ||
| export LDFLAGS="-resource-dir $(pwd)/resource-dir" | ||
| make test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
sunfishcode
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good; this just needs some documentation for the BUILTINS_LIB variable.
|
@abrown I've expanded the docs some more but I suspect you either thought there were more docs than there already were or I missed something, so can you double-check? |
sunfishcode
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the updated docs look good, and they cover the download-by-default behavior and how to override it.
Building the PIC version of libc.a is currently broken with LLVM 19+, and it's due to how this repository handles the compiler-rt library. This PR refactors the handling to automatically download an appropriate copy and handle it internally or otherwise use a
BUILTINS_LIBfrom the environment. This means that it's now always available forlibc.sowhich is needed to fix the build issue with LLVM 19+