Baldr: implement funcref
Categories
(Core :: JavaScript: WebAssembly, enhancement, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox70 | --- | fixed |
People
(Reporter: luke, Assigned: rhunt)
References
Details
Attachments
(9 files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
To complete our implementation of reference-types (bug 1508553). Specifically this means adding:
- the
funcref
type - allow
table.get
onfuncref
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•4 years ago
|
||
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
.)
![]() |
Reporter | |
Comment 2•4 years ago
|
||
![]() |
Reporter | |
Comment 3•4 years ago
|
||
Depends on D28723
![]() |
Reporter | |
Comment 4•4 years ago
|
||
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?
![]() |
Reporter | |
Updated•4 years ago
|
Comment 5•4 years ago
|
||
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•4 years ago
|
||
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.
![]() |
Reporter | |
Comment 7•4 years ago
|
||
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).
![]() |
Reporter | |
Comment 8•4 years ago
|
||
Depends on D28724
![]() |
Reporter | |
Comment 9•4 years ago
|
||
Depends on D29298
![]() |
Reporter | |
Comment 10•4 years ago
|
||
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
![]() |
Reporter | |
Comment 11•4 years ago
|
||
Depends on D29595
![]() |
Reporter | |
Updated•4 years ago
|
Comment 12•4 years ago
|
||
Pushed by lwagner@mozilla.com: 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•4 years ago
|
||
Backed out 5 changesets (Bug 1546138) for WasmTypes.h related spidermonkey builds bustages
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]
![]() |
Reporter | |
Comment 14•4 years ago
|
||
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•4 years ago
|
||
Pushed by lwagner@mozilla.com: 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•4 years ago
|
||
bugherder |
Comment 18•4 years ago
|
||
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
![]() |
Reporter | |
Comment 19•4 years ago
|
||
D'oh, this was missing a wasmReftypesEnabled()
guard in the new test.
Comment 20•4 years ago
|
||
Pushed by lwagner@mozilla.com: 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•4 years ago
|
||
bugherder |
![]() |
Reporter | |
Comment 22•4 years ago
|
||
Comment 23•4 years ago
|
||
Pushed by lwagner@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/dbd86e905a1d Allow funcref in table operators (r=lth)
Comment 24•4 years ago
|
||
bugherder |
Updated•4 years ago
|
Updated•4 years ago
|
![]() |
Reporter | |
Comment 25•4 years ago
|
||
(Ryan graciously volunteered to take this on after he finishes bug 1559963 so changing assignee.)
Assignee | ||
Comment 26•4 years ago
|
||
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
Assignee | ||
Comment 27•4 years ago
|
||
Comment 28•4 years ago
|
||
Pushed by rhunt@eqrion.net: https://hg.mozilla.org/integration/autoland/rev/fa5a02a46c50 Wasm: Print assertion message when wasm-testharness assertion fails. r=bbouvier
Comment 29•4 years ago
|
||
Pushed by rhunt@eqrion.net: https://hg.mozilla.org/integration/autoland/rev/00e5548e9c43 Wasm: Implement 'ref.func'. r=bbouvier
Comment 30•4 years ago
|
||
bugherder |
Assignee | ||
Comment 31•4 years ago
|
||
We should be able to close this now.
Updated•4 years ago
|
Updated•4 years ago
|
Comment 32•4 years ago
|
||
Ryan, can you check out the comment I added to the ref.func patch?
Updated•4 years ago
|
Assignee | ||
Comment 33•4 years ago
|
||
Filed bug 1573815 for this, will try to fix this soon.
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Updated•1 year ago
|
Description
•