Closed
Bug 1265461
Opened 9 years ago
Closed 8 years ago
Import and pass the test suite from WebAssembly/spec
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox48 | --- | affected |
People
(Reporter: bbouvier, Assigned: bbouvier)
References
Details
Attachments
(7 files, 4 obsolete files)
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mbx
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mbx
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
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
Assignee | ||
Comment 2•9 years ago
|
||
Dump of my current state (will be split up later).
Attachment #8743499 -
Attachment is obsolete: true
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8745303 -
Attachment is obsolete: true
Attachment #8750352 -
Flags: review?(luke)
Assignee | ||
Comment 8•9 years ago
|
||
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)
Assignee | ||
Comment 10•9 years ago
|
||
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.
Updated•9 years ago
|
Attachment #8750358 -
Flags: review?(mbebenita) → review+
Comment 11•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8750352 -
Flags: review?(luke) → review+
Comment 12•9 years ago
|
||
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•9 years ago
|
||
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 14•9 years ago
|
||
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+
Assignee | ||
Comment 15•9 years ago
|
||
(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 16•9 years ago
|
||
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 17•9 years ago
|
||
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+
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Comment 18•9 years ago
|
||
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•9 years ago
|
||
bugherder |
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
Updated•8 years ago
|
Priority: -- → P2
Assignee | ||
Comment 21•8 years ago
|
||
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: 8 years ago
Resolution: --- → FIXED
Comment 22•7 years ago
|
||
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.
Description
•