feat(wasi) Introduce strict/non-strict modes for `get_wasi_version` #1028

Hywan · 2019-12-02T14:40:58Z

Sequel of #957.

If a module has multiple import namespaces, get_wasi_version is
broken because it assumes a module must only have a single namespace.

This patch updates get_wasi_version to introduce a strict flag:

In strict mode, the previous behavior applies; only one namespace is expected to be found, and this namespace must be a WASI namespace to detect the version,
In non-strict mode, at least one WASI namespace must be declared, and the first one is used to detect the version if any.

The non-strict mode is slower because it compares namespace strings.

2 new private constants have been declared: SNAPSHOT0_NAMESPACE and SNAPSHOT1_NAMESPACE to avoid repetition and bugs.

If a module has multiple import namespaces, `get_wasi_version` is broken because it assumes a module must only have a single namespace. This patch fixes it by a slower `get_wasi_version` function, but a correct one. As soon as the `wasi_unstable` or `wasi_snapshot_preview1` namespace is met, `get_wasi_version` maps it to the respective `WasiVersion` variant. It assumes however that a module must hold a unique WASI version.

Hywan · 2019-12-02T14:42:22Z

bors try

bors · 2019-12-02T17:26:06Z

try

Timed out

MarkMcCaskey

Syrus and I talked about a more complete solution for this using an upgraded version of Wasm interfaces https://github.com/wasmerio/wapm-cli/tree/master/lib/wasm-interface ; I think that's the way to go here:

The problem is that Emscripten contains some WASI functions now and this function is used by the Wasmer CLI tool to find out which import object will run the code -- if it contains extra imports, then it won't actually run.

I'll prioritize getting an MVP on this; perhaps it's fine to ship this as-is or to make another function that does this logic under a new name.

I'm going to approve the PR so I don't block you, but I'm not sure if this PR should be shipped as-is.

MarkMcCaskey · 2019-12-04T00:38:38Z

I think the best path forward is to expose functions for partial and complete detection, or have a boolean for strict vs not-strict mode. We can then put the solution we come up with later underneath that API

Hywan · 2019-12-04T12:08:15Z

I think a strict mode is a good idea. Gonna update the PR.

In strict mode, `get_wasi_version` uses the previous behavior, i.e. it checks that there is only one namespace for all imports, and that this namespace is a WASI namespace (and uses it to find the WASI version). In non-strict mode, `get_wasi_version` checks that at least one WASI namespace exists (and uses it to find the WASI version). By default, `is_wasi_module` uses the non-strict mode.

Hywan · 2019-12-04T14:24:45Z

@MarkMcCaskey Is it good to merge?

MarkMcCaskey

Looks good! Sorry I missed the notification for this yesterday!

the "single namespace" logic and documentation will change with the release of the next WASI version with modularity, but hopefully by then we have a simpler way of detecting this. Thinking about it now, the current detection isn't very precise

Hywan · 2019-12-06T12:37:39Z

bors r+

1028: feat(wasi) Introduce strict/non-strict modes for `get_wasi_version` r=Hywan a=Hywan Sequel of #957. If a module has multiple import namespaces, `get_wasi_version` is broken because it assumes a module must only have a single namespace. This patch updates `get_wasi_version` to introduce a `strict` flag: * In strict mode, the previous behavior applies; only one namespace is expected to be found, and this namespace must be a WASI namespace to detect the version, * In non-strict mode, at least one WASI namespace must be declared, and the first one is used to detect the version if any. The non-strict mode is slower because it compares namespace strings. 2 new private constants have been declared: `SNAPSHOT0_NAMESPACE` and `SNAPSHOT1_NAMESPACE` to avoid repetition and bugs. Co-authored-by: Ivan Enderlin <ivan.enderlin@hoa-project.net>

…ix-wasi-get-version

bors · 2019-12-06T13:03:21Z

Canceled

Hywan · 2019-12-06T13:06:03Z

bors r+

1028: feat(wasi) Introduce strict/non-strict modes for `get_wasi_version` r=Hywan a=Hywan Sequel of #957. If a module has multiple import namespaces, `get_wasi_version` is broken because it assumes a module must only have a single namespace. This patch updates `get_wasi_version` to introduce a `strict` flag: * In strict mode, the previous behavior applies; only one namespace is expected to be found, and this namespace must be a WASI namespace to detect the version, * In non-strict mode, at least one WASI namespace must be declared, and the first one is used to detect the version if any. The non-strict mode is slower because it compares namespace strings. 2 new private constants have been declared: `SNAPSHOT0_NAMESPACE` and `SNAPSHOT1_NAMESPACE` to avoid repetition and bugs. Co-authored-by: Ivan Enderlin <ivan.enderlin@hoa-project.net>

bors · 2019-12-06T13:56:49Z

Build succeeded

wasmerio.wasmer

1030: feat(runtime-c-api) Ability to generate `ImportObject` for a specific WASI version r=Hywan a=Hywan ❗️⚠️ Contains #1028, must be merged before this one. This patch introduces 2 new functions: * `wasmer_wasi_generate_import_object_for_version` and * `wasmer_wasi_get_version`. It mimics the current API provided by `wasmer_wasi`, nothing fancy here. It's just a regular port to C/C++. Because `wasmer_wasi::get_wasi_version` returns an option, and in order to simplify the C/C++ API, `wasmer_wasi_get_version` can return `Version::Unknown` in case of an error. It's up to the user to check the version is valid (i.e. not unknown). To see only the changes provided by this PR (excluding #1028), check Hywan/wasmer@fix-wasi-get-version...feat-runtime-c-api-wasi-version. Co-authored-by: Ivan Enderlin <ivan.enderlin@hoa-project.net>

92: feat(wasi) Support `WasiGetVersion` & other `*ForVersion` WASI API r=Hywan a=Hywan Fix #90. Fix #77. ⚠️ Depends on wasmerio/wasmer#1028, wasmerio/wasmer#1029, and wasmerio/wasmer#1030. They must be merged before merging this PR! Marking this PR as a draft to be sure (the shared libraries must be updated too). This patch updates `bridge.go` and `wasmer.h` to support the new `wasmer_wasi_generate_import_object_for_version` and `wasmer_wasi_get_version` functions. After that, this patch updates `wasi.go` to implement the new `NewDefaultWasiImportObjectForVersion`, `NewWasiImportObjectForVersion` and the `WasiGetVersion` functions. In addition to that, we create a new `WasiVersion` type. Finally, this patch updates the tests to test this new API. And it works like a charm 👌. Co-authored-by: Ivan Enderlin <ivan.enderlin@hoa-project.net>

Hywan added bug Something isn't working 📦 lib-wasi About wasmer-wasi labels Dec 2, 2019

Hywan requested a review from MarkMcCaskey as a code owner December 2, 2019 14:40

doc(changelog) Add wasmerio#1028.

8df0591

bors bot added a commit that referenced this pull request Dec 2, 2019

Try #1028:

360a346

This was referenced Dec 2, 2019

feat(runtime-c-api) Ability to generate ImportObject for a specific WASI version #1030

Merged

feat(wasi) Support WasiGetVersion & other *ForVersion WASI API wasmerio/wasmer-go#92

Merged

MarkMcCaskey approved these changes Dec 2, 2019

View reviewed changes

MarkMcCaskey mentioned this pull request Dec 2, 2019

Add support for new WASI snapshot, backwards compat too #957

Merged

1 task

Hywan added 2 commits December 4, 2019 13:29

doc(changelog) Update wasmerio#1028 description.

0a24133

Hywan changed the title ~~fix(wasi) get_wasi_version is broken with multiple namespaces~~ feat(wasi) Introduce strict/non-strict modes for get_wasi_version Dec 4, 2019

Merge branch 'master' into fix-wasi-get-version

871db85

MarkMcCaskey approved these changes Dec 5, 2019

View reviewed changes

Merge branch 'master' into fix-wasi-get-version

dc418a7

Hywan mentioned this pull request Dec 6, 2019

Is go-wasmer can handle C codes? wasmerio/wasmer-go#77

Closed

Hywan added 2 commits December 6, 2019 14:02

feat(bin) wasmer requires a strict WASI version detection.

f8385e2

Merge branch 'fix-wasi-get-version' of github.com:Hywan/wasmer into f…

72c8b66

…ix-wasi-get-version

Hywan requested a review from syrusakbary as a code owner December 6, 2019 13:03

test(wasi) Update get_wasi_version signature.

8aa49f5

bors bot merged commit 8aa49f5 into wasmerio:master Dec 6, 2019

uuhan mentioned this pull request Jun 17, 2021

Fix: get wasi versions right #2424

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(wasi) Introduce strict/non-strict modes for `get_wasi_version` #1028

feat(wasi) Introduce strict/non-strict modes for `get_wasi_version` #1028

Hywan commented Dec 2, 2019 •

edited

Loading

Hywan commented Dec 2, 2019

bors bot commented Dec 2, 2019

MarkMcCaskey left a comment

MarkMcCaskey commented Dec 4, 2019

Hywan commented Dec 4, 2019

Hywan commented Dec 4, 2019

MarkMcCaskey left a comment

Hywan commented Dec 6, 2019

bors bot commented Dec 6, 2019

Hywan commented Dec 6, 2019

bors bot commented Dec 6, 2019

feat(wasi) Introduce strict/non-strict modes for get_wasi_version #1028

feat(wasi) Introduce strict/non-strict modes for get_wasi_version #1028

Conversation

Hywan commented Dec 2, 2019 • edited Loading

Hywan commented Dec 2, 2019

bors bot commented Dec 2, 2019

try

Timed out

MarkMcCaskey left a comment

Choose a reason for hiding this comment

MarkMcCaskey commented Dec 4, 2019

Hywan commented Dec 4, 2019

Hywan commented Dec 4, 2019

MarkMcCaskey left a comment

Choose a reason for hiding this comment

Hywan commented Dec 6, 2019

bors bot commented Dec 6, 2019

Canceled

Hywan commented Dec 6, 2019

bors bot commented Dec 6, 2019

Build succeeded

feat(wasi) Introduce strict/non-strict modes for `get_wasi_version` #1028

feat(wasi) Introduce strict/non-strict modes for `get_wasi_version` #1028

Hywan commented Dec 2, 2019 •

edited

Loading