wasmtime / issue #5891 test: add sample `poll_oneoff` WAT... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / issue #5891 test: add sample `poll_oneoff` WAT...

Wasmtime GitHub notifications bot (Feb 27 2023 at 23:12):

I expect this test to fail on the first CI pass since that is what I am seeing locally. Some logs:

$ RUST_LOG=trace target/release/wasmtime tests/all/cli_tests/poll_oneoff.wat
...
 TRACE wasi_common::snapshots::preview_1::wasi_snapshot_preview1 > wiggle abi; module="wasi_snapshot_preview1" function="poll_oneoff"
 TRACE tracing::span::active                                     > -> wiggle abi;
 TRACE wasi_common::snapshots::preview_1::wasi_snapshot_preview1 > in_=*guest 0x64 out=*guest 0xc8 nsubscriptions=1
 TRACE wasi_common::snapshots::preview_1::wasi_snapshot_preview1 > result=Err(Error { inner: Inval })
 TRACE tracing::span::active                                     > <- wiggle abi;
 TRACE tracing::span                                             > -- wiggle abi;
 TRACE wasmtime_runtime::traphandlers::backtrace                 > ====== Capturing Backtrace ======
...

It seems clear that Wasmtime rejects this call as invalid, but it is difficult for me to figure what exactly is invalid about the call.

Wasmtime GitHub notifications bot (Feb 28 2023 at 09:33):

yamt commented on issue #5891:

Presumably the call works in other runtimes (i.e., WAMR) since it was upstreamed by a WAMR contributor.

i'm sure it works for wamr and toywasm.

Wasmtime GitHub notifications bot (Feb 28 2023 at 14:59):

sunfishcode commented on issue #5891:

There are two issues here.

One is that the return type of poll_oneoff has multiple results, which Wasmtime interprets to mean that the core-wasm function should return the errno as its return value and return the number of events via an out parameter. This what wasi-libc expects too. However, the test expects the number of events will be returned as the return value. I recognize that there's poor documentation. I propose Wasmtime's and wasi-libc's interpretation is preferable here, because otherwise there seems to be no way for poll_oneoff to indicate failure. It's been proposed elsewhere that poll_oneoff ideally shouldn't fail itself, and should instead report failures on individual file descriptors as events, and I agree in principle, however in the Preview1 API, it's still expected that poll_oneoff as a whole can fail in some situations, such as when nsubscriptions is 0. If people agree, I'll volunteer to submit a PR to the WASI repo documenting that functions with multiple return values in witx return their first value as a return value and any other values via pointer arguments.

The other is that the test passes the address 100 as the subscriptions parameter, however the alignment of __wasi_subscription_t on wasm32-wasi is 8, so the address is not aligned, and Wasmtime is checking the alignment.

What should we do for misaligned addresses? I can see it both ways. On one hand, core Wasm load/store support misaligned addresses, with the caveat that they may be slow, so perhaps WASI calls should allow misaligned addresses for consistency with that. On the other, most producers do provide alignment, as evidenced by the fact that this issue hasn't been noticed until now, and requiring aligned addresses might help avoid subtle and bugs in host implementations.

Wasmtime GitHub notifications bot (Feb 28 2023 at 15:12):

yamt commented on issue #5891:

There are two issues here.

One is that the return type of poll_oneoff has multiple results, which Wasmtime interprets to mean that the core-wasm function should return the errno as its return value and return the number of events via an out parameter.

the original code in wasi-threads doesn't have this issue.

The other is that the test passes the address 100 as the subscriptions parameter, however the alignment of __wasi_subscription_t on wasm32-wasi is 8, so the address is not aligned, and Wasmtime is checking the alignment.

good point. i will fix it in wasi-threads tests.

Wasmtime GitHub notifications bot (Feb 28 2023 at 17:01):

sunfishcode commented on issue #5891:

I've now submitted https://github.com/WebAssembly/WASI/pull/523 to document how expected return types work in witx, as well as proposing a behavior for functions on misaligned pointers.

Wasmtime GitHub notifications bot (Feb 28 2023 at 22:50):

abrown commented on issue #5891:

One is that the return type of poll_oneoff has multiple results, which Wasmtime interprets to mean that the core-wasm function should return the errno as its return value and return the number of events via an out parameter

Sorry, this is my mistake for lifting the snippet from an entirely different context and tacking on some not-thought-through error checking. I can fix this if we want to keep this test. Do we want to keep this test (cc: @alexcrichton, @pchickey)? If you all want to include this, I will gladly fix this up. Otherwise I'll close this because the original issue — alignment — is now fixed in the testsuite.

Thanks @sunfishcode and @yamt for looking into this!

Wasmtime GitHub notifications bot (Feb 28 2023 at 22:50):

abrown edited a comment on issue #5891:

One is that the return type of poll_oneoff has multiple results, which Wasmtime interprets to mean that the core-wasm function should return the errno as its return value and return the number of events via an out parameter

Sorry, this is my mistake for lifting the snippet from an entirely different context and tacking on some not-thought-through error checking. I can fix this if we want to keep this test. Do we want to keep this test (cc: @alexcrichton, @pchickey)? If you all want to include this, I will gladly fix this up (along with the alignment). Otherwise I'll close this because the original issue — alignment — is now fixed in the testsuite.

Thanks @sunfishcode and @yamt for looking into this!

Last updated: Nov 22 2024 at 17:03 UTC