Closed
Bug 1313024
Opened 8 years ago
Closed 8 years ago
Baldr: Pass 0xd specification tests
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox52 | --- | affected |
People
(Reporter: bbouvier, Assigned: bbouvier)
References
Details
Attachments
(9 files, 1 obsolete file)
18.47 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
1.05 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
275.81 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
31.01 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
11.60 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
5.86 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
13.59 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
12.51 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
7.26 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
Plan: - pass most tests as existing in the repository - import new tests - investigate which can be easily enabled (e.g. parser changes) and which need actual features (e.g. assign host functions to table elements)
Assignee | ||
Comment 1•8 years ago
|
||
This makes us pass all (but the binary parser spec test) as existing in the repo, by: - enhancing comment/whitespace support in the wast JS interpreter - allowing non-utf8 chars when reading files in js::shell::ReadAsFiles (this happens in comments.wast) - allowing imports of memories/tables when the expected limits have no max, but the actual one has some, per https://github.com/WebAssembly/spec/blob/master/interpreter/spec/eval.ml#L368
Attachment #8804607 -
Flags: review?(luke)
Assignee | ||
Comment 2•8 years ago
|
||
Updates the auto test importer...
Attachment #8804608 -
Flags: review?(luke)
Assignee | ||
Comment 3•8 years ago
|
||
... and really import newer spec tests. Basic investigation of issues has been done, now digging a bit further.
Attachment #8804609 -
Flags: review?(luke)
Assignee | ||
Comment 4•8 years ago
|
||
This allows memories and tables to be referenced by names and simplifies AstRef a bit.
Attachment #8804808 -
Flags: review?(luke)
Assignee | ||
Comment 5•8 years ago
|
||
This allows data segments to be printed on several lines. I had an idea while making this patch: since a data segment can be associated to several AstNames now, we can take advantage of this when printing the linear format by breaking long data segments into several lines.
Attachment #8804809 -
Flags: review?(luke)
Assignee | ||
Comment 6•8 years ago
|
||
This implements assert_soft_invalid, which is not very useful. soft-invalid.wast tests validation for code that's unreachable or dead. The test header says that an impl may or may not classify these test cases as invalid (since the invalid code is in dead code), but if it does, the right error must be returned. So we can't do any better than just looking at the output and hoping that when SM returns an error during validation, it's the right one. I've done that and it seems true at this point... Other errors have been investigated a bit more and probably need more investigation or fixes: - imported host (JS) functions in wasm tables - signature checks against exotic exported function objects - type-check br to the top of a loop - get_global initializer expression can reference an own immutable global (vs imported only at the moment in SM) - the check that data segments fit must be done at instanciation, not compilation - a module with a 0-sized memory allows empty data segments at any offsets (? might be moot)
Attachment #8804810 -
Flags: review?(luke)
Comment 7•8 years ago
|
||
Comment on attachment 8804607 [details] [diff] [review] 1-enable-more-tests.patch Review of attachment 8804607 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/asmjs/WasmModule.cpp @@ +714,5 @@ > JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_WASM_BAD_IMP_SIZE, kind); > return false; > } > > + if ((actualMax && declaredMax && *actualMax > *declaredMax) || (!actualMax && declaredMax)) { Oh wow, real logic fail here, thanks!
Attachment #8804607 -
Flags: review?(luke) → review+
Updated•8 years ago
|
Attachment #8804608 -
Flags: review?(luke) → review+
Comment 8•8 years ago
|
||
Comment on attachment 8804609 [details] [diff] [review] 3.reimport-tests.patch Review of attachment 8804609 [details] [diff] [review]: ----------------------------------------------------------------- Great! ::: js/src/jit-test/tests/wasm/spec/memory.wast.js @@ +1,2 @@ > // |jit-test| test-also-wasm-baseline > +// TODO initializer expression can reference global mutable (module-defined) variables? It shouldn't be able to reference *any* module-defined variables: https://github.com/WebAssembly/design/blob/master/Modules.md#initializer-expression (Otherwise you could have cyclic definitions :) Could you file a spec/interpreter bug?
Attachment #8804609 -
Flags: review?(luke) → review+
Updated•8 years ago
|
Attachment #8804808 -
Flags: review?(luke) → review+
Comment 9•8 years ago
|
||
Comment on attachment 8804809 [details] [diff] [review] 5.data-seg-several-lines.patch Review of attachment 8804809 [details] [diff] [review]: ----------------------------------------------------------------- Nice
Attachment #8804809 -
Flags: review?(luke) → review+
Updated•8 years ago
|
Attachment #8804810 -
Flags: review?(luke) → review+
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #8) > Comment on attachment 8804609 [details] [diff] [review] > 3.reimport-tests.patch > > Review of attachment 8804609 [details] [diff] [review]: > ----------------------------------------------------------------- > > Great! > > ::: js/src/jit-test/tests/wasm/spec/memory.wast.js > @@ +1,2 @@ > > // |jit-test| test-also-wasm-baseline > > +// TODO initializer expression can reference global mutable (module-defined) variables? > > It shouldn't be able to reference *any* module-defined variables: > > https://github.com/WebAssembly/design/blob/master/Modules.md#initializer- > expression > (Otherwise you could have cyclic definitions :) Could you file a > spec/interpreter bug? I've opened https://github.com/WebAssembly/spec/issues/367 for that matter.
Assignee | ||
Comment 12•8 years ago
|
||
So wasm actually uses subFromStackPtr with big depths, which triggers the guard stack page segfault on windows, on a very specific wasm test designed for that purpose. The piece of code that's in reserveStack under x64/x86 really ought to be in subFromStackPtr instead, so users don't have to worry about shooting themselves in the feet. reserveStack can just call subFromStackPtr then and be generalized; except on arm64, which doesn't have subFromStackPtr but something else. This breaks the masm style checks (which can't handle different declarations under #ifdef), so we have to move the impls outside the check_macroassembler_style blocks.
Attachment #8805185 -
Flags: review?(jdemooij)
Assignee | ||
Comment 13•8 years ago
|
||
This adds a way to mark failures as known in the wast interpreter, because some infra VMs don't have enough memory for one test in resizing.wast that adds 10k pages of memory. It's a bit unfortunate to add this, but I am not too sure of any better way, but to inhibit the test entirely.
Attachment #8805188 -
Flags: review?(luke)
Comment 14•8 years ago
|
||
Comment on attachment 8805188 [details] [diff] [review] 8-known-failures.patch Review of attachment 8805188 [details] [diff] [review]: ----------------------------------------------------------------- 655mb is actually kindof a lot of memory; do you suppose we could instead try changing the test upstream to request a more modest amount? I would assume that the testing value drops off heavily after, say, 50mb.
Assignee | ||
Comment 15•8 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #14) > Comment on attachment 8805188 [details] [diff] [review] > 8-known-failures.patch > > Review of attachment 8805188 [details] [diff] [review]: > ----------------------------------------------------------------- > > 655mb is actually kindof a lot of memory; do you suppose we could instead > try changing the test upstream to request a more modest amount? I would > assume that the testing value drops off heavily after, say, 50mb. Good call! Let's see how the VM handle that first, to make sure that's low enough: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a1df588cd772d2f32832920e952a55037e08fb6
Assignee | ||
Comment 16•8 years ago
|
||
Let's discard the known-failures patch at the moment. https://github.com/WebAssembly/design/blob/master/Modules.md#initializer-expression says: Initializer expressions are evaluated at instantiation time. So let's delay initializer expressions errors for constant init offsets. It's a kind of a shame to not be able to tell at compile time that there is going to be an error at instanciation time 100% of the time, but only constant-foldable expressions fall under this case, and it is not spec'd at all at the moment, plus it sounds like an implementation detail. So I think here we should align with other implementations. Also, this allows 0-length segments at any offset. This is tested in spec/memory.wast, but since this test is blocked by the github issue, I've added JS tests too.
Attachment #8805188 -
Attachment is obsolete: true
Attachment #8805188 -
Flags: review?(luke)
Attachment #8805220 -
Flags: review?(luke)
Comment 17•8 years ago
|
||
Comment on attachment 8805220 [details] [diff] [review] 8-segments-init.patch Review of attachment 8805220 [details] [diff] [review]: ----------------------------------------------------------------- Hah, ok then. ::: js/src/jit-test/tests/wasm/binary.js @@ -286,5 @@ > wasmEval(moduleWithSections([tableSection(1), elemSection([{offset:1, elems:[]}])])); > assertErrorMessage(() => wasmEval(moduleWithSections([tableSection(1), elemSection([{offset:0, elems:[0]}])])), CompileError, /table element out of range/); > -assertErrorMessage(() => wasmEval(moduleWithSections([tableSection(0), elemSection([{offset:0, elems:[0]}])])), CompileError, /element segment does not fit/); > -assertErrorMessage(() => wasmEval(moduleWithSections([tableSection(1), elemSection([{offset:1, elems:[0]}])])), CompileError, /element segment does not fit/); > -assertErrorMessage(() => wasmEval(moduleWithSections([tableSection(1), elemSection([{offset:0, elems:[0,0]}])])), CompileError, /element segment does not fit/); I'm assuming the reason to remove these from binary.js is that they are now subsumed by the (more-readable) text format tests?
Attachment #8805220 -
Flags: review?(luke) → review+
Comment 18•8 years ago
|
||
Comment on attachment 8805185 [details] [diff] [review] 7.subfromstackptr.patch Review of attachment 8805185 [details] [diff] [review]: ----------------------------------------------------------------- Good find!
Attachment #8805185 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 19•8 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #17) > Comment on attachment 8805220 [details] [diff] [review] > 8-segments-init.patch > > Review of attachment 8805220 [details] [diff] [review]: > ----------------------------------------------------------------- > > Hah, ok then. > > ::: js/src/jit-test/tests/wasm/binary.js > @@ -286,5 @@ > > wasmEval(moduleWithSections([tableSection(1), elemSection([{offset:1, elems:[]}])])); > > assertErrorMessage(() => wasmEval(moduleWithSections([tableSection(1), elemSection([{offset:0, elems:[0]}])])), CompileError, /table element out of range/); > > -assertErrorMessage(() => wasmEval(moduleWithSections([tableSection(0), elemSection([{offset:0, elems:[0]}])])), CompileError, /element segment does not fit/); > > -assertErrorMessage(() => wasmEval(moduleWithSections([tableSection(1), elemSection([{offset:1, elems:[0]}])])), CompileError, /element segment does not fit/); > > -assertErrorMessage(() => wasmEval(moduleWithSections([tableSection(1), elemSection([{offset:0, elems:[0,0]}])])), CompileError, /element segment does not fit/); > > I'm assuming the reason to remove these from binary.js is that they are now > subsumed by the (more-readable) text format tests? Yes indeed.
Comment 20•8 years ago
|
||
Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c9ecc2d14114 Enable a few more spec tests; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/0418a48ed6f2 Update import_tests.sh to the latest spec repository; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/4963d6702c2c Reimport tests; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/ef0ab8f78fe9 Allow table/memory references in wast and simplify AstRef; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/58c6affba442 Allow data sections to be displayed on several lines; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/fb3cd4dc8aef Implement assert_soft_invalid in the wast interpreter; r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/9a5184bfa387 Generalize stack guard page touching in subFromStackPtr; r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/94efce672651 Raise segments errors at wasm instanciation, not compilation; r=luke
Assignee | ||
Comment 21•8 years ago
|
||
After this, a re-update of the latest tests, the only ones that are disabled are: - binary.wast (which would need a binary parser in WasmTextToBinary to be implemented -- WONTFIX) - linking.wast (for the extra signature checks for JS function wrappers; to be implemented in bug 1313180) - memory.wast (for the test discussed in https://github.com/WebAssembly/spec/issues/367) - imports.wast (for any imported JS functions to be put in tables; bug 1313180) Leaving open for the memory test.
Keywords: leave-open
Comment 22•8 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #21) Really great work here, thanks! > - binary.wast (which would need a binary parser in WasmTextToBinary to be > implemented -- WONTFIX) Actually, would this be very difficult? I'd assume it would just involve handling WasmToken::Text as a special case before the WasmToken::OpenParen loop in ParseModule(). I know binary.wast is puny atm, but I expect, with the binary format stable, we'll get a bunch more binary tests in the future and probably for interesting corner cases.
Comment 23•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c9ecc2d14114 https://hg.mozilla.org/mozilla-central/rev/0418a48ed6f2 https://hg.mozilla.org/mozilla-central/rev/4963d6702c2c https://hg.mozilla.org/mozilla-central/rev/ef0ab8f78fe9 https://hg.mozilla.org/mozilla-central/rev/58c6affba442 https://hg.mozilla.org/mozilla-central/rev/fb3cd4dc8aef https://hg.mozilla.org/mozilla-central/rev/9a5184bfa387 https://hg.mozilla.org/mozilla-central/rev/94efce672651
Assignee | ||
Comment 24•8 years ago
|
||
Attachment #8809078 -
Flags: review?(luke)
Comment 25•8 years ago
|
||
Comment on attachment 8809078 [details] [diff] [review] 9.binary-parsing.patch Review of attachment 8809078 [details] [diff] [review]: ----------------------------------------------------------------- Great!
Attachment #8809078 -
Flags: review?(luke) → review+
Comment 26•8 years ago
|
||
Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f0b9e37236b5 Pass binary.wast; r=luke
Comment 27•8 years ago
|
||
Should the leave-open keyword get removed with this latest push?
Assignee | ||
Comment 28•8 years ago
|
||
No, not yet; the spec tests probably need to get updated, plus there's one moot test that needs to get removed from the spec repo: https://github.com/WebAssembly/spec/issues/367
Comment 29•8 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #28) > No, not yet; the spec tests probably need to get updated, plus there's one > moot test that needs to get removed from the spec repo: > https://github.com/WebAssembly/spec/issues/367 Fyi the tree will get merged on Monday and it is always a hassle if bugs span multiple release. Since the "tracking/target/..." can only contain one value. If a patch will land in the next train, it might be good to create a new bug tracking those patches separately instead of in this bug report. That bug should block this one in that case.
Assignee | ||
Comment 30•8 years ago
|
||
Works for me; opened bug 1316554 to keep track of the remaining tests.
Assignee | ||
Updated•8 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment 31•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f0b9e37236b5
Comment 32•6 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
•