1471169 - Implement realm switching for Wasm calls

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Comment 1

[Jan de Mooij [:jandem]]

Attachment: WIP

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.

Comment 2

[Luke Wagner [: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)?

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Implement realm switching for Wasm calls

Quite different from the WIP patch, as discussed, but nicer/simpler.

Comment 4

[Jan de Mooij [:jandem]]

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 5

[Luke Wagner [:luke]]

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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec3598f6f3fe
Implement realm switching for Wasm calls. r=luke

Comment 7

[Stefan Hindli [:stefan_hindli]]

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