WebKit Bugzilla
Attachment 371934 Details for
Bug 198770
: [JSC] Polymorphic call stub's slow path should restore callee saves before performing tail call
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
bug-198770-20190612001939.patch (text/plain), 13.48 KB, created by
Yusuke Suzuki
on 2019-06-12 00:19:40 PDT
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Yusuke Suzuki
Created:
2019-06-12 00:19:40 PDT
Size:
13.48 KB
patch
obsolete
>Subversion Revision: 246348 >diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog >index 1c08f6039c11084d1187a15c60f0e13526899bdd..fab603e6f546c11fc753be3ce40085d0104fc498 100644 >--- a/Source/JavaScriptCore/ChangeLog >+++ b/Source/JavaScriptCore/ChangeLog >@@ -1,3 +1,48 @@ >+2019-06-12 Yusuke Suzuki <ysuzuki@apple.com> >+ >+ [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 NOBODY (OOPS!). >+ >+ 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-11 Alexey Shvayka <shvaikalesh@gmail.com> > > JSC should throw if proxy set returns falsish in strict mode context >diff --git a/Source/JavaScriptCore/bytecode/CallLinkInfo.cpp b/Source/JavaScriptCore/bytecode/CallLinkInfo.cpp >index ced9d4d67baddbd47fe0d6d41f46f775d09ca82d..d8dac1585fab2b998c1e5d88752ba12df6dadbde 100644 >--- a/Source/JavaScriptCore/bytecode/CallLinkInfo.cpp >+++ b/Source/JavaScriptCore/bytecode/CallLinkInfo.cpp >@@ -62,7 +62,6 @@ CallLinkInfo::CallLinkInfo() > , m_allowStubs(true) > , m_clearedByJettison(false) > , m_callType(None) >- , m_calleeGPR(255) > { > } > >diff --git a/Source/JavaScriptCore/bytecode/CallLinkInfo.h b/Source/JavaScriptCore/bytecode/CallLinkInfo.h >index fa0a115b5890e5978cc4eb5815cbc274dfffb007..89875da2be2370087dc01e5add63a9e81b10d6ad 100644 >--- a/Source/JavaScriptCore/bytecode/CallLinkInfo.h >+++ b/Source/JavaScriptCore/bytecode/CallLinkInfo.h >@@ -157,7 +157,7 @@ class CallLinkInfo : public PackedRawSentinelNode<CallLinkInfo> { > 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,12 +312,7 @@ class CallLinkInfo : public PackedRawSentinelNode<CallLinkInfo> { > return OBJECT_OFFSETOF(CallLinkInfo, m_slowPathCount); > } > >- void setCalleeGPR(unsigned calleeGPR) >- { >- m_calleeGPR = calleeGPR; >- } >- >- unsigned calleeGPR() >+ GPRReg calleeGPR() > { > return m_calleeGPR; > } >@@ -364,7 +359,7 @@ class CallLinkInfo : public PackedRawSentinelNode<CallLinkInfo> { > 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 }; > }; > >diff --git a/Source/JavaScriptCore/jit/Repatch.cpp b/Source/JavaScriptCore/jit/Repatch.cpp >index f07d325aaa17fc91ff8c8c23643e7329863ee836..19e5c04ab02304e23530d61cd07ce0ef424fb57a 100644 >--- a/Source/JavaScriptCore/jit/Repatch.cpp >+++ b/Source/JavaScriptCore/jit/Repatch.cpp >@@ -909,7 +909,7 @@ static void revertCall(VM* vm, CallLinkInfo& callLinkInfo, MacroAssemblerCodeRef > 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 @@ void unlinkFor(VM& vm, CallLinkInfo& callLinkInfo) > 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 @@ void linkPolymorphicCall( > 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 @@ void linkPolymorphicCall( > 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 @@ void linkPolymorphicCall( > 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)) { >diff --git a/Source/JavaScriptCore/jit/Repatch.h b/Source/JavaScriptCore/jit/Repatch.h >index 86a6c5a3dfe31bed82c4e950f3dd06fa14236413..02db1202bebbde4dd5b5e9780875b3890d7961ff 100644 >--- a/Source/JavaScriptCore/jit/Repatch.h >+++ b/Source/JavaScriptCore/jit/Repatch.h >@@ -50,7 +50,6 @@ void linkFor(ExecState*, CallLinkInfo&, CodeBlock*, JSObject* callee, MacroAssem > 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&); >diff --git a/Source/JavaScriptCore/jit/ThunkGenerators.cpp b/Source/JavaScriptCore/jit/ThunkGenerators.cpp >index e84f00447771ca30ff2272349c4e07e7aaa3de0b..cb9bab43a016ce90bb7c6427b22b5362bda0b5ea 100644 >--- a/Source/JavaScriptCore/jit/ThunkGenerators.cpp >+++ b/Source/JavaScriptCore/jit/ThunkGenerators.cpp >@@ -183,20 +183,17 @@ MacroAssemblerCodeRef<JITStubRoutinePtrTag> virtualThunkFor(VM* vm, CallLinkInfo > // 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. > >diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog >index 2ef7cb96d22a747ebe2168ebb62908bd7240dbdd..39cd649e0fbb20fb7f1e8c2e8f5a7fe881fc151c 100644 >--- a/JSTests/ChangeLog >+++ b/JSTests/ChangeLog >@@ -1,3 +1,13 @@ >+2019-06-12 Yusuke Suzuki <ysuzuki@apple.com> >+ >+ [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 NOBODY (OOPS!). >+ >+ * stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call.js: Added. >+ (test): >+ > 2019-06-11 Alexey Shvayka <shvaikalesh@gmail.com> > > JSC should throw if proxy set returns falsish in strict mode context >diff --git a/JSTests/stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call.js b/JSTests/stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call.js >new file mode 100644 >index 0000000000000000000000000000000000000000..3881f1c83f193883aee8d9ee1a2e9c83dcbadebd >--- /dev/null >+++ b/JSTests/stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call.js >@@ -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();
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 198770
:
371914
| 371934 |
371935