Stream: git-wasmtime

Topic: wasmtime / PR #6440 feat(preview1): implement core I/O fu...

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 16:58):

rvolosatovs opened PR #6440 from rvolosatovs:feat/preview1-preview2 to bytecodealliance:main:

Moved from https://github.com/bytecodealliance/preview2-prototyping/pull/175

There's a platform-specific bug somewhere, which I suspect originates somewhere in path_open flag translation logic

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 17:05):

rvolosatovs updated PR #6440.

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 17:40):

rvolosatovs updated PR #6440.

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 17:42):

rvolosatovs edited PR #6440:

Moved from https://github.com/bytecodealliance/preview2-prototyping/pull/175

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 18:25):

rvolosatovs edited PR #6440:

Moved from https://github.com/bytecodealliance/preview2-prototyping/pull/175

The implementation tries to follow existing preview1 behavior as much as possible - as a result, there are a few differences in behavior from existing adapter. Please see the original PR for more details.

A follow-up PRs fill be filed for:

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 18:25):

rvolosatovs has marked PR #6440 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 18:25):

rvolosatovs requested itsrainy for a review on PR #6440.

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 18:25):

rvolosatovs requested wasmtime-core-reviewers for a review on PR #6440.

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 18:27):

rvolosatovs edited PR #6440:

Moved from https://github.com/bytecodealliance/preview2-prototyping/pull/175

The implementation tries to follow existing preview1 behavior as much as possible - as a result, there are a few differences in behavior from existing adapter. Please see the original PR for more details.

A follow-up PRs fill be filed for:

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 20:20):

itsrainy requested pchickey for a review on PR #6440.

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 20:58):

pchickey submitted PR review:

This is great work! Just a few small issues. Thank you for finding all of these bad behaviors in the legacy implementation!

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 20:58):

pchickey submitted PR review:

This is great work! Just a few small issues. Thank you for finding all of these bad behaviors in the legacy implementation!

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 20:58):

pchickey created PR review comment:

this is still a todo!() - we can leave it to a follow up PR, but unreachable!() communicates to me that it is a bug in this code if it is reachable, while this is reachable from any guest program.

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 20:58):

pchickey created PR review comment:

I wasn't expecting this sort of machinery to be required - we can't get away with implementing the DescriptorsRef methods on WasiPreview1Ctx? I can't intuit where the problem is in a more straightforward implementation, so if this is required, lets leave a comment on the struct definition explaining why.

view this post on Zulip Wasmtime GitHub notifications bot (May 23 2023 at 20:58):

pchickey created PR review comment:

lets definitely not preserve that bug :). That one has been around for a while! Lets get rid of this check and follow up with a PR to fix the legacy impl, and a test. I expect that fix will only be around for its final release, but its still worth doing.

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 10:45):

rvolosatovs created PR review comment:

I took this directly from https://github.com/bytecodealliance/wasmtime/blob/5b93cdb84def5fa7cb3ff823bc80216f19c296c5/crates/wasi-preview1-component-adapter/src/lib.rs#L560-L569 and it was surprising to me as well.
It seems to me that the adapter has to implement rights handling, which the existing one does not do. I'll mark this as todo! and file an issue for rights handling follow-up

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 11:40):

rvolosatovs created PR review comment:

I'll add a comment, but the biggest reasons why this is required are:

  1. By far the biggest issue is bindgen generating host methods with &mut self receivers, preventing e.g. simultaneous lazy initialization of the descriptor table and calling a preview2 method using any data borrowed from that table, even looking up something like table().is_file is problematic
  2. WasiPreview1Adapter (assuming that's what you meant by WasiPreview1Ctx) does not have access to preview2 functionality and/or the underlying table, so it cannot initialize itself using something like https://docs.rs/once_cell/latest/once_cell/sync/struct.Lazy.html . Lazily initializing the descriptor map requires access to both the WasiPreview1Adapter and wasi::preopens::Host, so the only place where this functionality could exist is in an extension trait (WasiPreview1ViewExt). Note that from within this trait, we can never return anything borrowed directly, because we'd first need to call adapter_mut() to get access to the state (which we may need to lazily initialize). And even if we had a way to return a borrowed descriptor table directly, it would not be possible to call any other method on self due to 1., since that would cause a double mutable borrow, which is not allowed.

In short, an alternative solution would be storing Option<Arc<Mutex<Descriptors>>> in the adapter and returning Arc<Mutex<Descriptors>> in the WasiPreview1AdapterExt::descriptors(&mut self) method, which IMO is way more awkward to work with and simply redundant complexity.
Overall, the DescriptorsRef is just a workaround for bindgen design drawbacks. I believe this could be simplified dramatically if host methods would just take &self

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 12:34):

rvolosatovs updated PR #6440.

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 12:40):

rvolosatovs requested pchickey for a review on PR #6440.

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 12:40):

rvolosatovs edited PR #6440:

Moved from https://github.com/bytecodealliance/preview2-prototyping/pull/175

The implementation tries to follow existing preview1 behavior as much as possible - as a result, there are a few differences in behavior from existing adapter. Please see the original PR for more details.

A follow-up PRs fill be filed for:

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 12:40):

rvolosatovs edited PR #6440:

Moved from https://github.com/bytecodealliance/preview2-prototyping/pull/175

The implementation tries to follow existing preview1 behavior as much as possible - as a result, there are a few differences in behavior from existing adapter. Please see the original PR for more details.

A follow-up issues/PRs fill be filed for:

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 14:28):

rvolosatovs edited PR #6440:

Moved from https://github.com/bytecodealliance/preview2-prototyping/pull/175

Refs https://github.com/bytecodealliance/wasmtime/issues/6370

The implementation tries to follow existing preview1 behavior as much as possible - as a result, there are a few differences in behavior from existing adapter. Please see the original PR for more details.

A follow-up issues/PRs fill be filed for:

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 15:02):

pchickey created PR review comment:

Oh, that is indeed a bug in the component adapter as well. unreachabable!() is just how we spell todo!() and panic!() and etc over there since we cant use std's macros.

In the legacy implementation, set_rights is a no-op, it only validates the fd. That is the correct behavior for both of these spots. Would you mind making this fix to the component adapter as well?

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 15:04):

pchickey created PR review comment:

That makes sense. Thank you.

We should look into adding a bindgen option to enable &self receivers in the future, and then we may be able to unwind a bit of this complexity. For now, this is a fine implementation once it has a comment.

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 15:37):

rvolosatovs created PR review comment:

I've added a few comments and renamed the struct to Transaction so it's a bit more obvious what it actually is

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 15:38):

rvolosatovs created PR review comment:

Let's take care of it as part of https://github.com/bytecodealliance/wasmtime/issues/6446 ?

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 16:00):

rvolosatovs created PR review comment:

https://github.com/bytecodealliance/wasmtime/pull/6444

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 17:29):

pchickey submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 24 2023 at 18:07):

pchickey merged PR #6440.

Last updated: Nov 22 2024 at 16:03 UTC