1546138 - Baldr: implement funcref

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P1)

Description

[Luke Wagner [:luke]]

To complete our implementation of reference-types (bug 1508553). Specifically this means adding:

• the funcref type
• allow table.get on funcref table to validate
• add ref.func operator

The implementation for the latter two doesn't to be optimal; we can just call into C++ to do WasmInstanceObject::getExportedFunction. Later, we'll want to change the representation of function objects so that we can do something much more efficient and inline. But apparently V8 also started with a similar, slow implementation so we should do likewise for now so reference types can advance stages and ship.

Comment 1

[Lars T Hansen [:lth]]

Also table.set, table.fill, table.grow with funcref arguments on table-of-funcref. (Of these, code for table.fill on table-of-anyref is about to land and the other two are already present; table.grow on table-of-funcref should currently work if the init argument is a nullref type, as might table.set.)

Comment 2

[Luke Wagner [:luke]]

Attachment: Bug 1546138 - Baldr: reflect 'anyfunc' to 'funcref' renaming in methods (r=lth)

Comment 3

[Luke Wagner [:luke]]

Attachment: Bug 1546138 - Baldr: add FuncRef to ValType (r=lth)

Depends on D28723

Comment 4

[Luke Wagner [:luke]]

Looking into table.get, I see that to avoid a rooting issue, we're returning the address of the table entry containing the anyref, not the anyref word itself. That's a problem for table.get on a funcref table, since the returned funcref is dynamically looked up via WasmInstanceObject::getExportedFunction() and so there's no corresponding element whose address can be returned.

The comment (https://searchfox.org/mozilla-central/source/js/src/wasm/WasmInstance.cpp#854-857) describes a rooting issue, but I can't quite see where it is. What would go wrong if, on trunk, I tried to return the anyref by value and assigned the result of the MWasmCall the appropriate MIRType so that it was included in stackmaps?

Comment 5

[Lars T Hansen [:lth]]

table.get can return three different kinds of value:

• a non-null table element value
• a null table element value
• a signal that the access was out of bounds

At the moment, we return null for the OOB signal and otherwise a pointer to a slot that holds the value. Really we want to avoid the indirection and return just the null/non-null value or an OOB value, eg -1, but since we don't fully control what Ion does with the value we risk the -1 ending up in a rooted location and being exposed to GC before we check it for a trap. This basically comes down to how tightly coupled the instance call and the subsequent check are in code generated by Ion, since they are two MIR nodes that can in principle drift apart during optimization.

(The baseline compiler is not an issue. Cranelift may be an issue, eventually.)

Thus any fix here either couples the MIR nodes more tightly so that we guarantee that this is not a problem, or finds a sentinel value that can be exposed to GC that won't cause a problem. I'm fine with either solution. One possibility is to store a sentinel object on the tls and compare to that; it's a little more work but probably manageable.

Now it's possible I've misunderstood something and that the GC is fine with seeing eg -1, or that there are enough constraints in place that the MIR nodes will never drift apart in a meaningful sense, but iirc the code is the way it is because I encountered a problem or could not convince myself there wasn't one.

Comment 6

[Lars T Hansen [:lth]]

One more thing: The principled solution here is really to stack-allocate a slot for the flag and pass a pointer to that to tableGet(), and for tableGet() to return a pointer (which is null if the flag says failed, say). (Or vice versa.) Since there's no mechanism as yet for stack allocation of memory like this in the baseline compiler, even though I think there is in Ion, it was decided in the heat of the battle to do what we currently do. But one could consider fixing the problem by introducing a stack-allocation method in Rabaldr.

Comment 7

[Luke Wagner [:luke]]

Stack allocation would be one solution, but I think this will add some additional complexity to MWasmCall. Instead, my inclination would be to have MWasmCall do the trap check in the codegen of LWasmCall (and thus inseparable by optimization and unobservable by GC).

Comment 8

[Luke Wagner [:luke]]

Attachment: Bug 1546138 - Baldr: remove (ref T) special case from Val union (r=lth)

Depends on D28724

Comment 9

[Luke Wagner [:luke]]

Attachment: Bug 1546138 - Baldr: rename TableKind::TypedFunction to AsmJS (r=lth)

Depends on D29298

Comment 10

[Luke Wagner [:luke]]

Attachment: Bug 1546138 - Baldr: do failure checking in masm.wasmCallBulitinInstanceMethod (r=lth)

This patch further centralizes builtin signature information into
SymbolicAddressSignature, removing more than 100 lines of code
and preparing the way for the next patch, which will use this
eager error check to return a JSObject* from Instance::tableGet.

Depends on D29299

Comment 11

[Luke Wagner [:luke]]

Attachment: Bug 1546138 - Baldr: remove indirection from table.get return value (r=lth)

Depends on D29595

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b42fdbc6f6b
Baldr: reflect 'anyfunc' to 'funcref' renaming in methods (r=lth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/54beb5e36110
Baldr: add FuncRef to ValType (r=lth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f78bcde2e81
Baldr: rename TableKind::TypedFunction to AsmJS (r=lth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/094145132b7f
Baldr: do failure checking in masm.wasmCallBulitinInstanceMethod (r=lth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9d1ae270d58
Baldr: remove indirection from table.get return value (r=lth)

Comment 13

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 5 changesets (Bug 1546138) for WasmTypes.h related spidermonkey builds bustages

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&searchStr=linux%2Cx64%2Cdebug%2Cspidermonkey%2Cbuilds%2Cspidermonkey-sm-compacting-linux64%2Fdebug%2Csm%28cgc%29&fromchange=bc57aed491f5ac85108cb7d459894a37db4d0b47&tochange=9ae64983479d4560db4d44719f1154813fdec2e0&selectedJob=244308063

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/9ae64983479d4560db4d44719f1154813fdec2e0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=244308063&repo=mozilla-inbound&lineNumber=43644

[task 2019-05-02T22:13:51.349Z] TEST-PASS | js/src/jit-test/tests/wasm/funcref.js | Success (code 0, args "--baseline-eager") [0.3 s]
[task 2019-05-02T22:13:51.407Z] TEST-PASS | js/src/jit-test/tests/wasm/full-cycle.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off --more-compartments") [0.7 s]
[task 2019-05-02T22:13:51.433Z] TEST-PASS | js/src/jit-test/tests/wasm/globals-impl.js | Success (code 0, args "--baseline-eager") [0.3 s]
[task 2019-05-02T22:13:51.446Z] TEST-PASS | js/src/jit-test/tests/wasm/funcref.js | Success (code 0, args "--test-wasm-await-tier2") [0.3 s]
[task 2019-05-02T22:13:51.467Z] TEST-PASS | js/src/jit-test/tests/wasm/globals-impl.js | Success (code 0, args "") [0.3 s]
[task 2019-05-02T22:13:51.492Z] TEST-PASS | js/src/jit-test/tests/wasm/globals-impl.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off --more-compartments") [0.4 s]
[task 2019-05-02T22:13:51.583Z] TEST-PASS | js/src/jit-test/tests/wasm/globals-impl.js | Success (code 0, args "--wasm-compiler=ion") [0.3 s]
[task 2019-05-02T22:13:51.631Z] TEST-PASS | js/src/jit-test/tests/wasm/globals-impl.js | Success (code 0, args "--test-wasm-await-tier2") [0.3 s]
[task 2019-05-02T22:13:51.642Z] TEST-PASS | js/src/jit-test/tests/wasm/funcref.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off --more-compartments") [0.7 s]
[task 2019-05-02T22:13:51.688Z] TEST-PASS | js/src/jit-test/tests/wasm/globals-impl.js | Success (code 0, args "--wasm-compiler=baseline") [0.3 s]
[task 2019-05-02T22:13:51.709Z] TEST-PASS | js/src/jit-test/tests/wasm/globals.js | Success (code 0, args "") [0.4 s]
[task 2019-05-02T22:13:51.754Z] TEST-PASS | js/src/jit-test/tests/wasm/globals.js | Success (code 0, args "--wasm-compiler=ion") [0.3 s]
[task 2019-05-02T22:13:51.796Z] TEST-PASS | js/src/jit-test/tests/wasm/globals.js | Success (code 0, args "--baseline-eager") [0.4 s]
[task 2019-05-02T22:13:51.819Z] Assertion failure: IsValid(ptc), at /builds/worker/workspace/build/src/js/src/wasm/WasmTypes.h:249
[task 2019-05-02T22:13:51.820Z] Exit code: -11
[task 2019-05-02T22:13:51.820Z] FAIL - wasm/globals.js
[task 2019-05-02T22:13:51.820Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/wasm/globals.js | Assertion failure: IsValid(ptc), at /builds/worker/workspace/build/src/js/src/wasm/WasmTypes.h:249 (code -11, args "--wasm-compiler=baseline") [0.3 s]
[task 2019-05-02T22:13:51.820Z] INFO exit-status : -11
[task 2019-05-02T22:13:51.820Z] INFO timed-out : False
[task 2019-05-02T22:13:51.820Z] INFO stderr 2> Assertion failure: IsValid(ptc), at /builds/worker/workspace/build/src/js/src/wasm/WasmTypes.h:249
[task 2019-05-02T22:13:51.905Z] TEST-PASS | js/src/jit-test/tests/wasm/globals.js | Success (code 0, args "--test-wasm-await-tier2") [0.4 s]
[task 2019-05-02T22:13:51.936Z] Assertion failure: IsValid(ptc), at /builds/worker/workspace/build/src/js/src/wasm/WasmTypes.h:249
[task 2019-05-02T22:13:51.936Z] Exit code: -11
[task 2019-05-02T22:13:51.936Z] FAIL - wasm/globals.js
[task 2019-05-02T22:13:51.936Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/wasm/globals.js | Assertion failure: IsValid(ptc), at /builds/worker/workspace/build/src/js/src/wasm/WasmTypes.h:249 (code -11, args "--ion-eager --ion-offthread-compile=off --more-compartments") [0.5 s]
[task 2019-05-02T22:13:51.936Z] INFO exit-status : -11
[task 2019-05-02T22:13:51.936Z] INFO timed-out : False
[task 2019-05-02T22:13:51.936Z] INFO stderr 2> Assertion failure: IsValid(ptc), at /builds/worker/workspace/build/src/js/src/wasm/WasmTypes.h:249
[task 2019-05-02T22:13:51.969Z] TEST-PASS | js/src/jit-test/tests/wasm/grow-memory.js | Success (code 0, args "") [0.4 s]
[task 2019-05-02T22:13:51.969Z] TEST-PASS | js/src/jit-test/tests/wasm/grow-memory.js | Success (code 0, args "--baseline-eager") [0.3 s]
[task 2019-05-02T22:13:51.976Z] TEST-PASS | js/src/jit-test/tests/wasm/grow-memory.js | Success (code 0, args "--wasm-compiler=ion") [0.3 s]
[task 2019-05-02T22:13:51.986Z] TEST-PASS | js/src/jit-test/tests/wasm/grow-memory.js | Success (code 0, args "--wasm-compiler=baseline") [0.3 s]
[task 2019-05-02T22:13:52.063Z] TEST-PASS | js/src/jit-test/tests/wasm/grow-memory.js | Success (code 0, args "--test-wasm-await-tier2") [0.3 s]
[task 2019-05-02T22:13:52.096Z] TEST-PASS | js/src/jit-test/tests/wasm/import-export-sigs.js | Success (code 0, args "") [0.3 s]
[task 2019-05-02T22:13:52.130Z] TEST-PASS | js/src/jit-test/tests/wasm/grow-memory.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off --more-compartments") [0.5 s]
[task 2019-05-02T22:13:52.235Z] TEST-PASS | js/src/jit-test/tests/wasm/import-export-sigs.js | Success (code 0, args "--baseline-eager") [0.3 s]

Comment 14

[Luke Wagner [:luke]]

D'oh; I accidentally removed the "type_.isValid()" test from Val::trace() so if you GC while a RootedVal is on the stack that hasn't yet been initialized (making it valid), it asserts. Trivial fix.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77be2a536573
Baldr: reflect 'anyfunc' to 'funcref' renaming in methods (r=lth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/da9544b976b1
Baldr: add FuncRef to ValType (r=lth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b60f1ed65b1a
Baldr: rename TableKind::TypedFunction to AsmJS (r=lth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/edf39b4a6ec1
Baldr: do failure checking in masm.wasmCallBulitinInstanceMethod (r=lth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/13e26dbd7cc7
Baldr: remove indirection from table.get return value (r=lth)

Comment 16

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/77be2a536573
https://hg.mozilla.org/mozilla-central/rev/da9544b976b1
https://hg.mozilla.org/mozilla-central/rev/b60f1ed65b1a
https://hg.mozilla.org/mozilla-central/rev/edf39b4a6ec1
https://hg.mozilla.org/mozilla-central/rev/13e26dbd7cc7

Comment 17

[Andrei Ciure[:aciure]]

This is causing spidermonkey bustages on beta-sims:

https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Cpending%2Crunnable&revision=8fd042d9dbeeae91aeb0240ae08c38a19b987e4d&searchStr=spidermonkey

https://treeherder.mozilla.org/#/jobs?repo=try&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Cpending%2Crunnable&revision=3ab554d9b3f18806842e55bcb5a4d537503fca1b&searchStr=spidermonkey

Comment 18

[Cristina Coroiu [:ccoroiu]]

Backed out 5 changesets (bug 1546138) for bustage at js/src/jit-test/tests/wasm/funcref.js for upcoming beta

Backout: https://hg.mozilla.org/integration/autoland/rev/e5562f9f81cac49cef56b436477b001cb027b9b4

Comment 19

[Luke Wagner [:luke]]

D'oh, this was missing a wasmReftypesEnabled() guard in the new test.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/50a4ded4f513
Baldr: reflect 'anyfunc' to 'funcref' renaming in methods (r=lth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c7c24409605
Baldr: add FuncRef to ValType (r=lth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/831cb0513855
Baldr: rename TableKind::TypedFunction to AsmJS (r=lth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9dd53d55f6
Baldr: do failure checking in masm.wasmCallBulitinInstanceMethod (r=lth)
https://hg.mozilla.org/integration/mozilla-inbound/rev/274fb69f7d77
Baldr: remove indirection from table.get return value (r=lth)

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/50a4ded4f513
https://hg.mozilla.org/mozilla-central/rev/5c7c24409605
https://hg.mozilla.org/mozilla-central/rev/831cb0513855
https://hg.mozilla.org/mozilla-central/rev/8a9dd53d55f6
https://hg.mozilla.org/mozilla-central/rev/274fb69f7d77

Comment 22

[Luke Wagner [:luke]]

Attachment: Bug 1546138 - Allow funcref in table operators (r=lth)

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbd86e905a1d
Allow funcref in table operators (r=lth)

Comment 24

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/dbd86e905a1d

Comment 25

[Luke Wagner [:luke]]

(Ryan graciously volunteered to take this on after he finishes bug 1559963 so changing assignee.)

Comment 26

[Ryan Hunt [:rhunt]]

Attachment: Bug 1546138 - Wasm: Implement 'ref.func'. r?bbouvier

This commit implements the 'ref.func' instruction by emitting an instance call
to WasmInstanceObject::getExportedFunction.

The referenced function must be referenced in the start section, an export, or
within an element segment. See [1] for more details.

[1] https://github.com/WebAssembly/reference-types/issues/31

Depends on D40585

Comment 27

[Ryan Hunt [:rhunt]]

Attachment: Bug 1546138 - Wasm: Print assertion message when wasm-testharness assertion fails. r?bbouvier

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa5a02a46c50
Wasm: Print assertion message when wasm-testharness assertion fails. r=bbouvier

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00e5548e9c43
Wasm: Implement 'ref.func'. r=bbouvier

Comment 30

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/fa5a02a46c50
https://hg.mozilla.org/mozilla-central/rev/00e5548e9c43

Comment 31

[Ryan Hunt [:rhunt]]

We should be able to close this now.

Comment 32

[Lars T Hansen [:lth]]

Ryan, can you check out the comment I added to the ref.func patch?

Comment 33

[Ryan Hunt [:rhunt]]

Filed bug 1573815 for this, will try to fix this soon.