[WASM] Wasm interpreter's calling convention doesn't match Wasm JIT's convention.
authortzagallo@apple.com <tzagallo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 15 Feb 2020 02:57:49 +0000 (02:57 +0000)
committertzagallo@apple.com <tzagallo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 15 Feb 2020 02:57:49 +0000 (02:57 +0000)
https://bugs.webkit.org/show_bug.cgi?id=207727

JSTests:

Reviewed by Mark Lam.

* wasm/regress/llint-callee-saves-with-fast-memory.js: Added.
* wasm/regress/llint-callee-saves-without-fast-memory.js: Added.

Source/JavaScriptCore:

Reviewed by Mark Lam.

The Wasm JIT has unusual calling conventions, which were further complicated by the addition
of the interpreter, and the interpreter did not correctly follow these conventions (by incorrectly
saving and restoring the callee save registers used for the memory base and size). Here's a summary
of the calling convention:

- When entering Wasm from JS, the wrapper must:
    - Preserve the base and size when entering LLInt regardless of the mode. (Prior to this
      patch we only preserved the base in Signaling mode)
    - Preserve the memory base in either mode, and the size for BoundsChecking.
- Both tiers must preserve every *other* register they use. e.g. the LLInt must preserve PB
  and wasmInstance, but must *not* preserve memoryBase and memorySize.
- Changes to memoryBase and memorySize are visible to the caller. This means that:
    - Intra-module calls can assume these registers are up-to-date even if the memory was
      resized. The only exception here is if the LLInt calls a signaling JIT, in which case
      the JIT will not update the size register, since it won't be using it.
    - Inter-module and JS calls require the caller to reload these registers. These calls may
      result in memory changes (e.g. the callee may call memory.grow).
    - A Signaling JIT caller must be aware that the LLInt may trash the size register, since
      it always bounds checks.

* llint/WebAssembly.asm:
* wasm/WasmAirIRGenerator.cpp:
(JSC::Wasm::AirIRGenerator::addCall):
* wasm/WasmB3IRGenerator.cpp:
(JSC::Wasm::B3IRGenerator::addCall):
* wasm/WasmCallee.cpp:
(JSC::Wasm::LLIntCallee::calleeSaveRegisters):
* wasm/WasmCallingConvention.h:
* wasm/WasmLLIntPlan.cpp:
(JSC::Wasm::LLIntPlan::didCompleteCompilation):
* wasm/WasmMemoryInformation.cpp:
(JSC::Wasm::PinnedRegisterInfo::get):
(JSC::Wasm::getPinnedRegisters): Deleted.

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

JSTests/ChangeLog
JSTests/wasm/regress/llint-callee-saves-with-fast-memory.js [new file with mode: 0644]
JSTests/wasm/regress/llint-callee-saves-without-fast-memory.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/llint/WebAssembly.asm
Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp
Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp
Source/JavaScriptCore/wasm/WasmCallee.cpp
Source/JavaScriptCore/wasm/WasmCallingConvention.h
Source/JavaScriptCore/wasm/WasmLLIntPlan.cpp
Source/JavaScriptCore/wasm/WasmMemoryInformation.cpp

index 72f141d..90fa995 100644 (file)
@@ -1,3 +1,13 @@
+2020-02-14  Tadeu Zagallo  <tzagallo@apple.com>
+
+        [WASM] Wasm interpreter's calling convention doesn't match Wasm JIT's convention.
+        https://bugs.webkit.org/show_bug.cgi?id=207727
+
+        Reviewed by Mark Lam.
+
+        * wasm/regress/llint-callee-saves-with-fast-memory.js: Added.
+        * wasm/regress/llint-callee-saves-without-fast-memory.js: Added.
+
 2020-02-13  Caio Lima  <ticaiolima@gmail.com>
 
         [ESNext][BigInt] We don't support BigInt literal as PropertyName
diff --git a/JSTests/wasm/regress/llint-callee-saves-with-fast-memory.js b/JSTests/wasm/regress/llint-callee-saves-with-fast-memory.js
new file mode 100644 (file)
index 0000000..e4740c9
--- /dev/null
@@ -0,0 +1,40 @@
+//@ skip if $architecture != "x86-64"
+//@ requireOptions("--useWebAssemblyFastMemory=true")
+// FIXME: Stop skipping when we enable fast memory for iOS. https://bugs.webkit.org/show_bug.cgi?id=170774
+
+import { instantiate } from '../wabt-wrapper.js';
+
+const instance = instantiate(`
+    (module
+
+    (memory 0)
+
+    (func $grow
+        (memory.grow (i32.const 1))
+        (drop)
+    )
+
+    (func $f (param $bail i32)
+        (br_if 0 (local.get $bail))
+        (call $grow)
+        (i32.store (i32.const 42) (i32.const 0))
+    )
+
+    (func (export "main")
+        (local $i i32)
+        (local.set $i (i32.const 100000))
+        (loop $warmup
+            (i32.sub (local.get $i) (i32.const 1))
+            (local.tee $i)
+            (call $f (i32.const 1))
+            (br_if $warmup)
+        )
+        (call $f (i32.const 0))
+    )
+
+    )
+`);
+
+
+// This should not throw an OutOfBounds exception
+instance.exports.main();
diff --git a/JSTests/wasm/regress/llint-callee-saves-without-fast-memory.js b/JSTests/wasm/regress/llint-callee-saves-without-fast-memory.js
new file mode 100644 (file)
index 0000000..1fd9b58
--- /dev/null
@@ -0,0 +1,38 @@
+//@ requireOptions("--useWebAssemblyFastMemory=false")
+
+import { instantiate } from '../wabt-wrapper.js';
+
+const instance = instantiate(`
+    (module
+
+    (memory 0)
+
+    (func $grow
+        (memory.grow (i32.const 1))
+        (drop)
+    )
+
+    (func $f (param $bail i32)
+        (br_if 0 (local.get $bail))
+        (call $grow)
+        (i32.store (i32.const 42) (i32.const 0))
+    )
+
+    (func (export "main")
+        (local $i i32)
+        (local.set $i (i32.const 100000))
+        (loop $warmup
+            (i32.sub (local.get $i) (i32.const 1))
+            (local.tee $i)
+            (call $f (i32.const 1))
+            (br_if $warmup)
+        )
+        (call $f (i32.const 0))
+    )
+
+    )
+`);
+
+
+// This should not throw an OutOfBounds exception
+instance.exports.main();
index 9583cc5..6089d4c 100644 (file)
@@ -1,3 +1,44 @@
+2020-02-14  Tadeu Zagallo  <tzagallo@apple.com> and Michael Saboff  <msaboff@apple.com>
+
+        [WASM] Wasm interpreter's calling convention doesn't match Wasm JIT's convention.
+        https://bugs.webkit.org/show_bug.cgi?id=207727
+
+        Reviewed by Mark Lam.
+
+        The Wasm JIT has unusual calling conventions, which were further complicated by the addition
+        of the interpreter, and the interpreter did not correctly follow these conventions (by incorrectly
+        saving and restoring the callee save registers used for the memory base and size). Here's a summary
+        of the calling convention:
+
+        - When entering Wasm from JS, the wrapper must:
+            - Preserve the base and size when entering LLInt regardless of the mode. (Prior to this
+              patch we only preserved the base in Signaling mode)
+            - Preserve the memory base in either mode, and the size for BoundsChecking.
+        - Both tiers must preserve every *other* register they use. e.g. the LLInt must preserve PB
+          and wasmInstance, but must *not* preserve memoryBase and memorySize.
+        - Changes to memoryBase and memorySize are visible to the caller. This means that:
+            - Intra-module calls can assume these registers are up-to-date even if the memory was
+              resized. The only exception here is if the LLInt calls a signaling JIT, in which case
+              the JIT will not update the size register, since it won't be using it.
+            - Inter-module and JS calls require the caller to reload these registers. These calls may
+              result in memory changes (e.g. the callee may call memory.grow).
+            - A Signaling JIT caller must be aware that the LLInt may trash the size register, since
+              it always bounds checks.
+
+        * llint/WebAssembly.asm:
+        * wasm/WasmAirIRGenerator.cpp:
+        (JSC::Wasm::AirIRGenerator::addCall):
+        * wasm/WasmB3IRGenerator.cpp:
+        (JSC::Wasm::B3IRGenerator::addCall):
+        * wasm/WasmCallee.cpp:
+        (JSC::Wasm::LLIntCallee::calleeSaveRegisters):
+        * wasm/WasmCallingConvention.h:
+        * wasm/WasmLLIntPlan.cpp:
+        (JSC::Wasm::LLIntPlan::didCompleteCompilation):
+        * wasm/WasmMemoryInformation.cpp:
+        (JSC::Wasm::PinnedRegisterInfo::get):
+        (JSC::Wasm::getPinnedRegisters): Deleted.
+
 2020-02-13  Stephan Szabo  <stephan.szabo@sony.com>
 
         [PlayStation] Make special udis86 C file handling only happen for Visual Studio
index 90585b1..0b1153e 100644 (file)
@@ -178,29 +178,28 @@ end
 # Wasm specific helpers
 
 macro preserveCalleeSavesUsedByWasm()
+    # NOTE: We intentionally don't save memoryBase and memorySize here. See the comment
+    # in restoreCalleeSavesUsedByWasm() below for why.
     subp CalleeSaveSpaceStackAligned, sp
     if ARM64 or ARM64E
-        emit "stp x23, x26, [x29, #-16]"
-        emit "stp x19, x22, [x29, #-32]"
+        emit "stp x19, x26, [x29, #-16]"
     elsif X86_64
-        storep memorySize, -0x08[cfr]
-        storep memoryBase, -0x10[cfr]
-        storep PB, -0x18[cfr]
-        storep wasmInstance, -0x20[cfr]
+        storep PB, -0x8[cfr]
+        storep wasmInstance, -0x10[cfr]
     else
         error
     end
 end
 
 macro restoreCalleeSavesUsedByWasm()
+    # NOTE: We intentionally don't restore memoryBase and memorySize here. These are saved
+    # and restored when entering Wasm by the JSToWasm wrapper and changes to them are meant
+    # to be observable within the same Wasm module.
     if ARM64 or ARM64E
-        emit "ldp x23, x26, [x29, #-16]"
-        emit "ldp x19, x22, [x29, #-32]"
+        emit "ldp x19, x26, [x29, #-16]"
     elsif X86_64
-        loadp -0x08[cfr], memorySize
-        loadp -0x10[cfr], memoryBase
-        loadp -0x18[cfr], PB
-        loadp -0x20[cfr], wasmInstance
+        loadp -0x8[cfr], PB
+        loadp -0x10[cfr], wasmInstance
     else
         error
     end
index 27201e8..44c4fb0 100644 (file)
@@ -2173,6 +2173,9 @@ auto AirIRGenerator::addCall(uint32_t functionIndex, const Signature& signature,
         restoreWebAssemblyGlobalState(RestoreCachedStackLimit::Yes, m_info.memory, currentInstance, continuation);
     } else {
         auto* patchpoint = emitCallPatchpoint(m_currentBlock, signature, results, args);
+        // We need to clobber the size register since the LLInt always bounds checks
+        if (m_mode == MemoryMode::Signaling)
+            patchpoint->clobberLate(RegisterSet { PinnedRegisterInfo::get().sizeRegister });
         patchpoint->setGenerator([unlinkedWasmToWasmCalls, functionIndex] (CCallHelpers& jit, const B3::StackmapGenerationParams&) {
             AllowMacroScratchRegisterUsage allowScratch(jit);
             CCallHelpers::Call call = jit.threadSafePatchableNearCall();
index e04c466..43b2c6f 100644 (file)
@@ -1726,6 +1726,9 @@ auto B3IRGenerator::addCall(uint32_t functionIndex, const Signature& signature,
                 patchpoint->effects.writesPinned = true;
                 patchpoint->effects.readsPinned = true;
 
+                // We need to clobber the size register since the LLInt always bounds checks
+                if (m_mode == MemoryMode::Signaling)
+                    patchpoint->clobberLate(RegisterSet { PinnedRegisterInfo::get().sizeRegister });
                 patchpoint->setGenerator([unlinkedWasmToWasmCalls, functionIndex] (CCallHelpers& jit, const B3::StackmapGenerationParams&) {
                     AllowMacroScratchRegisterUsage allowScratch(jit);
                     CCallHelpers::Call call = jit.threadSafePatchableNearCall();
index 0a911b5..3ace8cd 100644 (file)
@@ -94,8 +94,6 @@ RegisterAtOffsetList* LLIntCallee::calleeSaveRegisters()
 #else
 #error Unsupported architecture.
 #endif
-        registers.set(GPRInfo::regCS3); // Memory base
-        registers.set(GPRInfo::regCS4); // Memory size
         ASSERT(registers.numberOfSetRegisters() == numberOfLLIntCalleeSaveRegisters);
         calleeSaveRegisters.construct(WTFMove(registers));
     });
index e7a91c5..49fac74 100644 (file)
@@ -46,7 +46,7 @@
 
 namespace JSC { namespace Wasm {
 
-constexpr unsigned numberOfLLIntCalleeSaveRegisters = 4;
+constexpr unsigned numberOfLLIntCalleeSaveRegisters = 2;
 
 using ArgumentLocation = B3::ValueRep;
 enum class CallRole : uint8_t {
index cac18b6..e7b9d5c 100644 (file)
@@ -146,7 +146,9 @@ void LLIntPlan::didCompleteCompilation(const AbstractLocker& locker)
             SignatureIndex signatureIndex = m_moduleInformation->internalFunctionSignatureIndices[functionIndex];
             const Signature& signature = SignatureInformation::get(signatureIndex);
             CCallHelpers jit;
-            std::unique_ptr<InternalFunction> function = createJSToWasmWrapper(jit, signature, &m_unlinkedWasmToWasmCalls[functionIndex], m_moduleInformation.get(), m_mode, functionIndex);
+            // The LLInt always bounds checks
+            MemoryMode mode = MemoryMode::BoundsChecking;
+            std::unique_ptr<InternalFunction> function = createJSToWasmWrapper(jit, signature, &m_unlinkedWasmToWasmCalls[functionIndex], m_moduleInformation.get(), mode, functionIndex);
 
             LinkBuffer linkBuffer(jit, nullptr, JITCompilationCanFail);
             if (UNLIKELY(linkBuffer.didFailToAllocate())) {
index 7cf594e..8ca6c87 100644 (file)
 
 namespace JSC { namespace Wasm {
 
-static Vector<GPRReg> getPinnedRegisters(unsigned remainingPinnedRegisters)
-{
-    Vector<GPRReg> registers;
-    jsCallingConvention().calleeSaveRegisters.forEach([&] (Reg reg) {
-        if (!reg.isGPR())
-            return;
-        GPRReg gpr = reg.gpr();
-        if (!remainingPinnedRegisters || RegisterSet::stackRegisters().get(reg))
-            return;
-        if (RegisterSet::runtimeTagRegisters().get(reg)) {
-            // Since we don't need to, we currently don't pick from the tag registers to allow
-            // JS->Wasm stubs to freely use these registers.
-            return;
-        }
-        --remainingPinnedRegisters;
-        registers.append(gpr);
-    });
-    return registers;
-}
-
 const PinnedRegisterInfo& PinnedRegisterInfo::get()
 {
     static LazyNeverDestroyed<PinnedRegisterInfo> staticPinnedRegisterInfo;
@@ -63,8 +43,6 @@ const PinnedRegisterInfo& PinnedRegisterInfo::get()
         unsigned numberOfPinnedRegisters = 2;
         if (!Context::useFastTLS())
             ++numberOfPinnedRegisters;
-        Vector<GPRReg> pinnedRegs = getPinnedRegisters(numberOfPinnedRegisters);
-
         GPRReg baseMemoryPointer = GPRInfo::regCS3;
         GPRReg sizeRegister = GPRInfo::regCS4;
         GPRReg wasmContextInstancePointer = InvalidGPRReg;