1265461 - Import and pass the test suite from WebAssembly/spec

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Benjamin Bouvier [:bbouvier]]

Attachment: import_tests.patch

Comment 1

[Benjamin Bouvier [:bbouvier]]

Attachment: wasm.spec.patch

block_comments.wast PASSED
block.wast PASSED
forward.wast PASSED
memory_redundancy.wast PASSED
names.wast PASSED

All other tests failing... Fortunately, it seems that's it's because of parsing: names in the wasm spec suite allow dashes, our impl does not. That should be easy to fix.

Comment 2

[Benjamin Bouvier [:bbouvier]]

Attachment: dump

Dump of my current state (will be split up later).

Comment 3

[Benjamin Bouvier [:bbouvier]]

Attachment: 1. Simple tweaks

Comment 4

[Benjamin Bouvier [:bbouvier]]

Attachment: 2. Allow several items in a local list

Comment 5

[Benjamin Bouvier [:bbouvier]]

Attachment: 3. Tweak the wasm over recursion message

Comment 6

[Benjamin Bouvier [:bbouvier]]

Attachment: 4. Allow imports to reference signatures by type

Comment 7

[Benjamin Bouvier [:bbouvier]]

Attachment: 5. Allow if/else to contain then/else plus a list of expressions

Comment 8

[Benjamin Bouvier [:bbouvier]]

Attachment: Import the wast tests

The interesting parts are in list.js and import_tests.sh. Everything else just contains the tests imported from https://github.com/WebAssembly/spec/tree/master/ml-proto/test

Comment 9

[Benjamin Bouvier [:bbouvier]]

Attachment: Enhance the wast interpreter

Comment 10

[Benjamin Bouvier [:bbouvier]]

Oops, just realized the list.js in the last patch should be in the patch of comment 8. For what it's worth, the latest list.js contains a list of all passing tests, and if they don't pass, a reason why.

Comment 11

[Michael Bebenita [:mbx]]

Comment on attachment 8750359 [details] [diff] [review]
Enhance the wast interpreter

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

Looks great!

::: js/src/jit-test/tests/wasm/spec/list.js
@@ +1,5 @@
>  // This is automatically generated by import_tests.sh. Do no modify by hand!
>  var specTests = [];
> +//specTests.push("address.wast");           // TODO wrapping offsets
> +//specTests.push("binary.wast");            // TODO binary text format
> +//specTests.push("block_comments.wast");    // PASS

Should tests that PASS be commented out?

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 8750353 [details] [diff] [review]
2. Allow several items in a local list

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

It seems like this change allows (param $i i32 i32 i32), but I don't see how that matches ml-proto:
  https://github.com/WebAssembly/spec/blob/master/ml-proto/host/parser.mly#L292

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 8750355 [details] [diff] [review]
4. Allow imports to reference signatures by type

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

Nice!

Comment 14

[Luke Wagner [:luke]]

Comment on attachment 8750357 [details] [diff] [review]
5. Allow if/else to contain then/else plus a list of expressions

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

::: js/src/asmjs/WasmTextToBinary.cpp
@@ +2657,5 @@
>      WasmAstExpr* cond = ParseExpr(c);
>      if (!cond)
>          return nullptr;
>  
> +    if (!c.ts.getIf(WasmToken::OpenParen))

I think this should be a match(), so that we report an error if we don't find the open-paren.

@@ +3556,2 @@
>      }
> +    r.popTarget(i.thenName());

IIUC, the 'then' block does not enclose the 'else' block; they are siblings.  If that's right, could you create a .wast test for the ml-proto test suite that uses integer indices to check this?  (I would've thought there would be one already.)

Comment 15

[Benjamin Bouvier [:bbouvier]]

Attachment: 2. Allow several items in a local list

(In reply to Luke Wagner [:luke] from comment #12)
> Comment on attachment 8750353 [details] [diff] [review]
> 2. Allow several items in a local list
> 
> Review of attachment 8750353 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It seems like this change allows (param $i i32 i32 i32), but I don't see how
> that matches ml-proto:
>  
> https://github.com/WebAssembly/spec/blob/master/ml-proto/host/parser.mly#L292

Indeed, I may have overlooked this. My ML is rusty, so I assume this means the most logical thing: either there is one name and one type only, or there are 0-n types.

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 8750730 [details] [diff] [review]
2. Allow several items in a local list

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

Exactly, yes.  Indeed, that line in parser.mly isn't even plain ocaml, it's the magic ocamlyacc pixie dust.

Comment 17

[Jason Orendorff [:jorendorff]]

Comment on attachment 8750354 [details] [diff] [review]
3. Tweak the wasm over recursion message

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

Sure... but is this because a test is expecting a specific string in an error message? Gross, if so.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8817ba456f6d
https://hg.mozilla.org/integration/mozilla-inbound/rev/92ddd15e624e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a8c24eec31f
https://hg.mozilla.org/integration/mozilla-inbound/rev/92ad104e5406
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1ee7c18da2f
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ba9f85eb25c
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f5cb07d3d5a

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8817ba456f6d
https://hg.mozilla.org/mozilla-central/rev/92ddd15e624e
https://hg.mozilla.org/mozilla-central/rev/8a8c24eec31f
https://hg.mozilla.org/mozilla-central/rev/92ad104e5406
https://hg.mozilla.org/mozilla-central/rev/b1ee7c18da2f
https://hg.mozilla.org/mozilla-central/rev/9ba9f85eb25c
https://hg.mozilla.org/mozilla-central/rev/9f5cb07d3d5a

Comment 20

[Hannes Verschore [:h4writer]]

Merge has happened. Moving the bugs from P2 to P1.

Comment 21

[Benjamin Bouvier [:bbouvier]]

With the landing of bug 1316554, we now pass all the spec tests up to those checked in in the github repo on November 17th. Since this is a never-ending scheme (import new tests -> implement missing functionality -> check in in gecko), and since we're at a point of time where the spec tests are pretty stable (not changing every other day), and all the features have been implemented in SpiderMonkey, I think it is time to close this meta-bug.

Comment 22

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.