tests: check wasm compiler_builtins object architecture

[Vastargazing]

View all comments

See #132802

This adds a run-make test for the wasm sysroot regression fixed in #137457

The test checks that the .o members in the prebuilt
libcompiler_builtins rlib for wasm32-wasip1 are wasm objects rather than
host 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-wasip1 because that's the wasm target covered by the existing
tests/run-make CI setup, and the test asserts that it actually saw .o
members in the archive.

Closes: #132802

r? @tgross35

[rustbot]

tgross35 is currently at their maximum review capacity.
They may take a while to respond.

[tgross35]

Thank you for picking this up!

View changes since this review

[tgross35]

Reading into an object::File then checking .architecture() would probably be better here

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[Vastargazing]

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!

[tgross35]

@bors try jobs=test-various*

[tgross35]

@bors try jobs=test-various*

[tgross35]

Thanks for the updates, r=me with the passing try job

(Fyi you may have missed the above rustbot comment about how to update the PR status, this was still labeled waiting-on-author)

View changes since this review

[rust-bors]

💔 Test for 30c565b failed: CI. Failed job:

• try - test-various (web logs, enhanced plaintext logs)

[tgross35]

failed to parse member lib.rmeta-link in compiler_builtins rlib: Unknown file magic

Looks like another extension needs to be skipped

View changes since this review

[rustbot]

⚠️ Warning ⚠️

• This PR is based on an upstream commit that is older than 28 days.

  It's recommended to update your branch according to the rustc-dev-guide.

[Vastargazing]

Pushed a fix - lib.rmeta-link (wasm-specific link-time metadata member) was slipping past the .rmeta skip filter and tripping object::File::parse on the try build. Added it to the skip list.

@rustbot ready

[tgross35]

@bors try jobs=test-various

[rust-bors]

💔 Test for b9892dc failed: CI. Failed job:

• try - test-various (web logs, enhanced plaintext logs)

[tgross35]

Another small one

@rustbot author

[Vastargazing]

Reworked the skip logic - instead of listing extensions, now i just skip any member that doesn't parse as an object file (rmeta, bitcode .rcgu.o, etc). Real objects still get the arch check, so host objects would still fail.

@rustbot ready

[tgross35]

I don't think that's a better solution since if there's a problem reading the entry (e.g. if object is compiled without wasm support) then the test quietly does nothing. So I'd prefer sticking with the ignored list, or at least the original where everything ending with .o is checked.

But that is an interesting point - any idea why .rcgu.o isn't parsing? They should be valid object files.

(If you can't run locally then to get a more useful output message in CI, you could keep the current version but push the name to a vec if it fails to parse. Then after the loop, print the whole vec and fail if any entries don't match .rmeta and other expected parse failures. That way we get the whole list of what didn't parse at once).

[Vastargazing]

Found the issue

run-make-support builds the object crate without the wasm feature. So FileKind::Wasm isn't compiled in, it misses the \0asm magic, and throws "Unknown file magic". The objects are completely fine, our parser just can't see them.

Went back to the original .o + \0asm magic-byte check - dependency-free, and a host ELF/Mach-O can't start with \0asm anyway so it still catches the regression. Dropped the silent skip.

(If you'd rather keep the object::File + architecture() check, that just needs the wasm feature enabled in run-make-support - wasmparser is already in the tree. Happy to switch.)

@rustbot ready

[tgross35]

(If you'd rather keep the object::File + architecture() check, that just needs the wasm feature enabled in run-make-support - wasmparser is already in the tree. Happy to switch.)

Yes please - we'll probably want it at some other point anyway :) Sorry this has had so much back and forth