- Revision
- 260632
- Author
- [email protected]
- Date
- 2020-04-24 02:20:58 -0700 (Fri, 24 Apr 2020)
Log Message
Merge r256665 - [WASM] Wasm interpreter's calling convention doesn't match Wasm JIT's convention.
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/_javascript_Core:
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.
Modified Paths
Added Paths
Diff
Modified: releases/WebKitGTK/webkit-2.28/JSTests/ChangeLog (260631 => 260632)
--- releases/WebKitGTK/webkit-2.28/JSTests/ChangeLog 2020-04-24 09:20:50 UTC (rev 260631)
+++ releases/WebKitGTK/webkit-2.28/JSTests/ChangeLog 2020-04-24 09:20:58 UTC (rev 260632)
@@ -1,3 +1,13 @@
+2020-02-14 Tadeu Zagallo <[email protected]>
+
+ [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-03-19 Tomas Popela <[email protected]>
[JSC][BigEndians] Several JSC stress tests failing
Added: releases/WebKitGTK/webkit-2.28/JSTests/wasm/regress/llint-callee-saves-with-fast-memory.js (0 => 260632)
--- releases/WebKitGTK/webkit-2.28/JSTests/wasm/regress/llint-callee-saves-with-fast-memory.js (rev 0)
+++ releases/WebKitGTK/webkit-2.28/JSTests/wasm/regress/llint-callee-saves-with-fast-memory.js 2020-04-24 09:20:58 UTC (rev 260632)
@@ -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();
Added: releases/WebKitGTK/webkit-2.28/JSTests/wasm/regress/llint-callee-saves-without-fast-memory.js (0 => 260632)
--- releases/WebKitGTK/webkit-2.28/JSTests/wasm/regress/llint-callee-saves-without-fast-memory.js (rev 0)
+++ releases/WebKitGTK/webkit-2.28/JSTests/wasm/regress/llint-callee-saves-without-fast-memory.js 2020-04-24 09:20:58 UTC (rev 260632)
@@ -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();
Modified: releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/ChangeLog (260631 => 260632)
--- releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/ChangeLog 2020-04-24 09:20:50 UTC (rev 260631)
+++ releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/ChangeLog 2020-04-24 09:20:58 UTC (rev 260632)
@@ -1,3 +1,44 @@
+2020-02-14 Tadeu Zagallo <[email protected]> and Michael Saboff <[email protected]>
+
+ [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-03-23 Michael Catanzaro <[email protected]>
REGRESSION(r249808): [GTK] Crash in JSC Config::permanentlyFreeze() on architecture ppc64el
Modified: releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/llint/WebAssembly.asm (260631 => 260632)
--- releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/llint/WebAssembly.asm 2020-04-24 09:20:50 UTC (rev 260631)
+++ releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/llint/WebAssembly.asm 2020-04-24 09:20:58 UTC (rev 260632)
@@ -178,15 +178,14 @@
# 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
@@ -193,14 +192,14 @@
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
Modified: releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/wasm/WasmAirIRGenerator.cpp (260631 => 260632)
--- releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/wasm/WasmAirIRGenerator.cpp 2020-04-24 09:20:50 UTC (rev 260631)
+++ releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/wasm/WasmAirIRGenerator.cpp 2020-04-24 09:20:58 UTC (rev 260632)
@@ -2173,6 +2173,9 @@
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();
Modified: releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/wasm/WasmB3IRGenerator.cpp (260631 => 260632)
--- releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/wasm/WasmB3IRGenerator.cpp 2020-04-24 09:20:50 UTC (rev 260631)
+++ releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/wasm/WasmB3IRGenerator.cpp 2020-04-24 09:20:58 UTC (rev 260632)
@@ -1726,6 +1726,9 @@
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();
Modified: releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/wasm/WasmCallee.cpp (260631 => 260632)
--- releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/wasm/WasmCallee.cpp 2020-04-24 09:20:50 UTC (rev 260631)
+++ releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/wasm/WasmCallee.cpp 2020-04-24 09:20:58 UTC (rev 260632)
@@ -94,8 +94,6 @@
#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));
});
Modified: releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/wasm/WasmCallingConvention.h (260631 => 260632)
--- releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/wasm/WasmCallingConvention.h 2020-04-24 09:20:50 UTC (rev 260631)
+++ releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/wasm/WasmCallingConvention.h 2020-04-24 09:20:58 UTC (rev 260632)
@@ -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 {
Modified: releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/wasm/WasmLLIntPlan.cpp (260631 => 260632)
--- releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/wasm/WasmLLIntPlan.cpp 2020-04-24 09:20:50 UTC (rev 260631)
+++ releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/wasm/WasmLLIntPlan.cpp 2020-04-24 09:20:58 UTC (rev 260632)
@@ -146,7 +146,9 @@
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())) {
Modified: releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/wasm/WasmMemoryInformation.cpp (260631 => 260632)
--- releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/wasm/WasmMemoryInformation.cpp 2020-04-24 09:20:50 UTC (rev 260631)
+++ releases/WebKitGTK/webkit-2.28/Source/_javascript_Core/wasm/WasmMemoryInformation.cpp 2020-04-24 09:20:58 UTC (rev 260632)
@@ -35,26 +35,6 @@
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 @@
unsigned numberOfPinnedRegisters = 2;
if (!Context::useFastTLS())
++numberOfPinnedRegisters;
- Vector<GPRReg> pinnedRegs = getPinnedRegisters(numberOfPinnedRegisters);
-
GPRReg baseMemoryPointer = GPRInfo::regCS3;
GPRReg sizeRegister = GPRInfo::regCS4;
GPRReg wasmContextInstancePointer = InvalidGPRReg;
