Closed Bug 1313024 Opened 8 years ago Closed 8 years ago

Baldr: Pass 0xd specification tests

Categories

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

defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox52 --- affected

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(9 files, 1 obsolete file)

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)
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)
Updates the auto test importer...
Attachment #8804608 - Flags: review?(luke)
... and really import newer spec tests. Basic investigation of issues has been done, now digging a bit further.
Attachment #8804609 - Flags: review?(luke)
This allows memories and tables to be referenced by names and simplifies AstRef a bit.
Attachment #8804808 - Flags: review?(luke)
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)
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 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+
Attachment #8804608 - Flags: review?(luke) → review+
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+
Attachment #8804808 - Flags: review?(luke) → review+
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+
Attachment #8804810 - Flags: review?(luke) → review+
(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.
Marking P1 as discussed on irc
Priority: -- → P1
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)
Attached patch 8-known-failures.patch (obsolete) — Splinter Review
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 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.
(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
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 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 on attachment 8805185 [details] [diff] [review]
7.subfromstackptr.patch

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

Good find!
Attachment #8805185 - Flags: review?(jdemooij) → review+
(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.
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
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
(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 on attachment 8809078 [details] [diff] [review]
9.binary-parsing.patch

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

Great!
Attachment #8809078 - Flags: review?(luke) → review+
Should the leave-open keyword get removed with this latest push?
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
(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.
Depends on: 1316554
Works for me; opened bug 1316554 to keep track of the remaining tests.
Status: ASSIGNED → RESOLVED
Closed: 8 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.