Stream: git-wasmtime

Topic: wasmtime / PR #6338 Make Wasmtime compatible with Stacked...

Wasmtime GitHub notifications bot (May 03 2023 at 22:40):

alexcrichton opened PR #6338 from alexcrichton:miri-stacked-borrows to bytecodealliance:main:

This is a series of commits hot on the tail of https://github.com/bytecodealliance/wasmtime/pull/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.

Wasmtime GitHub notifications bot (May 03 2023 at 22:40):

alexcrichton requested wasmtime-core-reviewers for a review on PR #6338.

Wasmtime GitHub notifications bot (May 03 2023 at 22:40):

alexcrichton requested jameysharp for a review on PR #6338.

Wasmtime GitHub notifications bot (May 03 2023 at 22:40):

alexcrichton requested wasmtime-default-reviewers for a review on PR #6338.

Wasmtime GitHub notifications bot (May 03 2023 at 23:25):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (May 03 2023 at 23:25):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (May 03 2023 at 23:25):

sunfishcode created PR review comment:

This could be simplified slightly to .sub(...).

Wasmtime GitHub notifications bot (May 03 2023 at 23:25):

sunfishcode created PR review comment:

Could you add a comment about why this is a casted 1 instead of null_mut()?

Wasmtime GitHub notifications bot (May 03 2023 at 23:25):

sunfishcode created PR review comment:

Since this takes an immutable &self, should it return a *const VMContext, with a separate vmctx_mut(&mut self) or so if a *mut VMContext is needed?

Wasmtime GitHub notifications bot (May 03 2023 at 23:25):

sunfishcode created PR review comment:

I'm not sure how to parse "the pointer's valid is trivially derivable from any &Instance pointer.

Wasmtime GitHub notifications bot (May 04 2023 at 14:33):

alexcrichton created PR review comment:

Oh the reason for this is the NonNull wrapper makes it incompatible with null_mut. Is there something you think is worth explicitly calling out here though?

Wasmtime GitHub notifications bot (May 04 2023 at 14:35):

alexcrichton created PR review comment:

I won't pretend to be an expert on Stacked or Tree Borrows unfortunately, so all I can really say for sure is that this passes everything successfully there, so it's at least not a fundamental requirement. While I think it's best to model as carrying mutability it's not actually required due to the way the vmctx pointer is derived now (as it's derived from a pointer stored in self rather than purely from self). This means that the new *_mut methods from https://github.com/bytecodealliance/wasmtime/commit/0977952dcd9d482bff7c288868ccb52769b3a92e aren't actually required and we can technically go back to vmctx_plus_offset(&self, u32) -> *mut T, but I figured it's best to leave as-is for at least documentation purposes.

Wasmtime GitHub notifications bot (May 04 2023 at 14:40):

sunfishcode created PR review comment:

Ah, makes sense. Yeah, when I see a 1 being casted to a pointer, my first thought is "is this a magic value that something somewhere else knows about?". A comment stating that the intent is that, no, this is just intended as a placeholder would help.

Wasmtime GitHub notifications bot (May 04 2023 at 15:09):

alexcrichton created PR review comment:

Oops I typo'd and it's supposed to be s/valid/value/

Wasmtime GitHub notifications bot (May 04 2023 at 15:09):

alexcrichton updated PR #6338.

Wasmtime GitHub notifications bot (May 04 2023 at 15:10):

alexcrichton updated PR #6338.

Wasmtime GitHub notifications bot (May 04 2023 at 15:14):

alexcrichton updated PR #6338.

Wasmtime GitHub notifications bot (May 04 2023 at 18:14):

CraftSpider submitted PR review.

Wasmtime GitHub notifications bot (May 04 2023 at 18:14):

CraftSpider submitted PR review.

Wasmtime GitHub notifications bot (May 04 2023 at 18:14):

CraftSpider created PR review comment:

Is NonNull::dangling not sufficient here? Internally, it's just invalid_mut(align_of::<T>())

Wasmtime GitHub notifications bot (May 04 2023 at 18:22):

CraftSpider created PR review comment:

Drive by comment: *mut and *const are, as far as I'm aware, treated as functionally the same in SB/TB, with their mutability determined by how they're derived, not their type. So, &T -> *mut T -> &T and &mut T -> *const T -> &mut T roundtrips are fine, but &T -> *mut T -> &mut T isn't.

This works here for the reasons alex mentioned - since it's derived from the original selfref pointer, it keeps its permissions as long as it isn't invalidated.

Wasmtime GitHub notifications bot (May 04 2023 at 19:58):

alexcrichton updated PR #6338.

Wasmtime GitHub notifications bot (May 04 2023 at 19:58):

alexcrichton created PR review comment:

Ah yes that works perfectly!

Wasmtime GitHub notifications bot (May 08 2023 at 17:23):

sunfishcode submitted PR review:

Yes, I can do that.

Wasmtime GitHub notifications bot (May 09 2023 at 14:55):

alexcrichton merged PR #6338.

Last updated: Nov 22 2024 at 17:03 UTC