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-linker: implement -fno-entry and correctly pass --shared and --pie when given #17815

Merged
merged 5 commits into from
Nov 5, 2023

Conversation

Luukdegram
Copy link
Member

As discussed in #16949 (comment) this changes the invocation when building for the WebAssembly target. Users who previously used -dynamic in combination with build-lib will now be required to use build-exe in combination with -fno-entry. This is the same behavior as found in other compilers such as Clang and Rust. -fno-entry will tell the linker there's no entry point available and therefore the built binary can be used, for instance, in a browser where no entry point is expected.

This change also now correctly passes the --shared and --pie linker flags to wasm-ld when enabled in the frontend. e.g. when the user used the combination of build-lib -dynamic or build-exe -fPIE.

Closes #16937
Closes #16938
Closes #11045

@Luukdegram Luukdegram added the breaking Implementing this issue could cause existing code to no longer compile or have different behavior. label Nov 1, 2023
@andrewrk
Copy link
Member

andrewrk commented Nov 2, 2023

Yay, thank you!

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! I think we can unify the CLI and build system across architectures rather than making it wasm-specific.

src/main.zig Outdated Show resolved Hide resolved
lib/std/Build/Step/Compile.zig Outdated Show resolved Hide resolved
src/main.zig Outdated Show resolved Hide resolved
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I only had a little bit of time to look today, I will look again tomorrow, but here is the one comment that I noticed:

lib/std/Build/Step/Compile.zig Outdated Show resolved Hide resolved
This adds support for the `-fno-entry` and `-fentry` flags respectively, for
zig build-{exe/lib} and the build system. For `zig cc` we use the `--no-entry`
flag to be compatible with clang and existing tooling.

In `start.zig` we now make the main function optional when the target is
WebAssembly, as to allow for the build-exe command in combination with
`-fno-entry`.

When the execution model is set, and is set to 'reactor', we now verify
when an entry name is given it matches what is expected. When no entry
point is given, we set it to `_initialize` by default. This means the user
will also be met with an error when they use the reactor model, but did
not provide the correct function.
When linking a WebAssembly binary using wasm-ld, we will now correctly pass
--shared when building a dynamic library and --pie when `-fpic` is given.
This is a breaking change and users who currently build dynamic libraries
for WebAssembly but wish to have an executable without a main, should use
build-exe instead while supplying `-fno-entry`.

We also verify the user does not supply --export-memory when -dynamic is
enabled. By default we set this flag now to 'false' when `-dynamic` is used
to ensure we do not enable it as it's enabled by default for regular
WebAssembly binaries.
This updates all linker tests to include `no_entry` as well as changes
all tests to executable so they do not need to be updated later when
the in-house WebAssembly linker supports dynamic libraries.
@Luukdegram
Copy link
Member Author

Thanks for the feedback both Vexu and Andrew. I've pushed some changes which I think handles all cases now. Happy to hear if there are any other improvements to make.

@andrewrk andrewrk merged commit 702b809 into ziglang:master Nov 5, 2023
10 checks passed
@andrewrk
Copy link
Member

andrewrk commented Nov 5, 2023

Great work!

peterhellberg added a commit to peterhellberg/wasm4 that referenced this pull request Nov 7, 2023
scheibo added a commit to scheibo/zigpkg that referenced this pull request Nov 8, 2023
needing to explicitly list exports feels like a regression?
@Luukdegram Luukdegram deleted the wasm-no-entry branch November 14, 2023 09:42
@Luukdegram Luukdegram restored the wasm-no-entry branch November 14, 2023 09:42
@Luukdegram Luukdegram deleted the wasm-no-entry branch November 14, 2023 09:42
@Luukdegram Luukdegram restored the wasm-no-entry branch November 14, 2023 09:42
@Luukdegram Luukdegram deleted the wasm-no-entry branch November 14, 2023 09:42
Pyrolistical added a commit to Pyrolistical/zig-wasm-canvas that referenced this pull request Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Implementing this issue could cause existing code to no longer compile or have different behavior.
Projects
None yet
3 participants