Baldr: Pass 0xd specification tests

RESOLVED FIXED

Status

()

Core
JavaScript Engine: JIT
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bbouvier, Assigned: bbouvier)

Tracking

({leave-open})

Trunk
leave-open
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 affected)

Details

Attachments

(9 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
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

a year ago
Created attachment 8804607 [details] [diff] [review]
1-enable-more-tests.patch

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

a year ago
Created attachment 8804608 [details] [diff] [review]
2.update-importer.patch

Updates the auto test importer...
Attachment #8804608 - Flags: review?(luke)
(Assignee)

Comment 3

a year ago
Created attachment 8804609 [details] [diff] [review]
3.reimport-tests.patch

... 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

a year ago
Created attachment 8804808 [details] [diff] [review]
4.allow-table-memory-ref.patch

This allows memories and tables to be referenced by names and simplifies AstRef a bit.
Attachment #8804808 - Flags: review?(luke)
(Assignee)

Comment 5

a year ago
Created attachment 8804809 [details] [diff] [review]
5.data-seg-several-lines.patch

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

a year ago
Created attachment 8804810 [details] [diff] [review]
6.assert-soft-invalid.patch

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+

Updated

a year ago
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+

Updated

a year ago
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+

Updated

a year ago
Attachment #8804810 - Flags: review?(luke) → review+
(Assignee)

Comment 10

a year 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.
Marking P1 as discussed on irc
Priority: -- → P1
(Assignee)

Comment 12

a year ago
Created attachment 8805185 [details] [diff] [review]
7.subfromstackptr.patch

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

a year ago
Created attachment 8805188 [details] [diff] [review]
8-known-failures.patch

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.
(Assignee)

Comment 15

a year 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

a year ago
Created attachment 8805220 [details] [diff] [review]
8-segments-init.patch

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+
(Assignee)

Comment 19

a year 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

a year 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

a year 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
(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

a year 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

a year ago
Created attachment 8809078 [details] [diff] [review]
9.binary-parsing.patch
Attachment #8809078 - Flags: review?(luke)
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

a year ago
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0b9e37236b5
Pass binary.wast; r=luke
Should the leave-open keyword get removed with this latest push?
(Assignee)

Comment 28

a year 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
(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)

Updated

a year ago
Depends on: 1316554
(Assignee)

Comment 30

a year ago
Works for me; opened bug 1316554 to keep track of the remaining tests.
(Assignee)

Updated

a year ago
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED

Comment 31

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f0b9e37236b5
You need to log in before you can comment on or make changes to this bug.