tests: check wasm compiler_builtins object architecture#156230
tests: check wasm compiler_builtins object architecture#156230Vastargazing wants to merge 1 commit into
Conversation
rustbot
added
A-run-make
|
|
This comment has been minimized.
This comment has been minimized.
Vastargazing
force-pushed
the
tests/wasm-compiler-builtins-object-arch
branch
from
211904e to
b25f8a1
Compare
There was a problem hiding this comment.
Please add a doc comment explaining what this does, and linking back to the issue
| if !name.ends_with(".o") { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This is only skipping rmeta right? Flip this around to name.ends_with(".rmeta") so we find if there's something weird.
| C objects were expected on this configuration; \ | ||
| test may be running without compiler-builtins-c enabled" |
There was a problem hiding this comment.
I think this comment isn't exactly accurate, there should be object files in the archive regardless of whether or not the C implementations are used.
| assert!( | ||
| obj_data.starts_with(b"\0asm"), | ||
| "object file `{name}` in compiler_builtins rlib is not a wasm binary \ | ||
| (first bytes: {:02x?}); C compiler-rt fallbacks were built with the wrong \ | ||
| toolchain — see rust-lang/rust#132802", | ||
| &obj_data[..4.min(obj_data.len())] | ||
| ); |
There was a problem hiding this comment.
Reading into an object::File then checking .architecture() would probably be better here
| @@ -0,0 +1,43 @@ | |||
| //@ only-wasm32-wasip1 | |||
There was a problem hiding this comment.
Can this be changed to only-wasm? It might not be in CI but anyone running locally for the other targets should get it covered
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
Vastargazing
force-pushed
the
tests/wasm-compiler-builtins-object-arch
branch
from
b25f8a1 to
962838f
Compare
|
Thanks for the feedback! I've just force-pushed the fixes to the branch. Addresssed everything we discussed: added the module-level docs with links, and broadened the directive to only-wasm to cover other targets. I also flipped the filter to skip only .rmeta so unexpected files will actually trigger a failure now. For the wasm check, i dropped the raw magic bytes and went with object::File::parse + matching on Wasm32/Wasm64. Also cleaned up the misleading assert message about compiler-builtins-c and dropped the trailing eprintln!. Let me know what you think! |
This comment has been minimized.
This comment has been minimized.
Add a run-make test that checks the `.o` members in the prebuilt `libcompiler_builtins` archive for `wasm32-wasip1` are wasm objects. This guards against building wasm compiler-rt fallbacks with the host C toolchain.
Vastargazing
force-pushed
the
tests/wasm-compiler-builtins-object-arch
branch
from
962838f to
50ad258
Compare
4 participants
See #132802
This adds a run-make test for the wasm sysroot regression fixed in #137457
The test checks that the
.omembers in the prebuiltlibcompiler_builtinsrlib forwasm32-wasip1are wasm objects rather thanhost ELF objects. Before that fix, bootstrap could use the host C compiler for
compiler-rt fallbacks on wasm targets and end up embedding host objects into
the wasm sysroot.
I used
wasm32-wasip1because that's the wasm target covered by the existingtests/run-makeCI setup, and the test asserts that it actually saw.omembers in the archive.
Closes: #132802
r? @tgross35