Diff
Modified: trunk/JSTests/ChangeLog (246371 => 246372)
--- trunk/JSTests/ChangeLog 2019-06-12 20:21:52 UTC (rev 246371)
+++ trunk/JSTests/ChangeLog 2019-06-12 20:31:11 UTC (rev 246372)
@@ -1,3 +1,13 @@
+2019-06-12 Yusuke Suzuki <[email protected]>
+
+ [JSC] Polymorphic call stub's slow path should restore callee saves before performing tail call
+ https://bugs.webkit.org/show_bug.cgi?id=198770
+
+ Reviewed by Saam Barati.
+
+ * stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call.js: Added.
+ (test):
+
2019-06-11 Alexey Shvayka <[email protected]>
JSC should throw if proxy set returns falsish in strict mode context
Added: trunk/JSTests/stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call-apply.js (0 => 246372)
--- trunk/JSTests/stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call-apply.js (rev 0)
+++ trunk/JSTests/stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call-apply.js 2019-06-12 20:31:11 UTC (rev 246372)
@@ -0,0 +1,17 @@
+//@ runDefault("--useConcurrentJIT=0")
+
+noInline(Function.prototype.apply);
+
+function test()
+{
+ for (var i = 0; i < 1e4; ++i) {
+ try {
+ Function.prototype.apply.call(WeakMap.bind());
+ } catch { }
+ try {
+ Function.prototype.apply.call(WeakMap);
+ } catch { }
+ }
+}
+
+test();
Added: trunk/JSTests/stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call-no-builtin.js (0 => 246372)
--- trunk/JSTests/stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call-no-builtin.js (rev 0)
+++ trunk/JSTests/stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call-no-builtin.js 2019-06-12 20:31:11 UTC (rev 246372)
@@ -0,0 +1,27 @@
+//@ runDefault("--useConcurrentJIT=0")
+
+function call(func)
+{
+ "use strict";
+ return func();
+}
+noInline(call);
+
+function test()
+{
+ for (var i = 0; i < 1e4; ++i) {
+ try {
+ call(WeakMap);
+ } catch {
+ }
+ try {
+ call(function () { });
+ } catch {
+ }
+ call((function () { }).bind());
+ call((function () { }).bind());
+ call((function () { }).bind());
+ }
+}
+
+test();
Added: trunk/JSTests/stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call-non-cell.js (0 => 246372)
--- trunk/JSTests/stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call-non-cell.js (rev 0)
+++ trunk/JSTests/stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call-non-cell.js 2019-06-12 20:31:11 UTC (rev 246372)
@@ -0,0 +1,20 @@
+//@ runDefault("--useConcurrentJIT=0")
+
+noInline(Function.prototype.apply);
+
+function test()
+{
+ for (var i = 0; i < 1e4; ++i) {
+ try {
+ Function.prototype.apply.call(WeakMap.bind());
+ } catch { }
+ try {
+ Function.prototype.apply.call(WeakMap);
+ } catch { }
+ }
+ try {
+ Function.prototype.apply.call(42);
+ } catch { }
+}
+
+test();
Added: trunk/JSTests/stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call.js (0 => 246372)
--- trunk/JSTests/stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call.js (rev 0)
+++ trunk/JSTests/stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call.js 2019-06-12 20:31:11 UTC (rev 246372)
@@ -0,0 +1,15 @@
+//@ runDefault("--useConcurrentJIT=0")
+function test()
+{
+ for (var i = 0; i < 100; ++i) {
+ var a = WeakSet.bind();
+ var b = new Proxy(a, a);
+ var c = new Promise(b);
+ var d = WeakSet.bind();
+ var e = new Proxy(d, WeakSet);
+ var f = new Promise(e);
+ }
+}
+
+for (var i = 0; i < 50; ++i)
+ test();
Modified: trunk/Source/_javascript_Core/ChangeLog (246371 => 246372)
--- trunk/Source/_javascript_Core/ChangeLog 2019-06-12 20:21:52 UTC (rev 246371)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-06-12 20:31:11 UTC (rev 246372)
@@ -1,3 +1,48 @@
+2019-06-12 Yusuke Suzuki <[email protected]>
+
+ [JSC] Polymorphic call stub's slow path should restore callee saves before performing tail call
+ https://bugs.webkit.org/show_bug.cgi?id=198770
+
+ Reviewed by Saam Barati.
+
+ Polymorphic call stub is a bit specially patched in JS call site. Typical JS call site for tail calls
+ are the following.
+
+ if (callee == patchableCallee) {
+ restore callee saves for tail call
+ prepare for tail call
+ jump to the target function
+ }
+ restore callee saves for slow path
+ call the slow path function
+
+ And linking patches patchableCallee, target function, and slow path function. But polymorphic call stub
+ patches the above `if` statement with the jump to the stub.
+
+ jump to the polymorphic call stub
+
+ This is because polymorphic call stub wants to use CallFrameShuffler to get scratch registers. As a result,
+ "restore callee saves for tail call" thing needs to be done in the polymorphic call stubs. While it is
+ correctly done for the major cases, we have `slowPath` skips, and that path missed restoring callee saves.
+ This skip happens if the callee is non JSCell or non JS function, so typically, InternalFunction is handled
+ in that path.
+
+ This patch does that skips after restoring callee saves.
+
+ * bytecode/CallLinkInfo.cpp:
+ (JSC::CallLinkInfo::CallLinkInfo):
+ * bytecode/CallLinkInfo.h:
+ (JSC::CallLinkInfo::setUpCall):
+ (JSC::CallLinkInfo::calleeGPR):
+ (JSC::CallLinkInfo::setCalleeGPR): Deleted.
+ * jit/Repatch.cpp:
+ (JSC::revertCall):
+ (JSC::linkVirtualFor):
+ (JSC::linkPolymorphicCall):
+ * jit/Repatch.h:
+ * jit/ThunkGenerators.cpp:
+ (JSC::virtualThunkFor):
+
2019-06-12 Commit Queue <[email protected]>
Unreviewed, rolling out r246322.
Modified: trunk/Source/_javascript_Core/bytecode/CallLinkInfo.cpp (246371 => 246372)
--- trunk/Source/_javascript_Core/bytecode/CallLinkInfo.cpp 2019-06-12 20:21:52 UTC (rev 246371)
+++ trunk/Source/_javascript_Core/bytecode/CallLinkInfo.cpp 2019-06-12 20:31:11 UTC (rev 246372)
@@ -62,7 +62,6 @@
, m_allowStubs(true)
, m_clearedByJettison(false)
, m_callType(None)
- , m_calleeGPR(255)
{
}
Modified: trunk/Source/_javascript_Core/bytecode/CallLinkInfo.h (246371 => 246372)
--- trunk/Source/_javascript_Core/bytecode/CallLinkInfo.h 2019-06-12 20:21:52 UTC (rev 246371)
+++ trunk/Source/_javascript_Core/bytecode/CallLinkInfo.h 2019-06-12 20:31:11 UTC (rev 246372)
@@ -157,7 +157,7 @@
bool isLinked() { return m_stub || m_calleeOrCodeBlock; }
void unlink(VM&);
- void setUpCall(CallType callType, CodeOrigin codeOrigin, unsigned calleeGPR)
+ void setUpCall(CallType callType, CodeOrigin codeOrigin, GPRReg calleeGPR)
{
m_callType = callType;
m_codeOrigin = codeOrigin;
@@ -312,13 +312,8 @@
return OBJECT_OFFSETOF(CallLinkInfo, m_slowPathCount);
}
- void setCalleeGPR(unsigned calleeGPR)
+ GPRReg calleeGPR()
{
- m_calleeGPR = calleeGPR;
- }
-
- unsigned calleeGPR()
- {
return m_calleeGPR;
}
@@ -364,7 +359,7 @@
bool m_allowStubs : 1;
bool m_clearedByJettison : 1;
unsigned m_callType : 4; // CallType
- unsigned m_calleeGPR : 8;
+ GPRReg m_calleeGPR { InvalidGPRReg };
uint32_t m_slowPathCount { 0 };
};
Modified: trunk/Source/_javascript_Core/jit/Repatch.cpp (246371 => 246372)
--- trunk/Source/_javascript_Core/jit/Repatch.cpp 2019-06-12 20:21:52 UTC (rev 246371)
+++ trunk/Source/_javascript_Core/jit/Repatch.cpp 2019-06-12 20:31:11 UTC (rev 246372)
@@ -909,7 +909,7 @@
if (!callLinkInfo.clearedByJettison()) {
MacroAssembler::revertJumpReplacementToBranchPtrWithPatch(
MacroAssembler::startOfBranchPtrWithPatchOnRegister(callLinkInfo.hotPathBegin()),
- static_cast<MacroAssembler::RegisterID>(callLinkInfo.calleeGPR()), 0);
+ callLinkInfo.calleeGPR(), 0);
linkSlowFor(vm, callLinkInfo, codeRef);
MacroAssembler::repatchPointer(callLinkInfo.hotPathBegin(), nullptr);
}
@@ -930,7 +930,7 @@
revertCall(&vm, callLinkInfo, vm.getCTIStub(linkCallThunkGenerator).retagged<JITStubRoutinePtrTag>());
}
-void linkVirtualFor(ExecState* exec, CallLinkInfo& callLinkInfo)
+static void linkVirtualFor(ExecState* exec, CallLinkInfo& callLinkInfo)
{
CallFrame* callerFrame = exec->callerFrame();
VM& vm = callerFrame->vm();
@@ -1035,51 +1035,7 @@
linkVirtualFor(exec, callLinkInfo);
return;
}
-
- GPRReg calleeGPR = static_cast<GPRReg>(callLinkInfo.calleeGPR());
-
- CCallHelpers stubJit(callerCodeBlock);
-
- CCallHelpers::JumpList slowPath;
-
- std::unique_ptr<CallFrameShuffler> frameShuffler;
- if (callLinkInfo.frameShuffleData()) {
- ASSERT(callLinkInfo.isTailCall());
- frameShuffler = std::make_unique<CallFrameShuffler>(stubJit, *callLinkInfo.frameShuffleData());
-#if USE(JSVALUE32_64)
- // We would have already checked that the callee is a cell, and we can
- // use the additional register this buys us.
- frameShuffler->assumeCalleeIsCell();
-#endif
- frameShuffler->lockGPR(calleeGPR);
- }
- GPRReg comparisonValueGPR;
-
- if (isClosureCall) {
- GPRReg scratchGPR;
- if (frameShuffler)
- scratchGPR = frameShuffler->acquireGPR();
- else
- scratchGPR = AssemblyHelpers::selectScratchGPR(calleeGPR);
- // Verify that we have a function and stash the executable in scratchGPR.
-#if USE(JSVALUE64)
- slowPath.append(stubJit.branchIfNotCell(calleeGPR));
-#else
- // We would have already checked that the callee is a cell.
-#endif
-
- // FIXME: We could add a fast path for InternalFunction with closure call.
- slowPath.append(stubJit.branchIfNotFunction(calleeGPR));
-
- stubJit.loadPtr(
- CCallHelpers::Address(calleeGPR, JSFunction::offsetOfExecutable()),
- scratchGPR);
-
- comparisonValueGPR = scratchGPR;
- } else
- comparisonValueGPR = calleeGPR;
-
Vector<int64_t> caseValues(callCases.size());
Vector<CallToCodePtr> calls(callCases.size());
UniqueArray<uint32_t> fastCounts;
@@ -1126,6 +1082,31 @@
caseValues[i] = newCaseValue;
}
+ GPRReg calleeGPR = callLinkInfo.calleeGPR();
+
+ CCallHelpers stubJit(callerCodeBlock);
+
+ std::unique_ptr<CallFrameShuffler> frameShuffler;
+ if (callLinkInfo.frameShuffleData()) {
+ ASSERT(callLinkInfo.isTailCall());
+ frameShuffler = std::make_unique<CallFrameShuffler>(stubJit, *callLinkInfo.frameShuffleData());
+#if USE(JSVALUE32_64)
+ // We would have already checked that the callee is a cell, and we can
+ // use the additional register this buys us.
+ frameShuffler->assumeCalleeIsCell();
+#endif
+ frameShuffler->lockGPR(calleeGPR);
+ }
+
+ GPRReg comparisonValueGPR;
+ if (isClosureCall) {
+ if (frameShuffler)
+ comparisonValueGPR = frameShuffler->acquireGPR();
+ else
+ comparisonValueGPR = AssemblyHelpers::selectScratchGPR(calleeGPR);
+ } else
+ comparisonValueGPR = calleeGPR;
+
GPRReg fastCountsBaseGPR;
if (frameShuffler)
fastCountsBaseGPR = frameShuffler->acquireGPR();
@@ -1134,8 +1115,32 @@
AssemblyHelpers::selectScratchGPR(calleeGPR, comparisonValueGPR, GPRInfo::regT3);
}
stubJit.move(CCallHelpers::TrustedImmPtr(fastCounts.get()), fastCountsBaseGPR);
- if (!frameShuffler && callLinkInfo.isTailCall())
+
+ if (!frameShuffler && callLinkInfo.isTailCall()) {
+ // We strongly assume that calleeGPR is not a callee save register in the slow path.
+ ASSERT(!callerCodeBlock->calleeSaveRegisters()->find(calleeGPR));
stubJit.emitRestoreCalleeSaves();
+ }
+
+ CCallHelpers::JumpList slowPath;
+ if (isClosureCall) {
+ // Verify that we have a function and stash the executable in scratchGPR.
+#if USE(JSVALUE64)
+ if (callLinkInfo.isTailCall())
+ slowPath.append(stubJit.branchIfNotCell(calleeGPR, DoNotHaveTagRegisters));
+ else
+ slowPath.append(stubJit.branchIfNotCell(calleeGPR));
+#else
+ // We would have already checked that the callee is a cell.
+#endif
+ // FIXME: We could add a fast path for InternalFunction with closure call.
+ slowPath.append(stubJit.branchIfNotFunction(calleeGPR));
+
+ stubJit.loadPtr(
+ CCallHelpers::Address(calleeGPR, JSFunction::offsetOfExecutable()),
+ comparisonValueGPR);
+ }
+
BinarySwitch binarySwitch(comparisonValueGPR, caseValues, BinarySwitch::IntPtr);
CCallHelpers::JumpList done;
while (binarySwitch.advance(stubJit)) {
Modified: trunk/Source/_javascript_Core/jit/Repatch.h (246371 => 246372)
--- trunk/Source/_javascript_Core/jit/Repatch.h 2019-06-12 20:21:52 UTC (rev 246371)
+++ trunk/Source/_javascript_Core/jit/Repatch.h 2019-06-12 20:31:11 UTC (rev 246372)
@@ -50,7 +50,6 @@
void linkDirectFor(ExecState*, CallLinkInfo&, CodeBlock*, MacroAssemblerCodePtr<JSEntryPtrTag>);
void linkSlowFor(ExecState*, CallLinkInfo&);
void unlinkFor(VM&, CallLinkInfo&);
-void linkVirtualFor(ExecState*, CallLinkInfo&);
void linkPolymorphicCall(ExecState*, CallLinkInfo&, CallVariant);
void resetGetByID(CodeBlock*, StructureStubInfo&, GetByIDKind);
void resetPutByID(CodeBlock*, StructureStubInfo&);
Modified: trunk/Source/_javascript_Core/jit/ThunkGenerators.cpp (246371 => 246372)
--- trunk/Source/_javascript_Core/jit/ThunkGenerators.cpp 2019-06-12 20:21:52 UTC (rev 246371)
+++ trunk/Source/_javascript_Core/jit/ThunkGenerators.cpp 2019-06-12 20:31:11 UTC (rev 246372)
@@ -183,20 +183,17 @@
// the DFG knows that the value is definitely a cell, or definitely a function.
#if USE(JSVALUE64)
- GPRReg tagMaskRegister = GPRInfo::tagMaskRegister;
if (callLinkInfo.isTailCall()) {
// Tail calls could have clobbered the GPRInfo::tagMaskRegister because they
// restore callee saved registers before getthing here. So, let's materialize
// the TagMask in a temp register and use the temp instead.
- tagMaskRegister = GPRInfo::regT4;
- jit.move(CCallHelpers::TrustedImm64(TagMask), tagMaskRegister);
- }
- slowCase.append(
- jit.branchTest64(CCallHelpers::NonZero, GPRInfo::regT0, tagMaskRegister));
+ slowCase.append(jit.branchIfNotCell(GPRInfo::regT0, DoNotHaveTagRegisters));
+ } else
+ slowCase.append(jit.branchIfNotCell(GPRInfo::regT0));
#else
slowCase.append(jit.branchIfNotCell(GPRInfo::regT1));
#endif
- auto notJSFunction = jit.branchIfNotType(GPRInfo::regT0, JSFunctionType);
+ auto notJSFunction = jit.branchIfNotFunction(GPRInfo::regT0);
// Now we know we have a JSFunction.
