Closed Bug 1546138 Opened 1 year ago Closed 10 months ago

Baldr: implement funcref

Categories

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

Product:
Core
Component:
Javascript: WebAssembly
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla70
Tracking Flags:
Tracking Status
firefox70 --- fixed

People

(Reporter: luke, Assigned: rhunt)

References

Details

Attachments

(9 files)

Bug 1546138 - Baldr: reflect 'anyfunc' to 'funcref' renaming in methods (r=lth)
47 bytes, text/x-phabricator-request
Details | Review
Bug 1546138 - Baldr: add FuncRef to ValType (r=lth)
47 bytes, text/x-phabricator-request
Details | Review
Bug 1546138 - Baldr: remove (ref T) special case from Val union (r=lth)
47 bytes, text/x-phabricator-request
Details | Review
Bug 1546138 - Baldr: rename TableKind::TypedFunction to AsmJS (r=lth)
47 bytes, text/x-phabricator-request
Details | Review
Bug 1546138 - Baldr: do failure checking in masm.wasmCallBulitinInstanceMethod (r=lth)
47 bytes, text/x-phabricator-request
Details | Review
Bug 1546138 - Baldr: remove indirection from table.get return value (r=lth)
47 bytes, text/x-phabricator-request
Details | Review
Bug 1546138 - Allow funcref in table operators (r=lth)
47 bytes, text/x-phabricator-request
Details | Review
Bug 1546138 - Wasm: Implement 'ref.func'. r?bbouvier
47 bytes, text/x-phabricator-request
Details | Review
Bug 1546138 - Wasm: Print assertion message when wasm-testharness assertion fails. r?bbouvier
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 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.

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

Depends on D28723

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?

Flags: needinfo?(jseward)
Flags: needinfo?(jseward) → needinfo?(lhansen)

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.

Flags: needinfo?(lhansen)

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.

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

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

Keywords: leave-open
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)

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]

Flags: needinfo?(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.

Flags: needinfo?(luke)
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)

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

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

Flags: needinfo?(luke)
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)
Pushed by lwagner@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbd86e905a1d
Allow funcref in table operators (r=lth)
Assignee: nobody → luke
Priority: -- → P1

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

Assignee: luke → rhunt
Depends on: 1559963

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

Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/fa5a02a46c50
Wasm: Print assertion message when wasm-testharness assertion fails. r=bbouvier
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/00e5548e9c43
Wasm: Implement 'ref.func'. r=bbouvier

We should be able to close this now.

Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
status-firefox70: --- → fixed
Target Milestone: --- → mozilla70

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

Status: RESOLVED → REOPENED
Flags: needinfo?(rhunt)
Resolution: FIXED → ---
Depends on: 1573815

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

Flags: needinfo?(rhunt)
Status: REOPENED → RESOLVED
Closed: 10 months ago → 10 months ago
Resolution: --- → FIXED
Regressions: 1596026
You need to log in before you can comment on or make changes to this bug.