Closed Bug 1265461 Opened 4 years ago Closed 3 years ago

Import and pass the test suite from WebAssembly/spec

Categories

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

defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox48 --- affected

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(7 files, 4 obsolete files)

Attached patch import_tests.patch (obsolete) — Splinter Review
No description provided.
Attached patch wasm.spec.patch (obsolete) — Splinter Review
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.
Assignee: nobody → bbouvier
Attachment #8742426 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attached patch dump (obsolete) — Splinter Review
Dump of my current state (will be split up later).
Attachment #8743499 - Attachment is obsolete: true
Attached patch 1. Simple tweaksSplinter Review
Attachment #8745303 - Attachment is obsolete: true
Attachment #8750352 - Flags: review?(luke)
Attachment #8750353 - Flags: review?(luke)
Attachment #8750354 - Flags: review?(jorendorff)
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
Attachment #8750358 - Flags: review?(mbebenita)
Attachment #8750359 - Flags: review?(mbebenita)
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.
Attachment #8750358 - Flags: review?(mbebenita) → review+
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?
Attachment #8750359 - Flags: review?(mbebenita) → review+
Attachment #8750352 - Flags: review?(luke) → review+
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 on attachment 8750355 [details] [diff] [review]
4. Allow imports to reference signatures by type

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

Nice!
Attachment #8750355 - Flags: review?(luke) → review+
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.)
Attachment #8750357 - Flags: review?(luke) → review+
(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.
Attachment #8750353 - Attachment is obsolete: true
Attachment #8750353 - Flags: review?(luke)
Attachment #8750730 - Flags: review?(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.
Attachment #8750730 - Flags: review?(luke) → review+
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.
Attachment #8750354 - Flags: review?(jorendorff) → review+
Keywords: leave-open
Depends on: 1306478
Priority: -- → P2
Depends on: 1308056
Depends on: 1312483
Depends on: 1313024
Merge has happened. Moving the bugs from P2 to P1.
Priority: P2 → P1
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.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.