-
Notifications
You must be signed in to change notification settings - Fork 36
Add Emscripten compat include paths to interpreter initialization #409
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
base: main
Are you sure you want to change the base?
Conversation
|
What's Happening here ? So on native builds, clang automatically discovers <xlocale.h> through the system SDK or libc include paths. How emcc does it This shows 3 paths As can be seen this is addressed through This PR adds the same (check right side of the img below as now we're able to search through
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #409 +/- ##
==========================================
- Coverage 82.40% 82.05% -0.35%
==========================================
Files 21 21
Lines 858 858
Branches 89 89
==========================================
- Hits 707 704 -3
- Misses 151 154 +3
... and 1 file with indirect coverage changes
🚀 New features to boost your workflow:
|
|
cc @mcbarton The same change would be needed in cppinterop to run the emscripten tests (cause Otherwise some tests would just fail through I just ran the cppinterop tests (after building both cppinterop and llvm against latest emsdk 4-x) and this is where I caught the above. |
|
clang-tidy review says "All clean, LGTM! 👍" |
Thanks the tip @anutosh491 . I will look into this. Did you run the llvm Emscripten tests to see if a change is needed to get them to still pass too? |
Not yet ! But we probably won't need any change there as this is the only real change I could spot. The llvm emscripten tests don't really process any "#Include" 's as such which means no include path searching change ! |
test/test_interpreter.cpp
Outdated
|
|
||
| #if defined(XEUS_CPP_EMSCRIPTEN_WASM_BUILD) | ||
| TEST_CASE("headers found in sysroot/include/compat") | ||
| { |
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.
Added a simple test confirming we can access headers from the location of interest. Technically only need for the wasm build cause its straightforward for the native kernel (so probably not needed)
But the cc1 command as shown in our tests now cover all 3 paths that emcc covers as I've pointed above
clang version 20.1.8 (https://github.com/emscripten-forge/recipes 26347a2820b7a44b0329d3b9bd524c56ff3cdce7)
Failed to detect the resource-dir
exec: : Function not implemented
Target: wasm32-unknown-emscripten
Thread model: posix
InstalledDir:
"" -cc1 -triple wasm32-unknown-emscripten -emit-obj -disable-free -clear-ast-before-backend -disable-llvm-verifier -discard-value-names -main-file-name "<<< inputs >>>" -mrelocation-model static -mframe-pointer=none -ffp-contract=on -fno-rounding-math -mconstructor-aliases -target-cpu generic -debugger-tuning=gdb -fdebug-compilation-dir=/ -v -fcoverage-compilation-dir=/ -resource-dir /home/runner/micromamba/envs/xeus-cpp-wasm-host -internal-isystem /include/wasm32-emscripten/c++/v1 -internal-isystem /include/c++/v1 -internal-isystem /home/runner/micromamba/envs/xeus-cpp-wasm-host/include -internal-isystem /include/wasm32-emscripten -internal-isystem /include -std=c++14 -fdeprecated-macro -ferror-limit 19 -fvisibility=default -fgnuc-version=4.2.1 -fskip-odr-check-in-gmf -fcxx-exceptions -fexceptions -iwithsysroot/include/compat -fincremental-extensions -o "<<< inputs >>>.o" -x c++ "<<< inputs >>>"
clang -cc1 version 20.1.8 based upon LLVM 20.1.8 default target wasm32-unknown-emscripten
ignoring nonexistent directory "/include/wasm32-emscripten/c++/v1"
ignoring nonexistent directory "/home/runner/micromamba/envs/xeus-cpp-wasm-host/include"
ignoring nonexistent directory "/include/wasm32-emscripten"
#include "..." search starts here:
#include <...> search starts here:
/include/compat
/include/c++/v1
/include
End of search list.
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
src/xinterpreter.cpp
Outdated
|
|
||
| #ifdef __EMSCRIPTEN__ | ||
| ClangArgs.push_back("-Xclang"); | ||
| ClangArgs.push_back("-iwithsysroot/include/compat"); |
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.
Doesn't that need to go to the emscripten toolchain file in clang ideally?
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.
Hmm not sure (possibly no ?)
Because the compat/ headers are added by emcc via -Xclang -iwithsysroot/include/compat only at the tool level at the very end https://github.com/emscripten-core/emscripten/blob/3622274db64d8ce706690197d1c24cd02715d937/tools/compile.py#L144
The upstream Clang WebAssembly toolchain ( Webassembly.cpp looks target-generic atleast for the path based utilties (for eg AddClangSystemIncludeArgs, addLibCxxIncludePaths) and won’t encode Emscripten SDK layout quirks like include/compat just yet
But I do see emscripten specific exception handling (for eg) so maybe we can have emscripten specific stuff here ?!
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.
Just thinking a bit more I think no
- The emscripten specifc exception handling (-enable-emscripten-*) flags I mentioned are present so the compiler backend can reproduce Emscripten’s codegen semantics.
- The file does not have a reason to know that “Emscripten’s sysroot layout has an /include/compat folder.” So Sysroot layout quirks like /include/compat belong in the Emscripten wrapper (emcc) or, for direct Clang users, must be added manually.
Something like
# Path to the Emscripten sysroot from the SDK
SYSROOT=$HOME/work/emsdk/upstream/emscripten/cache/sysroot
/path/to/your/clang \
--target=wasm32-unknown-emscripten \
-isysroot $SYSROOT \
-Xclang -iwithsysroot/include/compat \
-c a.cpp -o a.o
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.
If that include path is something that is required to compile emscripten code this should be part of the clang emscripten toolchain file.
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.
Well yeah that's what I was pointing out that technically in the toolchains folder we don't see an "emscripten" based file but more of a "webassembly" target-generic file
https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/WebAssembly.cpp
So not sure this can handle emscripten specifics but let me ask around. Sam Clegg from emscripten should be able to help me out here.
Add supporting tests
|
clang-tidy review says "All clean, LGTM! 👍" |

Description
Discovered this while moving to emscripten 4-x for xeus-cpp.
Problem : The following works for our native kernel but not our wasm kernel
Type of change
Please tick all options which are relevant.