Closed Bug 1250551 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: mbx, Assigned: mbx)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → mbebenita
Blocks: wasm
Attached patch wasm-test.diff (obsolete) — Splinter Review
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...
Attachment #8722834 - Flags: review?(luke)
Attachment #8722834 - Flags: review?(bbouvier)
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?
Attachment #8722834 - Flags: review?(bbouvier) → review+
Attached patch wasm-tests.patchSplinter Review
Addressed review comments.
Attachment #8722834 - Attachment is obsolete: true
Attachment #8722834 - Flags: review?(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.
Should we just land this a short-term solution. It's a fairly low risk patch since it's just a test case.
Agreed!
https://hg.mozilla.org/mozilla-central/rev/eacf24b301ac
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.