Changeset 249069 in webkit

Timestamp:

Aug 23, 2019 2:33:08 PM (5 years ago)

Author:

Justin Michaud

Message:

[WASM-References] Do not overwrite argument registers in jsCallEntrypoint
​https://bugs.webkit.org/show_bug.cgi?id=200952

Reviewed by Saam Barati.

JSTests:

• wasm/references/func_ref.js:

(assert.throws):

Source/JavaScriptCore:

The c call that we emitted was incorrect. If we had an int argument that was supposed to be placed in GPR0 by this loop,
we would clobber it while making the call (among many other possible registers). To fix this, we just inline the call
to isWebassemblyHostFunction.

• wasm/js/WebAssemblyFunction.cpp:

(JSC::WebAssemblyFunction::jsCallEntrypointSlow):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/wasm/references/func_ref.js (modified) (1 diff)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp (modified) (3 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2019-08-23  Justin Michaud  <justin_michaud@apple.com>
+
+        [WASM-References] Do not overwrite argument registers in jsCallEntrypoint
+        https://bugs.webkit.org/show_bug.cgi?id=200952
+
+        Reviewed by Saam Barati.
+
+        * wasm/references/func_ref.js:
+        (assert.throws):
+
 2019-08-22  Justin Michaud  <justin_michaud@apple.com>
 

trunk/JSTests/wasm/references/func_ref.js

@@ -449 +449 @@
     }
 }
+
+{
+    const $1 = new WebAssembly.Instance(new WebAssembly.Module((new Builder())
+      .Type().End()
+      .Import().End()
+      .Function().End()
+      .Export()
+          .Function("test")
+      .End()
+      .Code()
+        .Function("test", { params: ["i32", "funcref"], ret: "i32" })
+          .GetLocal(0)
+        .End()
+      .End().WebAssembly().get()), { });
+
+    const myfun = makeExportedFunction(1337);
+    assert.eq(myfun(), 1337)
+    assert.eq(42, $1.exports.test(42, myfun))
+    assert.throws(() => $1.exports.test(42, () => 5), Error, "Funcref must be an exported wasm function (evaluating 'func(...args)')")
+}
+
+{
+    const $1 = new WebAssembly.Instance(new WebAssembly.Module((new Builder())
+      .Type().End()
+      .Import().End()
+      .Function().End()
+      .Export()
+          .Function("test")
+      .End()
+      .Code()
+        .Function("test", { params: ["i32", "funcref"], ret: "i32" })
+          .GetLocal(0)
+          .If("i32")
+          .Block("i32", (b) =>
+            b.GetLocal(0)
+            .GetLocal(1)
+            .Call(0)
+          )
+          .Else()
+          .Block("i32", (b) =>
+            b.GetLocal(0)
+          )
+          .End()
+        .End()
+      .End().WebAssembly().get()), { });
+
+    const myfun = makeExportedFunction(1337);
+    function foo(b) {
+        $1.exports.test(b, myfun)
+    }
+    noInline(foo);
+
+    for (let i = 0; i < 100; ++i)
+        foo(0);
+
+    assert.throws(() => $1.exports.test(42, () => 5), Error, "Funcref must be an exported wasm function (evaluating 'func(...args)')")
+    assert.throws(() => $1.exports.test(42, myfun), RangeError, "Maximum call stack size exceeded.")
+    assert.throws(() => foo(1), RangeError, "Maximum call stack size exceeded.")
+}

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-08-23  Justin Michaud  <justin_michaud@apple.com>
+
+        [WASM-References] Do not overwrite argument registers in jsCallEntrypoint
+        https://bugs.webkit.org/show_bug.cgi?id=200952
+
+        Reviewed by Saam Barati.
+
+        The c call that we emitted was incorrect. If we had an int argument that was supposed to be placed in GPR0 by this loop,
+        we would clobber it while making the call (among many other possible registers). To fix this, we just inline the call 
+        to isWebassemblyHostFunction.
+
+        * wasm/js/WebAssemblyFunction.cpp:
+        (JSC::WebAssemblyFunction::jsCallEntrypointSlow):
+
 2019-08-23  Ross Kirsling  <ross.kirsling@sony.com>
 

trunk/Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp

@@ -267 +267 @@
 
     GPRReg scratchGPR = Wasm::wasmCallingConventionAir().prologueScratch(1);
-    GPRReg scratch2GPR = Wasm::wasmCallingConventionAir().prologueScratch(0);
-    jit.loadPtr(vm.addressOfSoftStackLimit(), scratch2GPR);
+    bool stackLimitGPRIsClobbered = false;
+    GPRReg stackLimitGPR = Wasm::wasmCallingConventionAir().prologueScratch(0);
+    jit.loadPtr(vm.addressOfSoftStackLimit(), stackLimitGPR);
 
     CCallHelpers::JumpList slowPath;
     slowPath.append(jit.branchPtr(CCallHelpers::Above, MacroAssembler::stackPointerRegister, GPRInfo::callFrameRegister));
-    slowPath.append(jit.branchPtr(CCallHelpers::Below, MacroAssembler::stackPointerRegister, scratch2GPR));
+    slowPath.append(jit.branchPtr(CCallHelpers::Below, MacroAssembler::stackPointerRegister, stackLimitGPR));
 
     // Ensure:
@@ -311 +312 @@
                 break;
             case Wasm::Funcref: {
-                // FIXME: Emit this inline <https://bugs.webkit.org/show_bug.cgi?id=198506>.
-                bool (*shouldThrow)(Wasm::Instance*, JSValue) = [] (Wasm::Instance* wasmInstance, JSValue arg) -> bool {
-                    JSWebAssemblyInstance* instance = wasmInstance->owner<JSWebAssemblyInstance>();
-                    JSGlobalObject* globalObject = instance->globalObject();
-                    VM& vm = globalObject->vm();
-                    return !isWebAssemblyHostFunction(vm, arg) && !arg.isNull();
-                };
-                jit.move(CCallHelpers::TrustedImmPtr(&instance()->instance()), GPRInfo::argumentGPR0);
-                jit.load64(CCallHelpers::Address(GPRInfo::callFrameRegister, jsOffset), GPRInfo::argumentGPR1);
-                jit.setupArguments<decltype(shouldThrow)>(GPRInfo::argumentGPR0, GPRInfo::argumentGPR1);
-                auto call = jit.call(OperationPtrTag);
-
-                jit.addLinkTask([=] (LinkBuffer& linkBuffer) {
-                    linkBuffer.link(call, FunctionPtr<OperationPtrTag>(shouldThrow));
-                });
-
-                slowPath.append(jit.branchTest32(CCallHelpers::NonZero, GPRInfo::returnValueGPR));
-
+                // Ensure we have a WASM exported function.
+                jit.load64(CCallHelpers::Address(GPRInfo::callFrameRegister, jsOffset), scratchGPR);
+                auto isNull = jit.branchIfNull(scratchGPR);
+                slowPath.append(jit.branchIfNotCell(scratchGPR));
+
+                stackLimitGPRIsClobbered = true;
+                jit.emitLoadStructure(vm, scratchGPR, scratchGPR, stackLimitGPR);
+                jit.loadPtr(CCallHelpers::Address(scratchGPR, Structure::classInfoOffset()), scratchGPR);
+
+                static_assert(std::is_final<WebAssemblyFunction>::value, "We do not check for subtypes below");
+                static_assert(std::is_final<WebAssemblyWrapperFunction>::value, "We do not check for subtypes below");
+
+                auto isWasmFunction = jit.branchPtr(CCallHelpers::Equal, scratchGPR, CCallHelpers::TrustedImmPtr(WebAssemblyFunction::info()));
+                slowPath.append(jit.branchPtr(CCallHelpers::NotEqual, scratchGPR, CCallHelpers::TrustedImmPtr(WebAssemblyWrapperFunction::info())));
+
+                isWasmFunction.link(&jit);
+                isNull.link(&jit);
                 FALLTHROUGH;
             }
@@ -433 +433 @@
         jit.storePtr(scratchGPR, vm.wasmContext.pointerToInstance());
     }
-    // This contains the cached stack limit still.
-    jit.storePtr(scratch2GPR, CCallHelpers::Address(scratchGPR, Wasm::Instance::offsetOfCachedStackLimit()));
+    if (stackLimitGPRIsClobbered)
+        jit.loadPtr(vm.addressOfSoftStackLimit(), stackLimitGPR);
+    jit.storePtr(stackLimitGPR, CCallHelpers::Address(scratchGPR, Wasm::Instance::offsetOfCachedStackLimit()));
 
     if (!!moduleInformation.memory) {
         GPRReg baseMemory = pinnedRegs.baseMemoryPointer;
-        GPRReg scratchOrSize = scratch2GPR;
+        GPRReg scratchOrSize = stackLimitGPR;
         auto mode = instance()->memoryMode();