Attachment 371935 Details for Bug 198770 – Patch

[patch] Patch

bug-198770-20190612010358.patch (text/plain), 13.48 KB, created by Yusuke Suzuki on 2019-06-12 01:03:59 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-06-12 01:03:59 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();

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 198770: 371914 | 371934 | 371935