Status

()

enhancement
P1
normal
ASSIGNED
2 months ago
13 days ago

People

(Reporter: luke, Assigned: luke)

Tracking

(Blocks 1 bug, {leave-open})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

Assignee

Description

2 months ago

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

Assignee

Comment 3

2 months ago

Depends on D28723

Assignee

Comment 4

2 months 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?

Flags: needinfo?(jseward)
Assignee

Updated

2 months ago
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.

Assignee

Comment 7

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

Assignee

Comment 10

2 months 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

Assignee

Updated

2 months ago
Keywords: leave-open

Comment 12

2 months 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)

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

Comment 14

Last month

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)

Comment 15

Last month
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)
Regressions: 1550996

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

Assignee

Comment 19

Last month

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

Flags: needinfo?(luke)

Comment 20

Last month
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 23

22 days ago
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
You need to log in before you can comment on or make changes to this bug.