[wit-component] add shared-everything linking support #1133

dicej · 2023-07-19T20:28:17Z

Per https://github.com/WebAssembly/component-model/blob/main/design/mvp/examples/SharedEverythingDynamicLinking.md and https://hackmd.io/IlY4lICRRNy9wQbNLdb2Wg.

This adds a new component link subcommand, which resembles component new but accepts an arbitrary number of "dynamic library" modules (as defined by https://github.com/WebAssembly/tool-conventions/blob/main/DynamicLinking.md), linking them together to create a component whose type is the union of the component types found in the input modules (via the usual custom section representation). This process can also be invoked programmatically via a new Linker API.

Linker analyzes and topologically sorts the input modules, then synthesizes two additional modules:

main: hosts the component's single memory and function table and exports any functions needed to break dependency cycles discovered in the input modules. Those functions use call.indirect to invoke the real functions, references to which are placed in the table by the init module.
init: populates the function table as described above, initializes global variables per the dynamic linking tool convention, and calls any static constructors and/or link-time fixup functions

Finally, Linker generates a directed graph of modules and their instantiations and passes it to ComponentEncoder to actually produce the component.

The implementation relies on a few modifications to the existing code in ComponentEncoder and friends. Specifically, I've expanded the meaning of "adapter" to include dynamic libraries as well as traditional adapters. The former are distiguished from the latter by the presence of a new LibraryInfo struct. This approach minimized the number of code changes in the interest of keeping this PR comprehensible, but we will probably want to refactor ComponentEncoder in the future to make the distinction between traditional adapters and dynamic libraries clearer, possibly generalizing how we analyze and instantiate modules of various flavors.

I've created a demo which ties everything together, for reference: https://github.com/dicej/component-linking-demo. It includes wasi-sdk and wasi-libc patches which have not yet been upstreamed; I'm planning to work on that next.

dicej · 2023-07-19T20:29:29Z

I need to add some more tests before this will be ready to merge, but I'm opening a draft PR in case anyone wants to comment in the meantime.

dicej · 2023-07-19T20:51:03Z

TIL (most of) the wasm-tools tests need to be able to run in a Wasm guest, and I broke that by using wasmtime in the linking test. I guess I have two options to address that: either rewrite the linking test to not use wasmtime (i.e. turn it into one of those "matches expected output WAT" tests) or exclude the linking test from the set of tests run inside Wasm. Anyone have a preference?

alexcrichton · 2023-07-19T20:58:29Z

Oh apologies that's a very recent addition, and it's ok to skip the wasmtime parts of the test and have something like [target.'cfg(not(target_family = "wasm"))'.dev-dependencies] for the wasmtime dep

This is something I threw together which I think might be helpful in testing and working on bytecodealliance#1133. In theory this should also make it easier to inspect this custom section in preexisting binaries. I've invented a text format of this out of nowhere as I don't believe there's a specification or anything else to follow. I've opted to use the `@dylink.0` annotation format to emphasize that it's a custom section, in the same manner that `@producers` uses that today. Note that this additionally copies some of the `wasmparser`-related parsing code from bytecodealliance#1133. Co-authored-by: Joel Dice <joel.dice@fermyon.com>

crates/wit-component/tests/linking.rs

* Add text format and printing support for `dylink.0` This is something I threw together which I think might be helpful in testing and working on #1133. In theory this should also make it easier to inspect this custom section in preexisting binaries. I've invented a text format of this out of nowhere as I don't believe there's a specification or anything else to follow. I've opted to use the `@dylink.0` annotation format to emphasize that it's a custom section, in the same manner that `@producers` uses that today. Note that this additionally copies some of the `wasmparser`-related parsing code from #1133. Co-authored-by: Joel Dice <joel.dice@fermyon.com> * Add more bindings for symbol parsing --------- Co-authored-by: Joel Dice <joel.dice@fermyon.com>

Per https://github.com/WebAssembly/component-model/blob/main/design/mvp/examples/SharedEverythingDynamicLinking.md and https://hackmd.io/IlY4lICRRNy9wQbNLdb2Wg. This adds a new `component link` subcommand, which resembles `component new` but accepts an arbitrary number of "dynamic library" modules (as defined by https://github.com/WebAssembly/tool-conventions/blob/main/DynamicLinking.md), linking them together to create a component whose type is the union of the component types found in the input modules (via the usual custom section representation). This process can also be invoked programmatically via a new `Linker` API. `Linker` analyzes and topologically sorts the input modules, then sythesizes two additional modules: - `main`: hosts the component's single memory and function table and exports any functions needed to break dependency cycles discovered in the input modules. Those functions use `call.indirect` to invoke the real functions, references to which are placed in the table by the `init` module. - `init`: populates the function table as described above, initializes global variables per the dynamic linking tool convention, and calls any static constructors and/or link-time fixup functions Finally, `Linker` generates a directed graph of modules and their instantiations and passes it to `ComponentEncoder` to actually produce the component. The implementation relies on a few modifications to the existing code in `ComponentEncoder` and friends. Specifically, I've expanded the meaning of "adapter" to include dynamic libraries as well as traditional adapters. The former are distiguished from the latter by the presence of a new `LibraryInfo` struct. This approach minimized the number of code changes in the interest of keeping this PR comprehensible, but we will probably want to refactor `ComponentEncoder` in the future to make the distinction between traditional adapters and dynamic libraries clearer, possibly generalizing how we analyze and instantiate modules of various flavors. I've created a demo which ties everything together, for reference: https://github.com/dicej/component-linking-demo. It includes `wasi-sdk` and `wasi-libc` patches which have not yet been upstreamed; I'm planning to work on that next. Signed-off-by: Joel Dice <joel.dice@fermyon.com> skip wasmtime part of linking test under Wasm Signed-off-by: Joel Dice <joel.dice@fermyon.com> add linking support to wit-component components.rs test runner Signed-off-by: Joel Dice <joel.dice@fermyon.com> set `Metadata::has_component_exports` only if specific world has exports Signed-off-by: Joel Dice <joel.dice@fermyon.com> add `link-dl-openable` component test and make output deterministic Signed-off-by: Joel Dice <joel.dice@fermyon.com> add linking tests Signed-off-by: Joel Dice <joel.dice@fermyon.com> support weak imports when linking We currently generate trapping stubs for such imports if no suitable export is found. Signed-off-by: Joel Dice <joel.dice@fermyon.com>

crates/wit-component/tests/linking.rs

alexcrichton

Looks reasonable to me, thanks for putting in the work on this! This definitely turned out pretty gnarly, but that's to be expected in this problem domain.

I don't intimiately understand the details of dynamic linking but everything appeared pretty reasonable to me here so I'm leaning on you mostly to ensure it's handled alright (and sounds like with the size of the demos you're working with that'll be ok). The main two meta-level comments I have which are also interspersed in some review are:

It may be worth watching out for hash maps and iteration order leading to nondeterministic output. In my opinion maps are fine when it's known they won't affect the output order (e.g. only used for lookup), but maps that are iterated over may want to be Index{Map,Set} instead.
In general I try to assert that insertion into a map or set succeeds without replacing something to catch possible bugs where I got something wrong by accident. It may be worth adding asserts to all the various places to handle that?

crates/wit-component/src/linking.rs

crates/wit-component/tests/components/error-link-duplicate-symbols/error.txt

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

Using `match` gives us exhaustiveness checking in case new variants are added later. Signed-off-by: Joel Dice <joel.dice@fermyon.com>

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

Turns out bit casting from u32 to i32 is what we want rather than `i32::try_from`. Signed-off-by: Joel Dice <joel.dice@fermyon.com>

* in `make_env_module`, track function count explicitly rather than use `types.len()` * strength-reduce `2.pow(x)` to `1 << x` * add various inline comments to elucidate tricky code * use named constants for type offsets in `make_init_module` * use `None` instead of `Some(0)` for table index to maximize compatibility * make error messages more consise by avoiding use of `Debug` Signed-off-by: Joel Dice <joel.dice@fermyon.com>

We now use `Index{Map|Set}` instead of `Hash{Map|Set}` where iteration order may affect the output to ensure deterministic behavior. Also, we assert that we are not overwriting old data unexpectedly when inserting or collecting into a set or map. Signed-off-by: Joel Dice <joel.dice@fermyon.com>

dicej · 2023-08-09T16:27:27Z

Thanks, as always, for the thorough review, @alexcrichton. I believe I've addressed all your feedback so far.

alexcrichton · 2023-08-09T17:48:34Z

👍 Thanks again!

We can switch to upstream `wasm-tools` now that bytecodealliance/wasm-tools#1133 has been merged. Note that various libwasi-emulated-*.so libraries have been split out of libc.so, and this may change again depending on where the discussion in WebAssembly/wasi-libc#429 lands. Signed-off-by: Joel Dice <joel.dice@fermyon.com>

Release latest changes such as: * `wasm-tools compose` now supports `-t` (bytecodealliance#1148) * The `abi` module is removed from the `wit-parser` crate (bytecodealliance#1149, bytecodealliance#1159) * Fix validation of lowered functions in the component model (bytecodealliance#1150) * Fix a panic decoding WIT from a component (bytecodealliance#1157) * Add text format and printing support for `dylink.0` (bytecodealliance#1135) * Support shared-everything linking in `wasm-tools component new` (bytecodealliance#1133) * Printing a WIT document now prints comments as well (bytecodealliance#1167) * Disallow `(borrow $t)` in component function results (bytecodealliance#1162)

Release latest changes such as: * `wasm-tools compose` now supports `-t` (#1148) * The `abi` module is removed from the `wit-parser` crate (#1149, #1159) * Fix validation of lowered functions in the component model (#1150) * Fix a panic decoding WIT from a component (#1157) * Add text format and printing support for `dylink.0` (#1135) * Support shared-everything linking in `wasm-tools component new` (#1133) * Printing a WIT document now prints comments as well (#1167) * Disallow `(borrow $t)` in component function results (#1162)

alexcrichton mentioned this pull request Jul 21, 2023

Add text format and printing support for dylink.0 #1135

Merged

alexcrichton reviewed Jul 21, 2023

View reviewed changes

crates/wit-component/tests/linking.rs Show resolved Hide resolved

dicej force-pushed the linking branch from 97ba8a8 to c634137 Compare July 26, 2023 21:55

dicej force-pushed the linking branch from c634137 to 6d9bf0d Compare August 8, 2023 16:02

dicej force-pushed the linking branch from 6d9bf0d to 40e827f Compare August 8, 2023 16:17

dicej marked this pull request as ready for review August 8, 2023 16:17

dicej requested a review from alexcrichton August 8, 2023 16:17

alexcrichton reviewed Aug 8, 2023

View reviewed changes

crates/wit-component/tests/linking.rs Outdated Show resolved Hide resolved

alexcrichton approved these changes Aug 8, 2023

View reviewed changes

dicej added 6 commits August 8, 2023 15:48

use wasmparser's dylink.0 support in linking test

0fb0e7b

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

replace if with match and tweak formatting

cfed9e7

Using `match` gives us exhaustiveness checking in case new variants are added later. Signed-off-by: Joel Dice <joel.dice@fermyon.com>

make stack size configurable when linking

492baed

Signed-off-by: Joel Dice <joel.dice@fermyon.com>

introduce const_u32 helper function

7f48e44

Turns out bit casting from u32 to i32 is what we want rather than `i32::try_from`. Signed-off-by: Joel Dice <joel.dice@fermyon.com>

alexcrichton merged commit 4678a61 into bytecodealliance:main Aug 9, 2023

alexcrichton mentioned this pull request Aug 18, 2023

Bump crate versions #1173

Merged

dicej mentioned this pull request Sep 1, 2023

wasm-compose: deduplicate identical modules #1190

Open

lum1n0us mentioned this pull request Sep 15, 2023

[Documentation] static core linking WebAssembly/component-model#239

Closed

[wit-component] add shared-everything linking support #1133

[wit-component] add shared-everything linking support #1133

Uh oh!

Conversation

dicej commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dicej commented Jul 19, 2023

Uh oh!

dicej commented Jul 19, 2023

Uh oh!

alexcrichton commented Jul 19, 2023

Uh oh!

Uh oh!

Uh oh!

alexcrichton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dicej commented Aug 9, 2023

Uh oh!

alexcrichton commented Aug 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dicej commented Jul 19, 2023 •

edited

Loading