Make Wasmtime compatible with Stacked Borrows in MIRI #6338
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The fact that Wasmtime executes correctly under Tree Borrows but not Stacked Borrows is a bit suspect and given what I've since learned about the aliasing models I wanted to give it a stab to get things working with Stacked Borrows. It turns out that this wasn't all that difficult, but required two underlying changes: * First the implementation of `Instance::vmctx` is now specially crafted in an intentional way to preserve the provenance of the returned pointer. This way all `&Instance` pointers will return a `VMContext` pointer with the same provenance and acquiring the pointer won't accidentally invalidate all prior pointers. * Second the conversion from `VMContext` to `Instance` has been updated to work with provenance and such. Previously the conversion looked like `&mut VMContext -> &mut Instance`, but I think this didn't play well with MIRI because `&mut VMContext` has no provenance over any data since it's zero-sized. Instead now the conversion is from `*mut VMContext` to `&mut Instance` where we know that `*mut VMContext` has provenance over the entire instance allocation. This shuffled a fair bit around to handle the new closure-based API to prevent escaping pointers, but otherwise no major change other than the structure and the types in play. This commit additionally picks up a dependency on the `sptr` crate which is a crate for prototyping strict-provenance APIs in Rust. This is I believe intended to be upstreamed into Rust one day (it's in the standard library as a Nightly-only API right now) but in the meantime this is a stable alternative.
This commit adds a new wrapper type `SendSyncPtr<T>` which automatically
impls the `Send` and `Sync` traits based on the `T` type contained.
Otherwise it works similarly to `NonNull<T>`. This helps clean up a
number of manual annotations of `unsafe impl {Send,Sync} for ...`
throughout the runtime.
In an effort to enable MIRI's "strict provenance" mode this commit removes the integer-to-pointer casts in the runtime `Table` implementation for Wasmtime. Most of the bits were already there to track all this, so this commit plumbed around the various pointer types and with the help of the `sptr` crate preserves the provenance of all related pointers.
The `MemoryImageSlot` type stored a `base: usize` field mostly because I was too lazy to have a `Send`/`Sync` type as a pointer, so this commit updates it to use `SendSyncPtr<u8>` and then plumbs the pointer-ness throughout the implementation. This removes all integer-to-pointer casts and has pointers stores as actual pointers when they're at rest.
This commit changes the "raw" representation of `Func` and `ExternRef` to a `*mut c_void` instead of the previous `usize`. This is done to satisfy MIRI's requirements with strict provenance, properly marking the intermediate value as a pointer rather than round-tripping through integers.
Additionally enable the strict-provenance features to force warnings emitted today to become errors.
github-actions
bot
added
wasmtime:api
Subscribe to Label Actioncc @fitzgen, @peterhuene
This issue or pull request has been labeled: "wasmtime:api", "wasmtime:c-api", "wasmtime:ref-types"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
sunfishcode
reviewed
May 3, 2023
|
I'm not going to be able to review this in a timely fashion but it looks like there's excellent feedback already. @sunfishcode, can you take on approving this PR, or hand it off to someone else? |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Here's to hoping that's spurious... |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Maybe the third time's the charm. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
alexcrichton
added a commit
to alexcrichton/wasmtime
that referenced
this pull request
May 8, 2023
In bytecodealliance#6338 that PR is bouncing on CI due to Windows running out of disk space. Locally a `./ci/run-tests.sh` compile produces a ~13G build directory. Testing on CI it looks like Windows builders have ~13G of free space on them for GitHub Actions. I think something in that PR has tipped us just over the edge of requiring too much space, although I'm not sure exactly what. To address the issue this commit disables DWARF debug information entirely on all builders on CI. Not only should this save a sliver of compile time but this reduces a local build directory to ~4.7G, over a 50% reduction. That should keep us fitting in CI for more time to come hopefully.
alexcrichton
added a commit
that referenced
this pull request
May 9, 2023
In #6338 that PR is bouncing on CI due to Windows running out of disk space. Locally a `./ci/run-tests.sh` compile produces a ~13G build directory. Testing on CI it looks like Windows builders have ~13G of free space on them for GitHub Actions. I think something in that PR has tipped us just over the edge of requiring too much space, although I'm not sure exactly what. To address the issue this commit disables DWARF debug information entirely on all builders on CI. Not only should this save a sliver of compile time but this reduces a local build directory to ~4.7G, over a 50% reduction. That should keep us fitting in CI for more time to come hopefully.
kpreisser
added a commit
to kpreisser/wasmtime-dotnet
that referenced
this pull request
May 10, 2023
… type of raw funcrefs/externrefs from usize to void*. Therefore we change `nuint` to `IntPtr` for these types, which although technically equivalent, better matches the native API.
peterhuene
pushed a commit
to bytecodealliance/wasmtime-dotnet
that referenced
this pull request
May 11, 2023
… type of raw funcrefs/externrefs from usize to void*. (#247) Therefore we change `nuint` to `IntPtr` for these types, which although technically equivalent, better matches the native API.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a series of commits hot on the tail of #6332. Previously Wasmtime was only valid under the Tree Borrows model in MIRI which is a much newer but less restrictive model for Rust's borrowing. After some discussion on Zulip though I concluded that there's no reason for us to rely purely on Tree Borrows and it's probably best to work under Stacked Borrows as well. This commit does that.
This commit also goes a bit further and gets everything working under strict provenance as well to fix any warnings coming out of MIRI. This means that we should be in a pretty strong position with respect to the verification running on CI, where the main remaining hole is lack of coverage.