Skip to content
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

wasm: do not default to pic and re-enable tests #16949

Closed
wants to merge 35 commits into from

Conversation

Luukdegram
Copy link
Member

For WebAssembly we want DynamicNoPIC for relocation mode. PIC is experimental, but will also force a shared library for WebAssembly which is not what the user expects as that will require runtime relocations to be performed.
This also re-enables the linker tests that regressed with llvm17

Closes #16937
Closes #16938

#16938 mentions:

Note that this test actually does not use LLVM or LLD:

The linker test verifies being able to link with an archive file. Since the Zig toolchain doesn't have an ar implementation yet, the test depends on compiler_rt, which is generated by LLVM. Due to LLVM17 actually supporting outputting PIC for WebAssembly, it generates a bunch of sections and relocations the in-house linker hasn't implemented yet. (Unfortunately, the stack trace is 💩 due to a double close to the file handle, which will be fixed in a separate PR).

andrewrk and others added 30 commits August 20, 2023 15:53
New OSs:
* UEFI
* LiteOS

New ABI:
* OpenHOS

Also update the LLD driver API wrappers.
release/17.x branch, commit 8f4dd44097c9ae25dd203d5ac87f3b48f854bba8
release/17.x branch, commit 8f4dd44097c9ae25dd203d5ac87f3b48f854bba8
release/17.x branch, commit 8f4dd44097c9ae25dd203d5ac87f3b48f854bba8
 * some manual fixes to generated CPU features code. in the future it
   would be nice to make the script do those automatically. I suspect
   the sm_90a thing is a bug in LLVM.
 * add liteos to various target OS switches. I know nothing about this
   OS; someone will need to work specifically on support for this OS
   when the time comes to support it properly in zig.
 * while waiting for the compiler, I went ahead and made more
   conservative choices about when to use `inline` in std/Target.zig
 * LLVMConstSelect is removed (see
   https://discourse.llvm.org/t/rfc-remove-most-constant-expressions/63179)
 * a couple functions use uint64_t instead of int now which means we
   no longer need `@intCast`.

release/17.x branch, commit 8f4dd44097c9ae25dd203d5ac87f3b48f854bba8
release/17.x branch, commit 8f4dd44097c9ae25dd203d5ac87f3b48f854bba8
release/17.x branch, commit 8f4dd44097c9ae25dd203d5ac87f3b48f854bba8
release/17.x branch, commit 8f4dd44097c9ae25dd203d5ac87f3b48f854bba8

This adds the flag `-D_LIBCPP_PSTL_CPU_BACKEND_SERIAL`. A future
enhancement could possibly pass something different if there is a
compelling parallel implementation. That libdispatch one might be worth
looking into.
release/17.x branch, commit 8f4dd44097c9ae25dd203d5ac87f3b48f854bba8
release/17.x branch, commit 8f4dd44097c9ae25dd203d5ac87f3b48f854bba8
Importantly, fixes incorrectly annotated types in `__aeabi_?2h`.
andrewrk and others added 5 commits August 23, 2023 12:46
These started failing after the LLVM 17 upgrade.

See tracking issues ziglang#16937 and ziglang#16938
See ziglang#16797 - it was fixed in the most recent LLVM 17 release candidate.
Updates llvm libraries to 17.0.0rc3 which contain bug fixes that
affected the zig test suite.
For WebAssembly we want DynamicNoPIC for relocation mode. PIC is experimental,
but will also force a shared library for WebAssembly which is not what the user
expects as that will require runtime relocations to be performed. This also means
that we generate an invalid object file unless Zig (or the user) appends `-shared`
to wasm-ld.

This also re-enables the linker tests that regressed with llvm17
@andrewrk
Copy link
Member

Thanks!

@andrewrk
Copy link
Member

PIC is experimental, but will also force a shared library for WebAssembly which is not what the user expects as that will require runtime relocations to be performed

if link_mode is "dynamic" then that's exactly what is being requested. Instead, link_mode should be set to static earlier on. I'll try to figure out why link_mode is being incorrectly set to Dynamic.

@andrewrk
Copy link
Member

hmmm for compiler_rt specifically, we're passing static:

.link_mode = .Static,

@andrewrk
Copy link
Member

OK the reason that link_mode is being set to Dynamic is because the tests are literally asking for it:

    const lib = b.addSharedLibrary(.{

force a shared library

that's exactly what's being asked for in the build.zig script.

@Luukdegram
Copy link
Member Author

Luukdegram commented Aug 25, 2023

OK the reason that link_mode is being set to Dynamic is because the tests are literally asking for it:

    const lib = b.addSharedLibrary(.{

force a shared library

that's exactly what's being asked for in the build.zig script.

You're completely right, but we're currently in this odd position where users really just want a singular Wasm binary that's static and doesn't have an entry point but doesn't want an archive file (so don't build a static library). Perhaps we should instead use build-exe/addExecutable and use -no-entry, just like other compilers such as Clang and Rust do. For context, in #8857 the original behavior was changed by Jakub and users have to use -dynamic to build a loadable WebAssembly module. However, WebAssemly does in fact have a concept of dynamic libraries now, which is not what the test wants to build. There's currently (in llvm17 branch) no way to express a single static Wasm binary that can be loaded into a runtime/browser but does not have an entry point.

edit: In the near future we will want to support real dynamic libraries in the in-house linker also. For this, we'll need a way for the user to specify they want a dynamic library. It would make sense to have them use -dynamic to express that. So perhaps -fno-entry is the way to go.

@andrewrk
Copy link
Member

andrewrk commented Aug 25, 2023

I agree with your reasoning - build-exe with -fno-entry is the way to go. Also I think it's generally good to join in with Clang and Rust unless there is some really good reason to be different.

Apologies for the force push - I think that messed things up a bit here. Also, I'm perfectly willing to move forward with these tests disabled, including merging this branch into master when LLVM 17 is released. No need to rush on the fix. Let's solve this proper!

@Luukdegram
Copy link
Member Author

No worries about the force push - Let's close this PR for now and I'll work on the proper fixes in a separate branch so the llvm17 work can get merged into master when ready. We can re-enable the linker tests in the branch with the proper fix in master later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LLVM 17 regression: link.wasm.archive.test LLVM 17 regression: link.wasm.export-data.test
3 participants