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

MarkMcCaskey · 2019-11-12T20:55:23Z

Review

Add a short description of the the change to the CHANGELOG.md file

Cargo.toml

lib/runtime-c-api/Cargo.toml

bjfish · 2019-11-12T20:59:50Z

lib/wasi/src/utils.rs

+    Snapshot1,
+}
+
+pub fn get_wasi_version(module: &Module) -> Option<WasiVersion> {


Please rustdoc the public functions/types or suppress with doc hidden.

syrusakbary · 2019-11-12T21:13:51Z

Cargo.lock

@@ -1504,7 +1504,6 @@ dependencies = [
 "tempfile 3.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
 "wabt 0.9.2 (registry+https://github.com/rust-lang/crates.io-index)",
 "wasmer-clif-backend 0.10.1",
- "wasmer-llvm-backend 0.10.1",


Probably you don't want to commit this either

syrusakbary · 2019-11-13T00:20:44Z

lib/wasi/src/utils.rs

+
+    // returns None if empty
+    let first = import_iter.next()?;
+    if import_iter.all(|idx| idx == first) {


What is this .all(|idx| idx == first) doing?

Because Emscripten implements part of wasi (setting some of the imported functions as wasi_unstable), we need to check that all the imports have this namespace (not just one). That way we can differentiate Emscripten and WASI modules

It checks that all namespaces have the same index, which I understood to be a faster way to check if they all share a namespace than looking up every one and doing string comparisons.

After it guarantees that they're all the same, it does logic on it. I can add comments to clarify this logic

Actually, it creates a bug. If the import namespaces are the following: wasi_unstable and env, we have 2 different namespaces, and get_wasi_version will return None. Working on a fix.

@Hywan as mentioned in #1028 , I think this is not a bug and that the behavior in #1028 is much closer to a bug because we'll start calling things which are not WASI (won't run with WASI imports) WASI and have no way to detect real WASI anymore.

Is it specified in the specification that, in case of WASI, all imports must land in the same namespace? I think it is very restrictive, and would be break some design (like blockchain or stuff like that that have special namespaces too).

syrusakbary · 2019-11-13T00:21:36Z

lib/wasi/src/utils.rs

-            .get(import_name.namespace_index);
-        if namespace != "wasi_unstable" {
-            return false;
+    get_wasi_version(module) == Some(WasiVersion::Snapshot1)


I would do it just .is_some() ? (both snapshot0 and snapshot1 are wasi modules)

I was thinking that I'd attempt to preserve more of the semantics of the function call; either way this is a breaking change -- I think you're right that it doesn't make sense to keep the function name as what it is and not have that behavior. I'll update it

MarkMcCaskey · 2019-11-13T01:05:07Z

bors r+

957: Add support for new WASI snapshot, backwards compat too r=MarkMcCaskey a=MarkMcCaskey # Review - [x] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Mark McCaskey <mark@wasmer.io> Co-authored-by: Mark McCaskey <5770194+markmccaskey@users.noreply.github.com>

bors · 2019-11-13T01:30:34Z

Build failed

wasmerio.wasmer

MarkMcCaskey · 2019-11-21T18:58:14Z

bors try

MarkMcCaskey · 2019-11-21T18:59:40Z

bors try

bors · 2019-11-21T18:59:42Z

try

Already running a review

bors · 2019-11-21T19:28:03Z

try

Build failed

wasmerio.wasmer

…o/wasmer into feature/add-support-for-new-wasi

MarkMcCaskey · 2019-11-21T19:31:15Z

bors try

bors · 2019-11-21T21:00:57Z

try

Build failed

wasmerio.wasmer

MarkMcCaskey · 2019-11-21T22:01:30Z

bors try

bors · 2019-11-21T22:36:38Z

try

Build succeeded

wasmerio.wasmer

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>

MarkMcCaskey added the 📦 lib-wasi About wasmer-wasi label Nov 12, 2019

MarkMcCaskey requested review from bjfish, Hywan and syrusakbary as code owners November 12, 2019 20:55

syrusakbary reviewed Nov 12, 2019

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

syrusakbary reviewed Nov 12, 2019

View reviewed changes

lib/runtime-c-api/Cargo.toml Outdated Show resolved Hide resolved

syrusakbary approved these changes Nov 12, 2019

View reviewed changes

bjfish reviewed Nov 12, 2019

View reviewed changes

MarkMcCaskey force-pushed the feature/add-support-for-new-wasi branch from 6150fa6 to 3f2a84d Compare November 12, 2019 21:00

Add support for new WASI snapshot, backwards compat too

f1e5cd3

MarkMcCaskey force-pushed the feature/add-support-for-new-wasi branch from 3f2a84d to f1e5cd3 Compare November 12, 2019 21:01

syrusakbary reviewed Nov 12, 2019

View reviewed changes

syrusakbary reviewed Nov 13, 2019

View reviewed changes

Mark McCaskey and others added 2 commits November 12, 2019 17:02

Update from feedback, improve docs on new wasi fns

2b2a062

Merge branch 'master' into feature/add-support-for-new-wasi

064ffd3

Mark McCaskey and others added 2 commits November 21, 2019 10:57

Update from feedback, fix CI issues, update wasi-test

be217e8

Merge branch 'master' into feature/add-support-for-new-wasi

36a2671

bors bot added a commit that referenced this pull request Nov 21, 2019

Try #957:

8ffa7a0

Mark McCaskey added 2 commits November 21, 2019 11:30

More fixes for CI

ddccdb9

Merge branch 'feature/add-support-for-new-wasi' of github.com:wasmeri…

68fa9e2

…o/wasmer into feature/add-support-for-new-wasi

bors bot added a commit that referenced this pull request Nov 21, 2019

Try #957:

2395a54

Fix imports in test; copy manual implementations of Debug for wasi

bdeec52

bors bot added a commit that referenced this pull request Nov 21, 2019

Try #957:

8af2fbe

syrusakbary approved these changes Nov 22, 2019

View reviewed changes

syrusakbary merged commit b9138aa into master Nov 22, 2019

bors bot deleted the feature/add-support-for-new-wasi branch November 22, 2019 00:17

Hywan mentioned this pull request Dec 2, 2019

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

MarkMcCaskey commented Nov 12, 2019 •

edited

Loading

bjfish Nov 12, 2019

syrusakbary Nov 12, 2019

syrusakbary Nov 13, 2019

MarkMcCaskey Nov 13, 2019

Hywan Dec 2, 2019

MarkMcCaskey Dec 2, 2019

Hywan Dec 4, 2019

syrusakbary Nov 13, 2019

MarkMcCaskey Nov 13, 2019

MarkMcCaskey commented Nov 13, 2019

bors bot commented Nov 13, 2019

MarkMcCaskey commented Nov 21, 2019

MarkMcCaskey commented Nov 21, 2019

bors bot commented Nov 21, 2019

bors bot commented Nov 21, 2019

MarkMcCaskey commented Nov 21, 2019

bors bot commented Nov 21, 2019

MarkMcCaskey commented Nov 21, 2019

bors bot commented Nov 21, 2019

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

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

Conversation

MarkMcCaskey commented Nov 12, 2019 • edited Loading

Review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkMcCaskey commented Nov 13, 2019

bors bot commented Nov 13, 2019

Build failed

MarkMcCaskey commented Nov 21, 2019

MarkMcCaskey commented Nov 21, 2019

bors bot commented Nov 21, 2019

try

bors bot commented Nov 21, 2019

try

Build failed

MarkMcCaskey commented Nov 21, 2019

bors bot commented Nov 21, 2019

try

Build failed

MarkMcCaskey commented Nov 21, 2019

bors bot commented Nov 21, 2019

try

Build succeeded

MarkMcCaskey commented Nov 12, 2019 •

edited

Loading