1250551 - Make it possible to run wasm ml-proto spec tests directly.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Michael Bebenita [:mbx]]

The wasm ml-proto spec tests are defined in wasm "script" files which can hold multiple modules and have various assertions that test behavior. It would be nice to run these files unmodified in our testing harness. I propose writing a small wasm parser / interpreter in JavaScript for the top level s-expr nodes. The parser is just smart enough to strip out and eval modules, and interpret assert expressions.

I have a WIP patch which looks promising.

Comment 1

[Michael Bebenita [:mbx]]

Attachment: wasm-test.diff

This patch lets you run unmodified wasm files directly, as long as you add them to a list in spec.js. It would be nice if we could make jit_test.py look for *.wasm files and run them through spec.js automatically. I don't know the test harness code well enough to do that, so if someone wants to help here...

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8722834 [details] [diff] [review]
wasm-test.diff

Review of attachment 8722834 [details] [diff] [review]:
-----------------------------------------------------------------

I like it!

::: js/src/jit-test/tests/wasm/spec.js
@@ +10,5 @@
> +
> +// Element list or string.
> +function Element(str, dollared, quoted) {
> +    this.list = [];
> +    this.str = str == null ? null : str;

I am always dubious of == when one of the sides is `null`, because of the implicit coercions (there are also uses of != below). Moreover, === and !== give more optimizations opportunities.

@@ +19,5 @@
> +    if (this.str != null) {
> +        if (this.dollared) {
> +            return "$" + this.str;
> +        } else if (this.quoted) {
> +            return '"' + this.str + '"';

(template strings ftw?)

@@ +49,5 @@
> +            ret.list.push(curr);
> +        }
> +    }
> +    function isspace(c) {
> +        return c == ' ' || c == '\n' || c == '\t';

There are a few more space chars: https://dxr.mozilla.org/mozilla-central/source/js/src/asmjs/WasmText.cpp?from=WasmText.cpp#934

@@ +124,5 @@
> +
> +var module = null;
> +var moduleText = null;
> +
> +// Recursively the expression.

nit: missing word

@@ +137,5 @@
> +        }
> +    } else if (exprName == "invoke") {
> +        var name = e.list[1].str;
> +        var args = e.list.slice(2).map(exec);
> +        var fn = null;

maybe assert(module !== null); ?

@@ +162,5 @@
> +}
> +
> +var tests = [];
> +
> +tests.push("spec/names.wast");

Alternative idea: put the `tests` variable in a different file called 'list.js' (or 'index.js' or whatever) within the spec/ directory, import this file with `load` in this file. This way, the list of tests and the way to parse those are cleanly split, and it's trivial finding where to add a test.

@@ +164,5 @@
> +var tests = [];
> +
> +tests.push("spec/names.wast");
> +
> +for (var test of tests) {

Could we reset module/moduleText between each test?

Comment 3

[Michael Bebenita [:mbx]]

Attachment: wasm-tests.patch

Addressed review comments.

Comment 4

[Luke Wagner [:luke]]

That's a cool way to do it!  Definitely seems valuable in the short-term for quickly importing the ml-proto test suite.  I had been thinking that the "right" way to do this was just to add a shell flag that allowed SM shell to directly load and execute a .wast (just like a .js file).  The advantage is that this .wast interpreter could create wasm modules that had i64, non-canonical f32/f64 which cannot be (currently) tested from pure JS.  However, that will take some further refactoring of wasm::Module and I wasn't going to be able to get to that for a little while.

Comment 5

[Michael Bebenita [:mbx]]

Should we just land this a short-term solution. It's a fairly low risk patch since it's just a test case.

Comment 6

[Luke Wagner [:luke]]

Agreed!

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eacf24b301ac

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eacf24b301ac