WebAssembly: Make the Unity AngryBots demo run
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 9 Mar 2017 19:18:34 +0000 (19:18 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 9 Mar 2017 19:18:34 +0000 (19:18 +0000)
https://bugs.webkit.org/show_bug.cgi?id=169268

Reviewed by Keith Miller.

JSTests:

* wasm/function-tests/many-arguments-to-function.js: Added.
(import.Builder.from.string_appeared_here.import.as.assert.from.string_appeared_here.I32Const.0.I32Const.1.I32Const.2.I32Const.3.I32Const.4.I32Const.5.I32Const.6.I32Const.7.I32Const.8.I32Const.9.I32Const.10.I32Const.11.I32Const.12.I32Const.13.I32Const.14.I32Const.15.I32Const.16.I32Const.17.Call.0.Return.End.End.foo):
(i.instance.exports.f0.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.Call.Return.End.End.foo):
(i.instance.exports.f0):
(instance.exports.f0.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.Call.Return.End.End.foo):
(instance.exports.f0):

Source/JavaScriptCore:

This patch fixes three bugs:
1. The WasmBinding code for making a JS call was off
by 1 in its stack layout code.
2. The WasmBinding code had a "<" comparison instead
of a ">=" comparison. This would cause us to calculate
the wrong frame pointer offset.
3. The code to reload wasm state inside B3IRGenerator didn't
properly represent its effects.

* wasm/WasmB3IRGenerator.cpp:
(JSC::Wasm::restoreWebAssemblyGlobalState):
(JSC::Wasm::parseAndCompile):
* wasm/WasmBinding.cpp:
(JSC::Wasm::wasmToJs):
* wasm/js/WebAssemblyInstanceConstructor.cpp:
(JSC::WebAssemblyInstanceConstructor::createInstance):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@213653 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/wasm/function-tests/many-arguments-to-function.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp
Source/JavaScriptCore/wasm/WasmBinding.cpp
Source/JavaScriptCore/wasm/js/WebAssemblyInstanceConstructor.cpp

index 05fa35c..343df94 100644 (file)
@@ -1,3 +1,17 @@
+2017-03-09  Saam Barati  <sbarati@apple.com>
+
+        WebAssembly: Make the Unity AngryBots demo run
+        https://bugs.webkit.org/show_bug.cgi?id=169268
+
+        Reviewed by Keith Miller.
+
+        * wasm/function-tests/many-arguments-to-function.js: Added.
+        (import.Builder.from.string_appeared_here.import.as.assert.from.string_appeared_here.I32Const.0.I32Const.1.I32Const.2.I32Const.3.I32Const.4.I32Const.5.I32Const.6.I32Const.7.I32Const.8.I32Const.9.I32Const.10.I32Const.11.I32Const.12.I32Const.13.I32Const.14.I32Const.15.I32Const.16.I32Const.17.Call.0.Return.End.End.foo):
+        (i.instance.exports.f0.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.F32Const.Call.Return.End.End.foo):
+        (i.instance.exports.f0):
+        (instance.exports.f0.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.GetLocal.Call.Return.End.End.foo):
+        (instance.exports.f0):
+
 2017-03-08  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [GTK] JSC test stress/arity-check-ftl-throw.js.ftl-no-cjit-validate-sampling-profiler crashing on GTK bot
diff --git a/JSTests/wasm/function-tests/many-arguments-to-function.js b/JSTests/wasm/function-tests/many-arguments-to-function.js
new file mode 100644 (file)
index 0000000..b14d08e
--- /dev/null
@@ -0,0 +1,186 @@
+import Builder from '../Builder.js'
+import * as assert from '../assert.js'
+
+{
+    const b = new Builder()
+        .Type().End()
+        .Import().Function("imp", "func", { params: ["i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32"], ret:"i32" }).End()
+        .Function().End()
+        .Export().Function("f0").End()
+        .Code()
+            .Function("f0", { params: [], ret: "i32" })
+                .I32Const(0)
+                .I32Const(1)
+                .I32Const(2)
+                .I32Const(3)
+                .I32Const(4)
+                .I32Const(5)
+                .I32Const(6)
+                .I32Const(7)
+                .I32Const(8)
+                .I32Const(9)
+                .I32Const(10)
+                .I32Const(11)
+                .I32Const(12)
+                .I32Const(13)
+                .I32Const(14)
+                .I32Const(15)
+                .I32Const(16)
+                .I32Const(17)
+                .Call(0)
+                .Return()
+            .End()
+        .End()
+
+    function foo(...args) {
+        for (let i = 0; i < args.length; i++) {
+            if (args[i] !== i)
+                throw new Error("Bad!");
+        }
+    }
+
+    let imp = {imp: {func: foo}}
+    let instance = new WebAssembly.Instance(new WebAssembly.Module(b.WebAssembly().get()), imp);
+    for (let i = 0; i < 100; i++)
+        instance.exports.f0();
+}
+
+{
+    const b = new Builder()
+        .Type().End()
+        .Import().Function("imp", "func", { params: ["f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32"], ret:"f32" }).End()
+        .Function().End()
+        .Export().Function("f0").End()
+        .Code()
+            .Function("f0", { params: [], ret: "f32" })
+                .F32Const(0.5)
+                .F32Const(1.5)
+                .F32Const(2.5)
+                .F32Const(3.5)
+                .F32Const(4.5)
+                .F32Const(5.5)
+                .F32Const(6.5)
+                .F32Const(7.5)
+                .F32Const(8.5)
+                .F32Const(9.5)
+                .F32Const(10.5)
+                .F32Const(11.5)
+                .F32Const(12.5)
+                .F32Const(13.5)
+                .F32Const(14.5)
+                .F32Const(15.5)
+                .F32Const(16.5)
+                .F32Const(17.5)
+                .Call(0)
+                .Return()
+            .End()
+        .End()
+
+    function foo(...args) {
+        for (let i = 0; i < args.length; i++) {
+            if (args[i] !== (i + 0.5))
+                throw new Error("Bad!");
+        }
+    }
+
+    let imp = {imp: {func: foo}}
+    let instance = new WebAssembly.Instance(new WebAssembly.Module(b.WebAssembly().get()), imp);
+    for (let i = 0; i < 100; i++)
+        instance.exports.f0();
+}
+
+{
+    const b = new Builder()
+        .Type().End()
+        .Import().Function("imp", "func", { params: ["f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32"], ret:"f32" }).End()
+        .Function().End()
+        .Export().Function("f0").End()
+        .Code()
+            .Function("f0", { params: ["f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32", "f32"] , ret: "f32" })
+                .GetLocal(0)
+                .GetLocal(1)
+                .GetLocal(2)
+                .GetLocal(3)
+                .GetLocal(4)
+                .GetLocal(5)
+                .GetLocal(6)
+                .GetLocal(7)
+                .GetLocal(8)
+                .GetLocal(9)
+                .GetLocal(10)
+                .GetLocal(11)
+                .GetLocal(12)
+                .GetLocal(13)
+                .GetLocal(14)
+                .GetLocal(15)
+                .GetLocal(16)
+                .GetLocal(17)
+                .Call(0)
+                .Return()
+            .End()
+        .End()
+
+    function foo(...args) {
+        for (let i = 0; i < args.length; i++) {
+            if (args[i] !== i)
+                throw new Error("Bad!");
+        }
+    }
+
+    let imp = {imp: {func: foo}}
+    let instance = new WebAssembly.Instance(new WebAssembly.Module(b.WebAssembly().get()), imp);
+    let arr = [];
+    for (let i = 0; i < 18; i++)
+        arr.push(i);
+    for (let i = 0; i < 100; i++)
+        instance.exports.f0(...arr);
+}
+
+
+{
+    let signature = [];
+    function addType(t, i) {
+        for (let j = 0; j < i; j++) {
+            signature.push(t);
+        }
+    }
+    addType("i32", 16);
+    addType("f32", 16);
+
+    let b = new Builder()
+        .Type().End()
+        .Import().Function("imp", "func", { params: signature, ret:"f32" }).End()
+        .Function().End()
+        .Export().Function("f0").End()
+        .Code()
+            .Function("f0", { params: signature , ret: "f32" });
+    for (let i = 0; i < (16 + 16); i++) {
+        b = b.GetLocal(i);
+    }
+
+    b = b.Call(0).Return().End().End();
+
+    function foo(...args) {
+        if (args.length !== 32)
+            throw new Error("Bad!")
+
+        for (let i = 0; i < 16; i++) {
+            if (args[i] !== i)
+                throw new Error("Bad!");
+            if (args[i + 16] !== (i + 16 + 0.5))
+                throw new Error("Bad!");
+        }
+    }
+
+    let imp = {imp: {func: foo}}
+    let instance = new WebAssembly.Instance(new WebAssembly.Module(b.WebAssembly().get()), imp);
+    let arr = [];
+    for (let i = 0; i < 16; i++)
+        arr.push(i);
+    for (let i = 16; i < 32; i++)
+        arr.push(i + 0.5);
+    for (let i = 0; i < 100; i++)
+        instance.exports.f0(...arr);
+}
+
+
index d9531e9..86db152 100644 (file)
@@ -1,3 +1,27 @@
+2017-03-09  Saam Barati  <sbarati@apple.com>
+
+        WebAssembly: Make the Unity AngryBots demo run
+        https://bugs.webkit.org/show_bug.cgi?id=169268
+
+        Reviewed by Keith Miller.
+
+        This patch fixes three bugs:
+        1. The WasmBinding code for making a JS call was off
+        by 1 in its stack layout code.
+        2. The WasmBinding code had a "<" comparison instead
+        of a ">=" comparison. This would cause us to calculate
+        the wrong frame pointer offset.
+        3. The code to reload wasm state inside B3IRGenerator didn't
+        properly represent its effects.
+
+        * wasm/WasmB3IRGenerator.cpp:
+        (JSC::Wasm::restoreWebAssemblyGlobalState):
+        (JSC::Wasm::parseAndCompile):
+        * wasm/WasmBinding.cpp:
+        (JSC::Wasm::wasmToJs):
+        * wasm/js/WebAssemblyInstanceConstructor.cpp:
+        (JSC::WebAssemblyInstanceConstructor::createInstance):
+
 2017-03-09  Mark Lam  <mark.lam@apple.com>
 
         Make the VM Traps mechanism non-polling for the DFG and FTL.
index f17de92..e6d6951 100644 (file)
@@ -304,8 +304,10 @@ static void restoreWebAssemblyGlobalState(VM& vm, const MemoryInformation& memor
             clobbers.set(info.sizeRegister);
 
         B3::PatchpointValue* patchpoint = block->appendNew<B3::PatchpointValue>(proc, B3::Void, Origin());
-        patchpoint->effects = Effects::none();
-        patchpoint->effects.writesPinned = true;
+        Effects effects = Effects::none();
+        effects.writesPinned = true;
+        effects.reads = B3::HeapRange::top();
+        patchpoint->effects = effects;
         patchpoint->clobber(clobbers);
 
         patchpoint->append(instance, ValueRep::SomeRegister);
@@ -1131,13 +1133,12 @@ Expected<std::unique_ptr<WasmInternalFunction>, String> parseAndCompile(VM& vm,
     WASM_FAIL_IF_HELPER_FAILS(parser.parse());
 
     procedure.resetReachability();
-    validate(procedure, "After parsing:\n");
+    if (!ASSERT_DISABLED)
+        validate(procedure, "After parsing:\n");
 
-    if (verbose)
-        dataLog("Pre SSA: ", procedure);
+    dataLogIf(verbose, "Pre SSA: ", procedure);
     fixSSA(procedure);
-    if (verbose)
-        dataLog("Post SSA: ", procedure);
+    dataLogIf(verbose, "Post SSA: ", procedure);
 
     {
         B3::prepareForGeneration(procedure, optLevel);
index c3ff22f..cbb3f99 100644 (file)
@@ -51,6 +51,8 @@ static void materializeImportJSCell(VM* vm, JIT& jit, unsigned importIndex, GPRR
 
 static MacroAssemblerCodeRef wasmToJs(VM* vm, Bag<CallLinkInfo>& callLinkInfos, SignatureIndex signatureIndex, unsigned importIndex)
 {
+    // FIXME: This function doesn't properly abstract away the calling convention.
+    // It'd be super easy to do so: https://bugs.webkit.org/show_bug.cgi?id=169401
     const WasmCallingConvention& wasmCC = wasmCallingConvention();
     const JSCCallingConvention& jsCC = jscCallingConvention();
     const Signature* signature = SignatureInformation::get(vm, signatureIndex);
@@ -121,9 +123,9 @@ static MacroAssemblerCodeRef wasmToJs(VM* vm, Bag<CallLinkInfo>& callLinkInfos,
 
     // FIXME perform a stack check before updating SP. https://bugs.webkit.org/show_bug.cgi?id=165546
 
-    unsigned numberOfParameters = argCount + 1; // There is a "this" argument.
-    unsigned numberOfRegsForCall = CallFrame::headerSizeInRegisters + numberOfParameters;
-    unsigned numberOfBytesForCall = numberOfRegsForCall * sizeof(Register) - sizeof(CallerFrameAndPC);
+    const unsigned numberOfParameters = argCount + 1; // There is a "this" argument.
+    const unsigned numberOfRegsForCall = CallFrame::headerSizeInRegisters + numberOfParameters;
+    const unsigned numberOfBytesForCall = numberOfRegsForCall * sizeof(Register) - sizeof(CallerFrameAndPC);
     const unsigned stackOffset = WTF::roundUpToMultipleOf(stackAlignmentBytes(), numberOfBytesForCall);
     jit.subPtr(MacroAssembler::TrustedImm32(stackOffset), MacroAssembler::stackPointerRegister);
     JIT::Address calleeFrame = CCallHelpers::Address(MacroAssembler::stackPointerRegister, -static_cast<ptrdiff_t>(sizeof(CallerFrameAndPC)));
@@ -135,7 +137,7 @@ static MacroAssemblerCodeRef wasmToJs(VM* vm, Bag<CallLinkInfo>& callLinkInfos,
         unsigned marshalledGPRs = 0;
         unsigned marshalledFPRs = 0;
         unsigned calleeFrameOffset = CallFrameSlot::firstArgument * static_cast<int>(sizeof(Register));
-        unsigned frOffset = CallFrameSlot::firstArgument * static_cast<int>(sizeof(Register));
+        unsigned frOffset = CallFrame::headerSizeInRegisters * static_cast<int>(sizeof(Register));
         for (unsigned argNum = 0; argNum < argCount; ++argNum) {
             Type argType = signature->argument(argNum);
             switch (argType) {
@@ -190,7 +192,7 @@ static MacroAssemblerCodeRef wasmToJs(VM* vm, Bag<CallLinkInfo>& callLinkInfos,
         unsigned marshalledGPRs = 0;
         unsigned marshalledFPRs = 0;
         unsigned calleeFrameOffset = CallFrameSlot::firstArgument * static_cast<int>(sizeof(Register));
-        unsigned frOffset = CallFrameSlot::firstArgument * static_cast<int>(sizeof(Register));
+        unsigned frOffset = CallFrame::headerSizeInRegisters * static_cast<int>(sizeof(Register));
         for (unsigned argNum = 0; argNum < argCount; ++argNum) {
             Type argType = signature->argument(argNum);
             switch (argType) {
@@ -201,7 +203,7 @@ static MacroAssemblerCodeRef wasmToJs(VM* vm, Bag<CallLinkInfo>& callLinkInfos,
                 RELEASE_ASSERT_NOT_REACHED(); // Handled above.
             case I32:
                 // Skipped: handled above.
-                if (marshalledGPRs < wasmCC.m_gprArgs.size())
+                if (marshalledGPRs >= wasmCC.m_gprArgs.size())
                     frOffset += sizeof(Register);
                 ++marshalledGPRs;
                 calleeFrameOffset += sizeof(Register);
index 43ed028..efc4fc9 100644 (file)
@@ -101,7 +101,6 @@ JSWebAssemblyInstance* WebAssemblyInstanceConstructor::createInstance(ExecState*
     JSWebAssemblyInstance* instance = JSWebAssemblyInstance::create(vm, instanceStructure, jsModule, moduleRecord->getModuleNamespace(exec));
     RETURN_IF_EXCEPTION(throwScope, nullptr);
 
-
     // Let funcs, memories and tables be initially-empty lists of callable JavaScript objects, WebAssembly.Memory objects and WebAssembly.Table objects, respectively.
     // Let imports be an initially-empty list of external values.
     unsigned numImportFunctions = 0;
@@ -145,6 +144,8 @@ JSWebAssemblyInstance* WebAssemblyInstanceConstructor::createInstance(ExecState*
             // Note: done as part of Plan compilation.
             // iv. Append v to funcs.
             // Note: adding the JSCell to the instance list fulfills closure requirements b. above (the WebAssembly.Instance wil be kept alive) and v. below (the JSFunction).
+
+            ASSERT(numImportFunctions == import.kindIndex);
             instance->setImportFunction(vm, cell, numImportFunctions++);
             // v. Append closure to imports.
             break;
@@ -216,6 +217,7 @@ JSWebAssemblyInstance* WebAssemblyInstanceConstructor::createInstance(ExecState*
             if (!value.isNumber())
                 return exception(createJSWebAssemblyLinkError(exec, vm, ASCIILiteral("imported global must be a number")));
             // iii. Append ToWebAssemblyValue(v) to imports.
+            ASSERT(numImportGlobals == import.kindIndex);
             switch (moduleInformation.globals[import.kindIndex].type) {
             case Wasm::I32:
                 instance->setGlobal(numImportGlobals++, value.toInt32(exec));