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

WebAssembly: remove unconditional --allow-undefined flag #14102

Merged
merged 5 commits into from
Dec 29, 2022

Conversation

Luukdegram
Copy link
Sponsor Member

@Luukdegram Luukdegram commented Dec 28, 2022

Supersedes #14069, see Frank's excellent explanation of why we wouldn't want to set this flag unconditionally.
This PR also effectively reverts #10517 and we will no longer automatically append --export for each function that is being exported. This is now possible as stage2 is the default compiler, with no dependency on the old stage1 compiler. This means we can make use of the new visibility field on std.builtin.ExportOptions, where we set it to hidden when building compiler-rt for any WebAssembly target. This will allow the linker to still resolve the symbols to other modules but prevents it from exporting compiler-rt functions to the host environment, which also allows the functions to be garbage collected and we don't end up with bloated binaries.

Note: This is a breaking change and users who wish to automatically export their symbols can do so by using the -rdynamic flag, or set the --export flag themselves. As we will now no longer provide the --allow-undefined flag unconditionally, you'll now get a proper linking error. When you do want to import a symbol from the host environment, use the --import-symbols flag.

cc @jedisct1

jedisct1 and others added 3 commits December 25, 2022 22:32
In ziglang#1622, when targeting WebAsembly, the --allow-undefined flag
became unconditionally added to the linker.

This is not always desirable.

First, this is error prone. Code with references to unkown symbols
will link just fine, but then fail at run-time.

This behavior is inconsistent with all other targets.

For freestanding wasm applications, and applications that only use
WASI, undefined references are better reported at compile-time.

This behavior is also inconsistent with clang itself. Autoconf and
cmake scripts checking for function presence think that all tested
functions exist, but then resulting application cannot run.

For example, this is one of the reasons compilation of Ruby 3.2.0
to WASI fails with zig cc, while it works out of the box with clang.
But all applications checking for symbol existence before compilation
are affected.

This reverts the behavior to the one Zig had before ziglang#1622, and
introduces an `import_symbols` flag to ignore undefined symbols,
assuming that the webassembly runtime will define them.
No longer automatically append the `--export` flag for each exported
function unconditionally. This was essentially a hack to prevent
binary bloat caused by compiler-rt symbols being always included in
the final binary as they were exported and therefore not garbage-
collected. This is no longer needed as we now support the ability to
set the visibility of exports.
This essentially reverts 6d951af
When we're compiling compiler_rt for any WebAssembly target, we do
not want to expose all the compiler-rt functions to the host runtime.
By setting the visibility of all exports to `hidden`, we allow the
linker to resolve the symbols during linktime, while not expose the
functions to the host runtime. This also means the linker can
properly garbage collect any compiler-rt function that does not get
resolved. The symbol visibility for all target remains the same as
before: `default`.
@Luukdegram Luukdegram added the breaking Implementing this issue could cause existing code to no longer compile or have different behavior. label Dec 28, 2022
This hack was initially introduced as we would export all symbols
unconditionally, including non-function definitions. This would cause
an error from the Wasmtime runtime engine, which this flag would
suppress. As we now properly export symbols, this flag is no longer
needed and any user running into this error can manually include it.

This commit also adds the `--import-symbols` ability to build.zig
@kubkon
Copy link
Member

kubkon commented Dec 28, 2022

There are some Wasm linker test failures on aarch64-macos it seems:

2022-12-28T15:15:31.0012420Z ========= Expected to find: ==========================
2022-12-28T15:15:31.0012610Z Section import
2022-12-28T15:15:31.0012770Z ========= But parsed file does not contain it: =======
2022-12-28T15:15:31.0012910Z Section memory
2022-12-28T15:15:31.0013030Z size 3
2022-12-28T15:15:31.0013130Z entries 1
2022-12-28T15:15:31.0013240Z min 10
2022-12-28T15:15:31.0020970Z Section global
2022-12-28T15:15:31.0021350Z size 9
2022-12-28T15:15:31.0021470Z entries 1
2022-12-28T15:15:31.0021580Z type i32
2022-12-28T15:15:31.0021690Z mutable true
2022-12-28T15:15:31.0021810Z i32.const 100000
2022-12-28T15:15:31.0021940Z Section export
2022-12-28T15:15:31.0022050Z size 10
2022-12-28T15:15:31.0022170Z entries 1
2022-12-28T15:15:31.0022270Z name memory
2022-12-28T15:15:31.0022380Z kind memory
2022-12-28T15:15:31.0022490Z index 0
2022-12-28T15:15:31.0022600Z Section custom
2022-12-28T15:15:31.0022700Z size 1092
2022-12-28T15:15:31.0022810Z name .debug_info
2022-12-28T15:15:31.0022930Z Section custom
2022-12-28T15:15:31.0023040Z size 348
2022-12-28T15:15:31.0023150Z name .debug_pubtypes
2022-12-28T15:15:31.0023270Z Section custom
2022-12-28T15:15:31.0023380Z size 186
2022-12-28T15:15:31.0023490Z name .debug_abbrev
2022-12-28T15:15:31.0023610Z Section custom
2022-12-28T15:15:31.0023710Z size 298
2022-12-28T15:15:31.0023820Z name .debug_line
2022-12-28T15:15:31.0023930Z Section custom
2022-12-28T15:15:31.0024040Z size 1205
2022-12-28T15:15:31.0024150Z name .debug_str
2022-12-28T15:15:31.0024260Z Section custom
2022-12-28T15:15:31.0024370Z size 327
2022-12-28T15:15:31.0024480Z name .debug_pubnames
2022-12-28T15:15:31.0024710Z Section custom
2022-12-28T15:15:31.0024820Z size 25
2022-12-28T15:15:31.0024920Z name name
2022-12-28T15:15:31.0025030Z type global
2022-12-28T15:15:31.0025140Z size 18
2022-12-28T15:15:31.0025230Z names 1
2022-12-28T15:15:31.0025330Z index 0
2022-12-28T15:15:31.0025440Z name __stack_pointer
2022-12-28T15:15:31.0025560Z Section custom
2022-12-28T15:15:31.0025660Z size 26
2022-12-28T15:15:31.0025770Z name producers
2022-12-28T15:15:31.0025880Z fields 1
2022-12-28T15:15:31.0025990Z field_name language
2022-12-28T15:15:31.0026110Z values 1
2022-12-28T15:15:31.0026210Z value_name C99
2022-12-28T15:15:31.0026320Z version 
2022-12-28T15:15:31.0026390Z 
2022-12-28T15:15:31.0026440Z error: TestFailed
2022-12-28T15:15:31.0360710Z /Users/m1/actions-runner/_work/zig/zig/build/stage3-release/lib/zig/std/build/CheckObjectStep.zig:347:25: 0x102f3c2bb in make (build)
2022-12-28T15:15:31.0361050Z                         return error.TestFailed;
2022-12-28T15:15:31.0361190Z                         ^
2022-12-28T15:15:31.0376320Z /Users/m1/actions-runner/_work/zig/zig/build/stage3-release/lib/zig/std/build.zig:1452:9: 0x102ece19f in make (build)
2022-12-28T15:15:31.0379060Z         try self.makeFn(self);
2022-12-28T15:15:31.0379220Z         ^
2022-12-28T15:15:31.0385240Z /Users/m1/actions-runner/_work/zig/zig/build/stage3-release/lib/zig/std/build.zig:516:9: 0x102eb9727 in makeOneStep (build)
2022-12-28T15:15:31.0385970Z         try s.make();
2022-12-28T15:15:31.0386370Z         ^
2022-12-28T15:15:31.0392030Z /Users/m1/actions-runner/_work/zig/zig/build/stage3-release/lib/zig/std/build.zig:510:17: 0x102eb963b in makeOneStep (build)
2022-12-28T15:15:31.0393100Z                 return err;
2022-12-28T15:15:31.0393260Z                 ^
2022-12-28T15:15:31.0398930Z /Users/m1/actions-runner/_work/zig/zig/build/stage3-release/lib/zig/std/build.zig:471:13: 0x102eb920b in make (build)
2022-12-28T15:15:31.0400070Z             try self.makeOneStep(s);
2022-12-28T15:15:31.0400250Z             ^
2022-12-28T15:15:31.0407170Z /Users/m1/actions-runner/_work/zig/zig/build/stage3-release/lib/zig/build_runner.zig:221:21: 0x102ebc25b in main (build)
2022-12-28T15:15:31.0407580Z             else => return err,
2022-12-28T15:15:31.0407770Z                     ^
2022-12-28T15:15:31.0409920Z error: the following build command failed with exit code 1:
2022-12-28T15:15:31.0410590Z /Users/m1/actions-runner/_work/zig/zig/build/zig-local-cache/o/ea629968ee83f0cde3c5778b5e505eab/build /Users/m1/actions-runner/_work/zig/zig/build/stage3-release/bin/zig /Users/m1/actions-runner/_work/zig/zig/test/link/wasm/extern-mangle /Users/m1/actions-runner/_work/zig/zig/build/zig-local-cache /Users/m1/actions-runner/_work/zig/zig/build/zig-global-cache test
2022-12-28T15:15:31.0438980Z The following command exited with error code 1 (expected 0):
2022-12-28T15:15:31.0439480Z cd /Users/m1/actions-runner/_work/zig/zig && build/stage3-release/bin/zig build --build-file /Users/m1/actions-runner/_work/zig/zig/test/link/wasm/extern-mangle/build.zig test 
2022-12-28T15:15:31.0441980Z error: UnexpectedExitCode
2022-12-28T15:15:31.0967250Z /Users/m1/actions-runner/_work/zig/zig/build/../lib/std/build/RunStep.zig:277:17: 0x100a88533 in runCommand (build)
2022-12-28T15:15:31.0969030Z                 return error.UnexpectedExitCode;
2022-12-28T15:15:31.0969170Z                 ^
2022-12-28T15:15:31.1034340Z /Users/m1/actions-runner/_work/zig/zig/build/../lib/std/build/RunStep.zig:183:5: 0x100a56f33 in make (build)
2022-12-28T15:15:31.1034750Z     try runCommand(
2022-12-28T15:15:31.1034880Z     ^
2022-12-28T15:15:31.1057200Z /Users/m1/actions-runner/_work/zig/zig/build/../lib/std/build.zig:1452:9: 0x1009f0253 in make (build)
2022-12-28T15:15:31.1063510Z         try self.makeFn(self);
2022-12-28T15:15:31.1063900Z         ^
2022-12-28T15:15:31.1069990Z /Users/m1/actions-runner/_work/zig/zig/build/../lib/std/build.zig:516:9: 0x1009c9727 in makeOneStep (build)
2022-12-28T15:15:31.1070980Z         try s.make();
2022-12-28T15:15:31.1071100Z         ^
2022-12-28T15:15:31.1077500Z /Users/m1/actions-runner/_work/zig/zig/build/../lib/std/build.zig:510:17: 0x1009c963b in makeOneStep (build)
2022-12-28T15:15:31.1078340Z                 return err;
2022-12-28T15:15:31.1078460Z                 ^
2022-12-28T15:15:31.1084950Z /Users/m1/actions-runner/_work/zig/zig/build/../lib/std/build.zig:510:17: 0x1009c963b in makeOneStep (build)
2022-12-28T15:15:31.1085710Z                 return err;
2022-12-28T15:15:31.1085850Z                 ^
2022-12-28T15:15:31.1092200Z /Users/m1/actions-runner/_work/zig/zig/build/../lib/std/build.zig:510:17: 0x1009c963b in makeOneStep (build)
2022-12-28T15:15:31.1092950Z                 return err;
2022-12-28T15:15:31.1093060Z                 ^
2022-12-28T15:15:31.1099350Z /Users/m1/actions-runner/_work/zig/zig/build/../lib/std/build.zig:471:13: 0x1009c920b in make (build)
2022-12-28T15:15:31.1100100Z             try self.makeOneStep(s);
2022-12-28T15:15:31.1100230Z             ^
2022-12-28T15:15:31.1107730Z /Users/m1/actions-runner/_work/zig/zig/build/../lib/build_runner.zig:221:21: 0x1009cc25b in main (build)
2022-12-28T15:15:31.1109820Z             else => return err,
2022-12-28T15:15:31.1109970Z                     ^

Force importing symbols to show the correct functions are being
imported from the host environment.
@Luukdegram
Copy link
Sponsor Member Author

My bad, I forgot to update the linker tests.

@mitchellh
Copy link
Sponsor Contributor

As a wasm noob learning more about this and playing with Zig+wasm (freestanding), does this mean I have to set those build settings for my export functions now to be available? Likewise for extern. I'm not doing anything fancy just curious who this impacts 😄

@Luukdegram
Copy link
Sponsor Member Author

As a wasm noob learning more about this and playing with Zig+wasm (freestanding), does this mean I have to set those build settings for my export functions now to be available? Likewise for extern. I'm not doing anything fancy just curious who this impacts 😄

This would indeed affect your current codebase. If you wanted those functions to be available to the host environment, you'd either have to add step.rdynamic=true; to make them all available or step.export_synmbol_names = <list of names> (for specific exports) to your build.zig. If omitted, those symbols can only resolve between other WebAssembly object files. This means Zig follows the same strategy as described here: https://lld.llvm.org/WebAssembly.html#exports apart from the ability to add the wasm-export-name attribute as Zig doesn't have such a feature.

This does impose the topic of developer experience. Perhaps we should add this attribute for any Zig export where the visibility is default. This would cause the declaration to be exported, and users would still be able to manually disable this for declarations using @export with visibility set to hidden, similar to how we set import-name and module-name implicitly for extern's. This would also not have any effect on zig cc as the previous implementation did. cc @andrewrk would really love your opinion on this.

@mitchellh
Copy link
Sponsor Contributor

Got it. Yeah I’m fine either way just wanted to check what’s up.

As a user, it feels weird that a lib I make with “export” doen’t become available to other thinks “loading it” by default. i.e If I am making an object file for ELF “export” makes it generally available to anything that links with that O file. If I create an object file for wasm it feels like “export” should make it generally available for “loading” it into the browser environment (its not “linking” I guess, though…).

Like I said, no real dog in the fight but my 2c. I’m fine adapting to either solution.

@Luukdegram
Copy link
Sponsor Member Author

Got it. Yeah I’m fine either way just wanted to check what’s up.

As a user, it feels weird that a lib I make with “export” doesn’t become available to other thinks “loading it” by default. i.e If I am making an object file for ELF “export” makes it generally available to anything that links with that O file. If I create an object file for wasm it feels like “export” should make it generally available for “loading” it into the browser environment (its not “linking” I guess, though…).

Like I said, no real dog in the fight but my 2c. I’m fine adapting to either solution.

This is actually the same behavior as with ELF. When you export it, you make it available to anything that links with the object file, just like with ELF. The difference being is that WebAssembly has a secondary 'link' stage which happens during module instantiation, i.e. when you load it into the browser or any other runtime which is more similar to running an ELF binary on an operating system. I do appreciate your opinion though, I think the majority of the people writing Zig code targetting WebAssembly will expect exactly what you mentioned when they write export ..., hence my suggestion to add the attribute to exported non-hidden declarations. That way the linker remains consistent with other linkers, and Zig users will get a better user experience.

@andrewrk andrewrk merged commit 40ba4d4 into ziglang:master Dec 29, 2022
@andrewrk
Copy link
Member

Great work!

@trgwii
Copy link

trgwii commented Feb 4, 2023

I think for sure the common / expected case should be the default when exporting, and the uncommon / unexpected case is the one you need @export for

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
Development

Successfully merging this pull request may close these issues.

6 participants