Update wiggle's `BorrowChecker` implementation #8277

alexcrichton · 2024-04-01T15:03:49Z

This commit is a refactoring and modernization of wiggle's BorrowChecker implementation. This type is quite old and predates everything related to the component model for example. This type additionally predates the implementation of WASI threads for Wasmtime as well. In general, this type is old and has not been updated in a long time.

Originally a BorrowChecker was intended to be a somewhat cheap method of enabling the host to have active safe shared and mutable borrows to guest memory. Over time though this hasn't really panned out. The WASI threads proposal, for example, doesn't allow safe shared or mutable borrows at all. Instead everything must be modeled as a copy in or copy out of data. This means that all of wasmtime-wasi and wasi-common have largely already been rewritten in such a way to minimize borrows into linear memory.

Nowadays the only types that represent safe borrows are the GuestSlice type and its equivalents (e.g. GuestSliceMut, GuestStr, etc). These are minimally used throughout wasi-common and wasmtime-wasi and when they are used they're typically isolated to a small region of memory.

This is all coupled with the facst that BorrowChecker never ended up being optimized. It's a Mutex<HashMap<..>> effectively and a pretty expensive one at that. The Mutex is required because &BorrowChecker must both allow mutations and be Sync. The HashMap is used to implement precise byte-level region checking to fulfill the original design requirements of what wiggle was envisioned to be.

Given all that, this commit guts BorrowChecker's implementation and functionality. The type is now effectively a glorified RefCell for the entire span of linear memory. Regions are no longer considered when borrows are made and instead a shared borrow is considered as borrowing the entirety of shared memory. This means that it's not possible to simultaneously have a safe shared and mutable borrow, even if they're disjoint, at the same time.

The goal of this commit is to address performance issues seen in #7973 which I've seen locally as well. The heavyweight implementation of BorrowChecker isn't really buying us much nowadays, especially with much development having since moved on to the component model. The hope is that this much coarser way of implementing borrow checking, which should be much more easily optimizable, is sufficient for the needs of WASI and not a whole lot else.

This commit is a refactoring and modernization of wiggle's `BorrowChecker` implementation. This type is quite old and predates everything related to the component model for example. This type additionally predates the implementation of WASI threads for Wasmtime as well. In general, this type is old and has not been updated in a long time. Originally a `BorrowChecker` was intended to be a somewhat cheap method of enabling the host to have active safe shared and mutable borrows to guest memory. Over time though this hasn't really panned out. The WASI threads proposal, for example, doesn't allow safe shared or mutable borrows at all. Instead everything must be modeled as a copy in or copy out of data. This means that all of `wasmtime-wasi` and `wasi-common` have largely already been rewritten in such a way to minimize borrows into linear memory. Nowadays the only types that represent safe borrows are the `GuestSlice` type and its equivalents (e.g. `GuestSliceMut`, `GuestStr`, etc). These are minimally used throughout `wasi-common` and `wasmtime-wasi` and when they are used they're typically isolated to a small region of memory. This is all coupled with the facst that `BorrowChecker` never ended up being optimized. It's a `Mutex<HashMap<..>>` effectively and a pretty expensive one at that. The `Mutex` is required because `&BorrowChecker` must both allow mutations and be `Sync`. The `HashMap` is used to implement precise byte-level region checking to fulfill the original design requirements of what `wiggle` was envisioned to be. Given all that, this commit guts `BorrowChecker`'s implementation and functionality. The type is now effectively a glorified `RefCell` for the entire span of linear memory. Regions are no longer considered when borrows are made and instead a shared borrow is considered as borrowing the entirety of shared memory. This means that it's not possible to simultaneously have a safe shared and mutable borrow, even if they're disjoint, at the same time. The goal of this commit is to address performance issues seen in bytecodealliance#7973 which I've seen locally as well. The heavyweight implementation of `BorrowChecker` isn't really buying us much nowadays, especially with much development having since moved on to the component model. The hope is that this much coarser way of implementing borrow checking, which should be much more easily optimizable, is sufficient for the needs of WASI and not a whole lot else.

fitzgen · 2024-04-01T16:09:48Z

crates/wiggle/src/borrow.rs

-
-    fn is_shared_borrowed(&self, r: Region) -> bool {
-        self.shared_borrows.values().any(|b| b.overlaps(r))
+    pub fn mut_unborrow(&self, _: BorrowHandle) {


Should there be different handle types for mut vs shared borrows?

In theory yeah, but I'm not too keen to do too much refactoring here because it's all basically a dated library on life support now. These are all ignored anyway so I should probably refactor to remove them entirely, but I'll leave that as a future TODO if necessary

Turns out we haven't been running wasi-common tests for some time in CI and they've started failing. Force enable the test at all times and then fix the test failures. The test failures here were introduced in bytecodealliance#8277 and weren't caught due to the test not running and the fix was to relax the implementation of `fd_pread` to avoid taking multiple mutable borrows.

* Fix running wasi-common tests on CI Turns out we haven't been running wasi-common tests for some time in CI and they've started failing. Force enable the test at all times and then fix the test failures. The test failures here were introduced in #8277 and weren't caught due to the test not running and the fix was to relax the implementation of `fd_pread` to avoid taking multiple mutable borrows. * Fix CI

* Fix running wasi-common tests on CI Turns out we haven't been running wasi-common tests for some time in CI and they've started failing. Force enable the test at all times and then fix the test failures. The test failures here were introduced in bytecodealliance#8277 and weren't caught due to the test not running and the fix was to relax the implementation of `fd_pread` to avoid taking multiple mutable borrows. * Fix CI

* c-api: Better differentiate between `wasm.h` and `wasmtime.h` APIs (#8344) This renames some types and adds some type aliases to help us better distinguish between `wasm.h` APIs and `wasmtime.h` APIs, primarily for `Store`-related types. In general, `WasmFoo` is related to `wasm.h` and `WasmtimeFoo` is related to `wasmtime.h`. * `StoreRef` -> `WasmStoreRef` * Introduce the `WasmStore[Data]` and `WasmStoreContext[Mut]` aliases * `StoreData` -> `WasmtimeStoreData` * `CStoreContext[Mut]` -> `WasmtimeStoreContext[Mut]` * Introduce the `Wasmtime{Store,Caller}` aliases * `wasmtime-c-api`: Improve non-support of GC references in `wasm.h` APIs (#8345) * c-api: Better differentiate between `wasm.h` and `wasmtime.h` APIs This renames some types and adds some type aliases to help us better distinguish between `wasm.h` APIs and `wasmtime.h` APIs, primarily for `Store`-related types. In general, `WasmFoo` is related to `wasm.h` and `WasmtimeFoo` is related to `wasmtime.h`. * `StoreRef` -> `WasmStoreRef` * Introduce the `WasmStore[Data]` and `WasmStoreContext[Mut]` aliases * `StoreData` -> `WasmtimeStoreData` * `CStoreContext[Mut]` -> `WasmtimeStoreContext[Mut]` * Introduce the `Wasmtime{Store,Caller}` aliases * c-api: Improve non-support of GC references in `wasm.h` APIs A couple small tweaks: error message improvements, exhaustive matching, etc... * Fix running wasi-common tests on CI (#8353) * Fix running wasi-common tests on CI Turns out we haven't been running wasi-common tests for some time in CI and they've started failing. Force enable the test at all times and then fix the test failures. The test failures here were introduced in #8277 and weren't caught due to the test not running and the fix was to relax the implementation of `fd_pread` to avoid taking multiple mutable borrows. * Fix CI * Update release notes for 20.0.0 (#8358) A busy release! * Enable the gc feature by default in the c-api (#8356) Match the Wasmtime crate in this respect * wasmtime-c-api: Add support for GC references in `wasmtime.h` APIs (#8346) Restores support for `externref` in `wasmtime_val_t`, methods for manipulating them and getting their wrapped host data, and examples/tests for these things. Additionally adds support for `anyref` in `wasmtime_val_t`, clone/delete methods similar to those for `externref`, and a few `i31ref`-specific methods. Also adds C and Rust example / test for working with `anyref`. * Fix calculation of gc refs in functions (#8355) * Fix calculation of gc refs in functions In addition to excluding i31 also exclude funcrefs. * Review comments * Remove `wasi_config_preopen_socket` from C header (#8364) This was removed in #8066 * Tidy up some headers related to shared memory (#8366) * Tidy up some headers related to shared memory * Don't declare an anonymous `struct wasmtime_sharedmemory`, instead `#include` the actual definition. * Fix an issue where a header in `sharedmemory.h` referred to a type in `extern.h` which wasn't `#include`'d. This function, `wasmtime_sharedmemory_into_extern`, additionally isn't necessary as it's no different than manually constructing it. Fix this by removing this function. * Run clang-format * c-api: Fix alignment of `wasmtime_val_*` (#8363) * c-api: Fix alignment of `wasmtime_val_*` This commit fixes an issue where `wasmtime_val_raw_t` had an incorrect alignment. In Rust `ValRaw` contains a `u128` which has an alignment of 16 but in C the representation had a smaller alignment meaning that the alignment of the two structures was different. This was seen to cause alignment faults when structure were passed from C++ to Rust, for example. This commit changes the Rust representation of `ValRaw`'s `v128` field to do the same as C which is to use `[u8; 16]`. This avoids the need to raise the alignment in C which appears to be nontrivial. Cranelift is appropriately adjusted to understand that loads/stores from `ValRaw` are no longer aligned. Technically this only applies to the `v128` field but it's not too bad to apply it to the other fields as well. * Try alternate syntax for alignof * Use `--locked` on all `cargo install` in CI, also remove non-locked example (#8369) * Use `--locked` on all `cargo install` in CI Prevents any updates to rustc or crates from accidentally causing issues by ensuring that the same set of deps is used over time. * Remove rust/WASI markdown parser example The documentation referring to this example was removed in #6994 and that forgot to remove this as well. This example is building without a lock file which is causing issues in #8368. --------- Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>

…8277 Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>

* We can only have one mutable borrow at a time after #8277 Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com> * Add a test like pread/pwrite, but for read/write --------- Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>

…ealliance#8415) * We can only have one mutable borrow at a time after bytecodealliance#8277 Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com> * Add a test like pread/pwrite, but for read/write --------- Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>

…8431) * We can only have one mutable borrow at a time after #8277 * Add a test like pread/pwrite, but for read/write --------- Co-authored-by: Trevor Elliott <telliott@fastly.com> Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>

Update the wasmtime dependency to wasmtime-20, and also remove the dependency on wasi-common in favor of using the preview1-on-preview2 implementation provided by wasmtime-wasi. There was also a change in wiggle's borrow checker for the 20.0.0 release that affected our host call implementations directly: borrows are no longer to a region, but the whole guest memory. This caused a bit of churn in the wiggle_abi tree, and I'm not completely certain that all issues have been resolved, as the test suite can only test so many paths through the host calls. For reference, here's the wiggle refactoring PR in wasmtime: bytecodealliance/wasmtime#8277.

This commit is a refactoring of the `wiggle` crate which powers the `*.witx`-based bindings generation of Wasmtime for wasip1 support. Originally `wiggle` had a full-blown runtime borrow checker which verified that borrows were disjoint when appropriate. In bytecodealliance#8277 this was removed in favor of a more coarse "either all shared or all mutable" guarantee. It turns out that this exactly matches what the Rust type system guarantees at compile time as well. This commit removes all runtime borrow checking in favor of compile-time borrow checking instead. This means that there is no longer the possibility of a runtime error arising from borrowing errors. Current bindings in Wasmtime needed no restructuring to work with this new API. The source of the refactors here are all in the `wiggle` crate. Changes include: * The `GuestPtr` type lost its type parameter. Additionally it only contains a `u32` pointer now instead. * The `GuestMemory` trait is replaced with a simple `enum` of possibilities. * Helper methods on `GuestPtr` are all moved to `GuestMemory`. * A number of abstractions were simplified now that borrow checking is no longer necessary. * Generated trait methods now all take `&mut GuestMemory<'_>` as an argument. These changes were then propagated to the `wasmtime-wasi` and `wasi-common` crates in their preview0 and preview1 implementations of WASI. All changes are just general refactors, no functional change is intended here.

* Remove the borrow checking from `wiggle` entirely This commit is a refactoring of the `wiggle` crate which powers the `*.witx`-based bindings generation of Wasmtime for wasip1 support. Originally `wiggle` had a full-blown runtime borrow checker which verified that borrows were disjoint when appropriate. In #8277 this was removed in favor of a more coarse "either all shared or all mutable" guarantee. It turns out that this exactly matches what the Rust type system guarantees at compile time as well. This commit removes all runtime borrow checking in favor of compile-time borrow checking instead. This means that there is no longer the possibility of a runtime error arising from borrowing errors. Current bindings in Wasmtime needed no restructuring to work with this new API. The source of the refactors here are all in the `wiggle` crate. Changes include: * The `GuestPtr` type lost its type parameter. Additionally it only contains a `u32` pointer now instead. * The `GuestMemory` trait is replaced with a simple `enum` of possibilities. * Helper methods on `GuestPtr` are all moved to `GuestMemory`. * A number of abstractions were simplified now that borrow checking is no longer necessary. * Generated trait methods now all take `&mut GuestMemory<'_>` as an argument. These changes were then propagated to the `wasmtime-wasi` and `wasi-common` crates in their preview0 and preview1 implementations of WASI. All changes are just general refactors, no functional change is intended here. * Review comments * Fix publishing of wiggle-macro crate * Fix wiggle docs

alexcrichton requested a review from a team as a code owner April 1, 2024 15:03

alexcrichton requested review from fitzgen and removed request for a team April 1, 2024 15:03

alexcrichton mentioned this pull request Apr 1, 2024

Poor performance of wasmtime file I/O maybe because tokio #7973

Closed

github-actions bot added the wasi Issues pertaining to WASI label Apr 1, 2024

fitzgen approved these changes Apr 1, 2024

View reviewed changes

alexcrichton added this pull request to the merge queue Apr 1, 2024

Merged via the queue into bytecodealliance:main with commit bc4d4cb Apr 1, 2024
19 checks passed

alexcrichton deleted the less-borrow-checking branch April 1, 2024 18:09

liutao-liu mentioned this pull request Apr 5, 2024

optimize perfermance of fd_read/fd_write #8303

Merged

alexcrichton mentioned this pull request Apr 12, 2024

Fix running wasi-common tests on CI #8353

Merged

elliottt added a commit to elliottt/wasmtime that referenced this pull request Apr 19, 2024

We can only have one mutable borrow at a time after bytecodealliance#…

60df49b

…8277 Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>

elliottt mentioned this pull request Apr 19, 2024

Restrict the preview1 implementation of fd_read to one iovec #8415

Merged

elliottt added a commit to elliottt/wasmtime that referenced this pull request Apr 19, 2024

We can only have one mutable borrow at a time after bytecodealliance#…

edf7191

…8277 Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>

dundargoc mentioned this pull request May 7, 2024

[20.0.0]: Backport fixes from main branch #8569

Merged

elliottt mentioned this pull request May 7, 2024

Upgrade to wasmtime-20, and switch to wasmtime-wasi fastly/Viceroy#366

Merged

alexcrichton mentioned this pull request May 29, 2024

Remove the borrow checking from wiggle entirely #8702

Merged

jameysharp mentioned this pull request Jul 11, 2024

Rust autofix incorrectly leaves "let" keyword in place semgrep/semgrep#10376

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update wiggle's `BorrowChecker` implementation #8277

Update wiggle's `BorrowChecker` implementation #8277

alexcrichton commented Apr 1, 2024

fitzgen Apr 1, 2024

alexcrichton Apr 1, 2024

Update wiggle's BorrowChecker implementation #8277

Update wiggle's BorrowChecker implementation #8277

Conversation

alexcrichton commented Apr 1, 2024

fitzgen Apr 1, 2024

Choose a reason for hiding this comment

alexcrichton Apr 1, 2024

Choose a reason for hiding this comment

Update wiggle's `BorrowChecker` implementation #8277

Update wiggle's `BorrowChecker` implementation #8277