Closed Bug 1471169 Opened Last year Closed Last year

Implement realm switching for Wasm calls

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(2 files)

No description provided.
Attached patch WIPSplinter Review
This seems to work on x64 for imports. If I comment out the realm switching in GenerateImportJitExit, the import test hits an assertion failure so that's good.

Indirect calls are more complicated because in MacroAssembler::wasmCallIndirect we need 2 extra scratch registers I think, one for the cx and one for the new realm.
Attachment #8987790 - Flags: feedback?(luke)
Comment on attachment 8987790 [details] [diff] [review]
WIP

Review of attachment 8987790 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks so much for doing this!

::: js/src/jit-test/tests/wasm/import-export.js
@@ +728,5 @@
>  })();
> +
> +(function testCrossRealmImport() {
> +    var g = newGlobal({sameCompartmentAs: this});
> +    g.evaluate("function f1() { return 123; }");

Could this function also assertCorrectRealm()?

@@ +738,5 @@
> +            (func (result i32) (call $imp1))
> +            (func (result i32) (call $imp2))
> +            (export "f1" 0)
> +            (export "f2" 1))
> +    `)), {a:{f1:g.f1,f2:g.assertCorrectRealm}});

Calling the assertCorrectRealm js::Native directly is great b/c it calls through the Interpreter stub which calls Instance::callImport without having set cx->realm, which allows asserting that cx->realm is already correctly set.  Could you import a same-realm assertCorrectRealm and call it between f1 and f2?

::: js/src/jit-test/tests/wasm/tables.js
@@ +210,5 @@
> +        (module
> +            (import $imp "a" "c" (result i32))
> +            (import "a" "b" (table 3 anyfunc))
> +            (export "tbl" table)
> +            (elem (i32.const 0) $imp))

To assert a bit harder, how about killing `f1` above, and having $imp import g.assertCorrectRealm and then the (elem) section would be changed to:
  (func $f (result i32) (call $imp) (i32.const 123))
  (elem (i32.const 0) $f)

@@ +219,5 @@
> +    var call = new Instance(new Module(wasmTextToBinary(`
> +        (module
> +            (import "a" "b" (table 3 anyfunc))
> +            (type $v2i (func (result i32)))
> +            (func $call (param $i i32) (result i32) (call_indirect $v2i (get_local $i)))

Similar to the other test, could you import a same-realm assertCorrectRealm and call it after the call_indirect?

::: js/src/jit/MacroAssembler.cpp
@@ +3631,5 @@
>          bind(&nonNull);
>  
>          loadWasmPinnedRegsFromTls();
>  
> +        //XXX: need two scratch registers to set cx->realm = tls->realm...

So we have one scratch reg here in WasmTableCallIndexReg (which is used to compute the table element pointer currently held by 'scratch', and dead after), so that could hold 'cx', so we just need something to hold 'realm'.

One idea is to clobber WasmTlsReg with 'realm' and then rematerialize it from 'scratch' after storing 'realm' to 'cx->realm'.

But, at the risk of being decadent, I *think* we can avoid that reload by just giving ourselves a WasmTableCallScratchReg2 that is edx, r12, r7, r11 on x86, x64, arm32 and arm64, resp.

I noticed we also need to restore cx->realm after the call() is done.  The annoying thing here is that we don't currently require the WasmTableCall*Regs to be disjoint from the return register and so ABINonArgReg* use the return register on a few archs (x86, x64) (and purposefully so, b/c ax has a smaller encoding for prologue/epilogue purposes).  I think the fix for this is:
 - update the "These registers must be disjoint from ..." comment to insert ", the ABI return reg"
 - define the WasmTableCall*Regs directly (not through ABINonArgRegs), avoiding ax (which I think is easy in both cases)

::: js/src/wasm/WasmInstance.cpp
@@ +107,5 @@
>  bool
>  Instance::callImport(JSContext* cx, uint32_t funcImportIndex, unsigned argc, const uint64_t* argv,
>                       MutableHandleValue rval)
>  {
> +    MOZ_ASSERT(cx->realm() == realm());

Excellent.  Could you also add this before the final return (just to catch a bug in the callee)?
Attachment #8987790 - Flags: feedback?(luke) → feedback+
Quite different from the WIP patch, as discussed, but nicer/simpler.
Attachment #8988179 - Flags: review?(luke)
Comment on attachment 8988179 [details] [diff] [review]
Implement realm switching for Wasm calls

Review of attachment 8988179 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/wasm/tables.js
@@ +206,5 @@
> +    var g = newGlobal({sameCompartmentAs: this});
> +
> +    var src = `
> +        (module
> +            (import $imp "a" "f1" (result i32))

I'll remove this import (and f1:assertCorrectRealm below), it's unused.

@@ +221,5 @@
> +        (module
> +            (import "a" "t" (table 3 anyfunc))
> +            (import $imp "a" "f1" (result i32))
> +            (type $v2i (func (result i32)))
> +            (func $call (param $i i32) (result i32) (i32.add (call_indirect $v2i (get_local $i)) (call $imp)))

Same here, and (call $imp) should be (current_memory) - forgot to fix the rest of the test when rewriting it..
Comment on attachment 8988179 [details] [diff] [review]
Implement realm switching for Wasm calls

Review of attachment 8988179 [details] [diff] [review]:
-----------------------------------------------------------------

Great job!  Thanks so much for doing this and testing carefully.  It's sad we have to restore cx->realm outside of wasmCall(Import|Indirect), but that was preexisting and maybe we can fix this in the future.
Attachment #8988179 - Flags: review?(luke) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec3598f6f3fe
Implement realm switching for Wasm calls. r=luke
https://hg.mozilla.org/mozilla-central/rev/ec3598f6f3fe
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.