Diff
Modified: trunk/JSTests/ChangeLog (225410 => 225411)
--- trunk/JSTests/ChangeLog 2017-12-01 21:58:10 UTC (rev 225410)
+++ trunk/JSTests/ChangeLog 2017-12-01 21:58:36 UTC (rev 225411)
@@ -1,3 +1,15 @@
+2017-12-01 JF Bastien <[email protected]>
+
+ WebAssembly: restore cached stack limit after out-call
+ https://bugs.webkit.org/show_bug.cgi?id=179106
+ <rdar://problem/35337525>
+
+ Reviewed by Saam Barati.
+
+ * wasm/function-tests/double-instance.js: Added.
+ (const.imp.boom):
+ (const.imp.get callAnother):
+
2017-11-30 JF Bastien <[email protected]>
WebAssembly: improve stack trace
Added: trunk/JSTests/wasm/function-tests/double-instance.js (0 => 225411)
--- trunk/JSTests/wasm/function-tests/double-instance.js (rev 0)
+++ trunk/JSTests/wasm/function-tests/double-instance.js 2017-12-01 21:58:36 UTC (rev 225411)
@@ -0,0 +1,64 @@
+import Builder from '../Builder.js'
+import * as assert from '../assert.js'
+
+// The call sequence is as follows:
+//
+// js -js2wasm-> i1.callAnother()
+// -wasm2wasm-> i0.callAnother()
+// -wasm2js-> i1.boom()
+// -calldirect-> i1.doStackCheck()
+// -calldirect-> dummy()
+// -calldirect-> i1.doStackCheck()
+// -calldirect-> dummy()
+//
+// We therefore have i1 indirectly calling into itself, through another
+// instance. When returning its cached stack limit should still be valid, but
+// our implementation used to set it to UINTPTR_MAX causing an erroneous stack
+// check failure at the second doStackCheck() call.
+
+const builder = new Builder()
+ .Type()
+ .End()
+ .Import()
+ .Function("imp", "boom", {params:["i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32"], ret:"i32"})
+ .Function("imp", "callAnother", {params:["i32"], ret:"i32"})
+ .End()
+ .Function().End()
+ .Export()
+ .Function("boom")
+ .Function("callAnother")
+ .End()
+ .Code()
+ .Function("boom", {params:["i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32"], ret:"i32"})
+ /* call doStackCheck */.GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).Call(4)
+ .Return()
+ .End()
+ .Function("callAnother", {params:["i32"], ret:"i32"})
+ /* call imp:callAnother */.GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).Call(1)
+ /* call doStackCheck */.GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).Call(4)
+ .Return()
+ .End()
+ .Function("doStackCheck", {params:["i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32"], ret:"i32"})
+ /* call dummy */.GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).GetLocal(0).Call(5)
+ .Return()
+ .End()
+ .Function("dummy", {params:["i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32", "i32"], ret:"i32"})
+ .GetLocal(0)
+ .Return()
+ .End()
+ .End();
+
+const bin = builder.WebAssembly().get();
+const module = new WebAssembly.Module(bin);
+
+let i1;
+
+const imp = {
+ boom: () => { throw new Error(`This boom should not get called!`); },
+ callAnother: () => { i1.exports["boom"](0xdeadbeef); },
+}
+
+const i0 = new WebAssembly.Instance(module, { imp });
+i1 = new WebAssembly.Instance(module, { imp: i0.exports });
+
+i1.exports["callAnother"](0xc0defefe);
Modified: trunk/Source/_javascript_Core/ChangeLog (225410 => 225411)
--- trunk/Source/_javascript_Core/ChangeLog 2017-12-01 21:58:10 UTC (rev 225410)
+++ trunk/Source/_javascript_Core/ChangeLog 2017-12-01 21:58:36 UTC (rev 225411)
@@ -1,3 +1,57 @@
+2017-12-01 JF Bastien <[email protected]>
+
+ WebAssembly: restore cached stack limit after out-call
+ https://bugs.webkit.org/show_bug.cgi?id=179106
+ <rdar://problem/35337525>
+
+ Reviewed by Saam Barati.
+
+ We cache the stack limit on the Instance so that we can do fast
+ stack checks where required. In regular usage the stack limit
+ never changes because we always run on the same thread, but in
+ rare cases an API user can totally migrate which thread (and
+ therefore stack) is used for execution between WebAssembly
+ traces. For that reason we set the cached stack limit to
+ UINTPTR_MAX on the outgoing Instance when transitioning back into
+ a different Instance. We usually restore the cached stack limit in
+ Context::store, but this wasn't called on all code paths. We had a
+ bug where an Instance calling into itself indirectly would
+ therefore fail to restore its cached stack limit properly.
+
+ This patch therefore restores the cached stack limit after direct
+ calls which could be to imports (both wasm->wasm and
+ wasm->embedder). We have to do all of them because we have no way
+ of knowing what imports will do (they're known at instantiation
+ time, not compilation time, and different instances can have
+ different imports). To make this efficient we also add a pointer
+ to the canonical location of the stack limit (i.e. the extra
+ indirection we're trying to save by caching the stack limit on the
+ Instance in the first place). This is potentially a small perf hit
+ on imported direct calls.
+
+ It's hard to say what the performance cost will be because we
+ haven't seen much code in the wild which does this. We're adding
+ two dependent loads and a store of the loaded value, which is
+ unlikely to get used soon after. It's more code, but on an
+ out-of-order processor it doesn't contribute to the critical path.
+
+ * wasm/WasmB3IRGenerator.cpp:
+ (JSC::Wasm::B3IRGenerator::restoreWebAssemblyGlobalState):
+ (JSC::Wasm::B3IRGenerator::addGrowMemory):
+ (JSC::Wasm::B3IRGenerator::addCall):
+ (JSC::Wasm::B3IRGenerator::addCallIndirect):
+ * wasm/WasmInstance.cpp:
+ (JSC::Wasm::Instance::Instance):
+ (JSC::Wasm::Instance::create):
+ * wasm/WasmInstance.h:
+ (JSC::Wasm::Instance::offsetOfPointerToActualStackLimit):
+ (JSC::Wasm::Instance::cachedStackLimit const):
+ (JSC::Wasm::Instance::setCachedStackLimit):
+ * wasm/js/JSWebAssemblyInstance.cpp:
+ (JSC::JSWebAssemblyInstance::create):
+ * wasm/js/WebAssemblyFunction.cpp:
+ (JSC::callWebAssemblyFunction):
+
2017-11-30 Yusuke Suzuki <[email protected]>
[JSC] Use JSFixedArray for op_new_array_buffer
Modified: trunk/Source/_javascript_Core/wasm/WasmB3IRGenerator.cpp (225410 => 225411)
--- trunk/Source/_javascript_Core/wasm/WasmB3IRGenerator.cpp 2017-12-01 21:58:10 UTC (rev 225410)
+++ trunk/Source/_javascript_Core/wasm/WasmB3IRGenerator.cpp 2017-12-01 21:58:36 UTC (rev 225411)
@@ -246,7 +246,8 @@
int32_t WARN_UNUSED_RETURN fixupPointerPlusOffset(ExpressionType&, uint32_t);
void restoreWasmContextInstance(Procedure&, BasicBlock*, Value*);
- void restoreWebAssemblyGlobalState(const MemoryInformation&, Value* instance, Procedure&, BasicBlock*);
+ enum class RestoreCachedStackLimit { No, Yes };
+ void restoreWebAssemblyGlobalState(RestoreCachedStackLimit, const MemoryInformation&, Value* instance, Procedure&, BasicBlock*);
Origin origin();
@@ -450,10 +451,17 @@
emitTierUpCheck(TierUpCount::functionEntryDecrement(), Origin());
}
-void B3IRGenerator::restoreWebAssemblyGlobalState(const MemoryInformation& memory, Value* instance, Procedure& proc, BasicBlock* block)
+void B3IRGenerator::restoreWebAssemblyGlobalState(RestoreCachedStackLimit restoreCachedStackLimit, const MemoryInformation& memory, Value* instance, Procedure& proc, BasicBlock* block)
{
restoreWasmContextInstance(proc, block, instance);
+ if (restoreCachedStackLimit == RestoreCachedStackLimit::Yes) {
+ // The Instance caches the stack limit, but also knows where its canonical location is.
+ Value* pointerToActualStackLimit = m_currentBlock->appendNew<MemoryValue>(m_proc, Load, pointerType(), origin(), instanceValue(), safeCast<int32_t>(Instance::offsetOfPointerToActualStackLimit()));
+ Value* actualStackLimit = m_currentBlock->appendNew<MemoryValue>(m_proc, Load, pointerType(), origin(), pointerToActualStackLimit);
+ m_currentBlock->appendNew<MemoryValue>(m_proc, Store, origin(), actualStackLimit, instanceValue(), safeCast<int32_t>(Instance::offsetOfCachedStackLimit()));
+ }
+
if (!!memory) {
const PinnedRegisterInfo* pinnedRegs = &PinnedRegisterInfo::get();
RegisterSet clobbers;
@@ -583,7 +591,7 @@
m_currentBlock->appendNew<ConstPtrValue>(m_proc, origin(), bitwise_cast<void*>(growMemory)),
m_currentBlock->appendNew<B3::Value>(m_proc, B3::FramePointer, origin()), instanceValue(), delta);
- restoreWebAssemblyGlobalState(m_info.memory, instanceValue(), m_proc, m_currentBlock);
+ restoreWebAssemblyGlobalState(RestoreCachedStackLimit::No, m_info.memory, instanceValue(), m_proc, m_currentBlock);
return { };
}
@@ -1149,7 +1157,7 @@
}
// The call could have been to another WebAssembly instance, and / or could have modified our Memory.
- restoreWebAssemblyGlobalState(m_info.memory, instanceValue(), m_proc, continuation);
+ restoreWebAssemblyGlobalState(RestoreCachedStackLimit::Yes, m_info.memory, instanceValue(), m_proc, continuation);
} else {
result = wasmCallingConvention().setupCall(m_proc, m_currentBlock, origin(), args, toB3Type(returnType),
[=] (PatchpointValue* patchpoint) {
@@ -1308,7 +1316,7 @@
});
// The call could have been to another WebAssembly instance, and / or could have modified our Memory.
- restoreWebAssemblyGlobalState(m_info.memory, instanceValue(), m_proc, m_currentBlock);
+ restoreWebAssemblyGlobalState(RestoreCachedStackLimit::Yes, m_info.memory, instanceValue(), m_proc, m_currentBlock);
return { };
}
Modified: trunk/Source/_javascript_Core/wasm/WasmInstance.cpp (225410 => 225411)
--- trunk/Source/_javascript_Core/wasm/WasmInstance.cpp 2017-12-01 21:58:10 UTC (rev 225410)
+++ trunk/Source/_javascript_Core/wasm/WasmInstance.cpp 2017-12-01 21:58:36 UTC (rev 225411)
@@ -41,11 +41,12 @@
}
}
-Instance::Instance(Context* context, Ref<Module>&& module, EntryFrame** topEntryFramePointer, StoreTopCallFrameCallback&& storeTopCallFrame)
+Instance::Instance(Context* context, Ref<Module>&& module, EntryFrame** pointerToTopEntryFrame, void** pointerToActualStackLimit, StoreTopCallFrameCallback&& storeTopCallFrame)
: m_context(context)
, m_module(WTFMove(module))
, m_globals(MallocPtr<uint64_t>::malloc(globalMemoryByteSize(m_module.get())))
- , m_topEntryFramePointer(topEntryFramePointer)
+ , m_pointerToTopEntryFrame(pointerToTopEntryFrame)
+ , m_pointerToActualStackLimit(pointerToActualStackLimit)
, m_storeTopCallFrame(WTFMove(storeTopCallFrame))
, m_numImportFunctions(m_module->moduleInformation().importFunctionCount())
{
@@ -53,9 +54,9 @@
new (importFunctionInfo(i)) ImportFunctionInfo();
}
-Ref<Instance> Instance::create(Context* context, Ref<Module>&& module, EntryFrame** topEntryFramePointer, StoreTopCallFrameCallback&& storeTopCallFrame)
+Ref<Instance> Instance::create(Context* context, Ref<Module>&& module, EntryFrame** pointerToTopEntryFrame, void** pointerToActualStackLimit, StoreTopCallFrameCallback&& storeTopCallFrame)
{
- return adoptRef(*new (NotNull, fastMalloc(allocationSize(module->moduleInformation().importFunctionCount()))) Instance(context, WTFMove(module), topEntryFramePointer, WTFMove(storeTopCallFrame)));
+ return adoptRef(*new (NotNull, fastMalloc(allocationSize(module->moduleInformation().importFunctionCount()))) Instance(context, WTFMove(module), pointerToTopEntryFrame, pointerToActualStackLimit, WTFMove(storeTopCallFrame)));
}
Instance::~Instance() { }
Modified: trunk/Source/_javascript_Core/wasm/WasmInstance.h (225410 => 225411)
--- trunk/Source/_javascript_Core/wasm/WasmInstance.h 2017-12-01 21:58:10 UTC (rev 225410)
+++ trunk/Source/_javascript_Core/wasm/WasmInstance.h 2017-12-01 21:58:36 UTC (rev 225411)
@@ -44,7 +44,7 @@
public:
using StoreTopCallFrameCallback = WTF::Function<void(void*)>;
- static Ref<Instance> create(Context*, Ref<Module>&&, EntryFrame** topEntryFramePointer, StoreTopCallFrameCallback&&);
+ static Ref<Instance> create(Context*, Ref<Module>&&, EntryFrame** pointerToTopEntryFrame, void** pointerToActualStackLimit, StoreTopCallFrameCallback&&);
void finalizeCreation(void* owner, Ref<CodeBlock>&& codeBlock)
{
@@ -78,11 +78,20 @@
static ptrdiff_t offsetOfMemory() { return OBJECT_OFFSETOF(Instance, m_memory); }
static ptrdiff_t offsetOfGlobals() { return OBJECT_OFFSETOF(Instance, m_globals); }
static ptrdiff_t offsetOfTable() { return OBJECT_OFFSETOF(Instance, m_table); }
- static ptrdiff_t offsetOfTopEntryFramePointer() { return OBJECT_OFFSETOF(Instance, m_topEntryFramePointer); }
+ static ptrdiff_t offsetOfPointerToTopEntryFrame() { return OBJECT_OFFSETOF(Instance, m_pointerToTopEntryFrame); }
+ static ptrdiff_t offsetOfPointerToActualStackLimit() { return OBJECT_OFFSETOF(Instance, m_pointerToActualStackLimit); }
static ptrdiff_t offsetOfCachedStackLimit() { return OBJECT_OFFSETOF(Instance, m_cachedStackLimit); }
- void* cachedStackLimit() const { return m_cachedStackLimit; }
- void setCachedStackLimit(void* limit) { m_cachedStackLimit = limit; }
+ void* cachedStackLimit() const
+ {
+ ASSERT(*m_pointerToActualStackLimit == m_cachedStackLimit);
+ return m_cachedStackLimit;
+ }
+ void setCachedStackLimit(void* limit)
+ {
+ ASSERT(*m_pointerToActualStackLimit == limit || bitwise_cast<void*>(std::numeric_limits<uintptr_t>::max()) == limit);
+ m_cachedStackLimit = limit;
+ }
// Tail accessors.
static size_t offsetOfTail() { return WTF::roundUpToMultipleOf<sizeof(uint64_t)>(sizeof(Instance)); }
@@ -111,7 +120,7 @@
}
private:
- Instance(Context*, Ref<Module>&&, EntryFrame**, StoreTopCallFrameCallback&&);
+ Instance(Context*, Ref<Module>&&, EntryFrame**, void**, StoreTopCallFrameCallback&&);
static size_t allocationSize(Checked<size_t> numImportFunctions)
{
@@ -125,7 +134,8 @@
RefPtr<Memory> m_memory;
RefPtr<Table> m_table;
MallocPtr<uint64_t> m_globals;
- EntryFrame** m_topEntryFramePointer { nullptr };
+ EntryFrame** m_pointerToTopEntryFrame { nullptr };
+ void** m_pointerToActualStackLimit { nullptr };
void* m_cachedStackLimit { bitwise_cast<void*>(std::numeric_limits<uintptr_t>::max()) };
StoreTopCallFrameCallback m_storeTopCallFrame;
unsigned m_numImportFunctions { 0 };
Modified: trunk/Source/_javascript_Core/wasm/WasmThunks.cpp (225410 => 225411)
--- trunk/Source/_javascript_Core/wasm/WasmThunks.cpp 2017-12-01 21:58:10 UTC (rev 225410)
+++ trunk/Source/_javascript_Core/wasm/WasmThunks.cpp 2017-12-01 21:58:36 UTC (rev 225411)
@@ -47,7 +47,7 @@
// The thing that jumps here must move ExceptionType into the argumentGPR1 before jumping here.
// We're allowed to use temp registers here. We are not allowed to use callee saves.
jit.loadWasmContextInstance(GPRInfo::argumentGPR2);
- jit.loadPtr(CCallHelpers::Address(GPRInfo::argumentGPR2, Instance::offsetOfTopEntryFramePointer()), GPRInfo::argumentGPR0);
+ jit.loadPtr(CCallHelpers::Address(GPRInfo::argumentGPR2, Instance::offsetOfPointerToTopEntryFrame()), GPRInfo::argumentGPR0);
jit.loadPtr(CCallHelpers::Address(GPRInfo::argumentGPR0), GPRInfo::argumentGPR0);
jit.copyCalleeSavesToEntryFrameCalleeSavesBuffer(GPRInfo::argumentGPR0);
jit.move(GPRInfo::callFrameRegister, GPRInfo::argumentGPR0);
Modified: trunk/Source/_javascript_Core/wasm/js/JSWebAssemblyInstance.cpp (225410 => 225411)
--- trunk/Source/_javascript_Core/wasm/js/JSWebAssemblyInstance.cpp 2017-12-01 21:58:10 UTC (rev 225410)
+++ trunk/Source/_javascript_Core/wasm/js/JSWebAssemblyInstance.cpp 2017-12-01 21:58:36 UTC (rev 225411)
@@ -170,7 +170,7 @@
// FIXME: These objects could be pretty big we should try to throw OOM here.
auto* jsInstance = new (NotNull, allocateCell<JSWebAssemblyInstance>(vm.heap)) JSWebAssemblyInstance(vm, instanceStructure,
- Wasm::Instance::create(&vm.wasmContext, WTFMove(module), &vm.topEntryFrame, WTFMove(storeTopCallFrame)));
+ Wasm::Instance::create(&vm.wasmContext, WTFMove(module), &vm.topEntryFrame, vm.addressOfSoftStackLimit(), WTFMove(storeTopCallFrame)));
jsInstance->finishCreation(vm, jsModule, moduleNamespace);
RETURN_IF_EXCEPTION(throwScope, nullptr);
Modified: trunk/Source/_javascript_Core/wasm/js/WebAssemblyFunction.cpp (225410 => 225411)
--- trunk/Source/_javascript_Core/wasm/js/WebAssemblyFunction.cpp 2017-12-01 21:58:10 UTC (rev 225410)
+++ trunk/Source/_javascript_Core/wasm/js/WebAssemblyFunction.cpp 2017-12-01 21:58:36 UTC (rev 225411)
@@ -150,7 +150,9 @@
// This is just for some extra safety instead of leaving a cached
// value in there. If we ever forget to set the value to be a real
// bounds, this will force every stack overflow check to immediately
- // fire.
+ // fire. The stack limit never changes while executing except when
+ // WebAssembly is used through the JSC API: API users can ask the code
+ // to migrate threads.
wasmInstance->setCachedStackLimit(bitwise_cast<void*>(std::numeric_limits<uintptr_t>::max()));
}
vm.wasmContext.store(prevWasmInstance, vm.softStackLimit());
