Changeset 246372 in webkit


Ignore:
Timestamp:
Jun 12, 2019 1:31:11 PM (5 years ago)
Author:
ysuzuki@apple.com
Message:

[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.

JSTests:

  • stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call.js: Added.

(test):

Source/JavaScriptCore:

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):

Location:
trunk
Files:
4 added
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r246346 r246372  
     12019-06-12  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        [JSC] Polymorphic call stub's slow path should restore callee saves before performing tail call
     4        https://bugs.webkit.org/show_bug.cgi?id=198770
     5
     6        Reviewed by Saam Barati.
     7
     8        * stress/poly-call-stub-slow-path-should-restore-callee-saves-when-doing-tail-call.js: Added.
     9        (test):
     10
    1112019-06-11  Alexey Shvayka  <shvaikalesh@gmail.com>
    212

  • trunk/Source/JavaScriptCore/ChangeLog

    r246368 r246372  
     12019-06-12  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        [JSC] Polymorphic call stub's slow path should restore callee saves before performing tail call
     4        https://bugs.webkit.org/show_bug.cgi?id=198770
     5
     6        Reviewed by Saam Barati.
     7
     8        Polymorphic call stub is a bit specially patched in JS call site. Typical JS call site for tail calls
     9        are the following.
     10
     11            if (callee == patchableCallee) {
     12                restore callee saves for tail call
     13                prepare for tail call
     14                jump to the target function
     15            }
     16            restore callee saves for slow path
     17            call the slow path function
     18
     19        And linking patches patchableCallee, target function, and slow path function. But polymorphic call stub
     20        patches the above `if` statement with the jump to the stub.
     21
     22            jump to the polymorphic call stub
     23
     24        This is because polymorphic call stub wants to use CallFrameShuffler to get scratch registers. As a result,
     25        "restore callee saves for tail call" thing needs to be done in the polymorphic call stubs. While it is
     26        correctly done for the major cases, we have `slowPath` skips, and that path missed restoring callee saves.
     27        This skip happens if the callee is non JSCell or non JS function, so typically, InternalFunction is handled
     28        in that path.
     29
     30        This patch does that skips after restoring callee saves.
     31
     32        * bytecode/CallLinkInfo.cpp:
     33        (JSC::CallLinkInfo::CallLinkInfo):
     34        * bytecode/CallLinkInfo.h:
     35        (JSC::CallLinkInfo::setUpCall):
     36        (JSC::CallLinkInfo::calleeGPR):
     37        (JSC::CallLinkInfo::setCalleeGPR): Deleted.
     38        * jit/Repatch.cpp:
     39        (JSC::revertCall):
     40        (JSC::linkVirtualFor):
     41        (JSC::linkPolymorphicCall):
     42        * jit/Repatch.h:
     43        * jit/ThunkGenerators.cpp:
     44        (JSC::virtualThunkFor):
     45
    1462019-06-12  Commit Queue  <commit-queue@webkit.org>
    247

  • trunk/Source/JavaScriptCore/bytecode/CallLinkInfo.cpp

    r245239 r246372  
    6363    , m_clearedByJettison(false)
    6464    , m_callType(None)
    65     , m_calleeGPR(255)
    6665{
    6766}

  • trunk/Source/JavaScriptCore/bytecode/CallLinkInfo.h

    r245239 r246372  
    158158    void unlink(VM&);
    159159
    160     void setUpCall(CallType callType, CodeOrigin codeOrigin, unsigned calleeGPR)
     160    void setUpCall(CallType callType, CodeOrigin codeOrigin, GPRReg calleeGPR)
    161161    {
    162162        m_callType = callType;
    … …  
    313313    }
    314314
    315     void setCalleeGPR(unsigned calleeGPR)
    316     {
    317         m_calleeGPR = calleeGPR;
    318     }
    319 
    320     unsigned calleeGPR()
     315    GPRReg calleeGPR()
    321316    {
    322317        return m_calleeGPR;
    … …  
    365360    bool m_clearedByJettison : 1;
    366361    unsigned m_callType : 4; // CallType
    367     unsigned m_calleeGPR : 8;
     362    GPRReg m_calleeGPR { InvalidGPRReg };
    368363    uint32_t m_slowPathCount { 0 };
    369364};

  • trunk/Source/JavaScriptCore/jit/Repatch.cpp

    r245018 r246372  
    910910            MacroAssembler::revertJumpReplacementToBranchPtrWithPatch(
    911911                MacroAssembler::startOfBranchPtrWithPatchOnRegister(callLinkInfo.hotPathBegin()),
    912                 static_cast<MacroAssembler::RegisterID>(callLinkInfo.calleeGPR()), 0);
     912                callLinkInfo.calleeGPR(), 0);
    913913            linkSlowFor(vm, callLinkInfo, codeRef);
    914914            MacroAssembler::repatchPointer(callLinkInfo.hotPathBegin(), nullptr);
    … …  
    931931}
    932932
    933 void linkVirtualFor(ExecState* exec, CallLinkInfo& callLinkInfo)
     933static void linkVirtualFor(ExecState* exec, CallLinkInfo& callLinkInfo)
    934934{
    935935    CallFrame* callerFrame = exec->callerFrame();
    … …  
    10361036        return;
    10371037    }
    1038    
    1039     GPRReg calleeGPR = static_cast<GPRReg>(callLinkInfo.calleeGPR());
    1040    
    1041     CCallHelpers stubJit(callerCodeBlock);
    1042    
    1043     CCallHelpers::JumpList slowPath;
    1044    
    1045     std::unique_ptr<CallFrameShuffler> frameShuffler;
    1046     if (callLinkInfo.frameShuffleData()) {
    1047         ASSERT(callLinkInfo.isTailCall());
    1048         frameShuffler = std::make_unique<CallFrameShuffler>(stubJit, *callLinkInfo.frameShuffleData());
    1049 #if USE(JSVALUE32_64)
    1050         // We would have already checked that the callee is a cell, and we can
    1051         // use the additional register this buys us.
    1052         frameShuffler->assumeCalleeIsCell();
    1053 #endif
    1054         frameShuffler->lockGPR(calleeGPR);
    1055     }
    1056     GPRReg comparisonValueGPR;
    1057    
    1058     if (isClosureCall) {
    1059         GPRReg scratchGPR;
    1060         if (frameShuffler)
    1061             scratchGPR = frameShuffler->acquireGPR();
    1062         else
    1063             scratchGPR = AssemblyHelpers::selectScratchGPR(calleeGPR);
    1064         // Verify that we have a function and stash the executable in scratchGPR.
    1065 
    1066 #if USE(JSVALUE64)
    1067         slowPath.append(stubJit.branchIfNotCell(calleeGPR));
    1068 #else
    1069         // We would have already checked that the callee is a cell.
    1070 #endif
    1071 
    1072         // FIXME: We could add a fast path for InternalFunction with closure call.
    1073         slowPath.append(stubJit.branchIfNotFunction(calleeGPR));
    1074    
    1075         stubJit.loadPtr(
    1076             CCallHelpers::Address(calleeGPR, JSFunction::offsetOfExecutable()),
    1077             scratchGPR);
    1078        
    1079         comparisonValueGPR = scratchGPR;
    1080     } else
    1081         comparisonValueGPR = calleeGPR;
    1082    
     1038
    10831039    Vector<int64_t> caseValues(callCases.size());
    10841040    Vector<CallToCodePtr> calls(callCases.size());
    … …  
    11271083    }
    11281084   
     1085    GPRReg calleeGPR = callLinkInfo.calleeGPR();
     1086
     1087    CCallHelpers stubJit(callerCodeBlock);
     1088
     1089    std::unique_ptr<CallFrameShuffler> frameShuffler;
     1090    if (callLinkInfo.frameShuffleData()) {
     1091        ASSERT(callLinkInfo.isTailCall());
     1092        frameShuffler = std::make_unique<CallFrameShuffler>(stubJit, *callLinkInfo.frameShuffleData());
     1093#if USE(JSVALUE32_64)
     1094        // We would have already checked that the callee is a cell, and we can
     1095        // use the additional register this buys us.
     1096        frameShuffler->assumeCalleeIsCell();
     1097#endif
     1098        frameShuffler->lockGPR(calleeGPR);
     1099    }
     1100
     1101    GPRReg comparisonValueGPR;
     1102    if (isClosureCall) {
     1103        if (frameShuffler)
     1104            comparisonValueGPR = frameShuffler->acquireGPR();
     1105        else
     1106            comparisonValueGPR = AssemblyHelpers::selectScratchGPR(calleeGPR);
     1107    } else
     1108        comparisonValueGPR = calleeGPR;
     1109
    11291110    GPRReg fastCountsBaseGPR;
    11301111    if (frameShuffler)
    … …  
    11351116    }
    11361117    stubJit.move(CCallHelpers::TrustedImmPtr(fastCounts.get()), fastCountsBaseGPR);
    1137     if (!frameShuffler && callLinkInfo.isTailCall())
     1118
     1119    if (!frameShuffler && callLinkInfo.isTailCall()) {
     1120        // We strongly assume that calleeGPR is not a callee save register in the slow path.
     1121        ASSERT(!callerCodeBlock->calleeSaveRegisters()->find(calleeGPR));
    11381122        stubJit.emitRestoreCalleeSaves();
     1123    }
     1124
     1125    CCallHelpers::JumpList slowPath;
     1126    if (isClosureCall) {
     1127        // Verify that we have a function and stash the executable in scratchGPR.
     1128#if USE(JSVALUE64)
     1129        if (callLinkInfo.isTailCall())
     1130            slowPath.append(stubJit.branchIfNotCell(calleeGPR, DoNotHaveTagRegisters));
     1131        else
     1132            slowPath.append(stubJit.branchIfNotCell(calleeGPR));
     1133#else
     1134        // We would have already checked that the callee is a cell.
     1135#endif
     1136        // FIXME: We could add a fast path for InternalFunction with closure call.
     1137        slowPath.append(stubJit.branchIfNotFunction(calleeGPR));
     1138
     1139        stubJit.loadPtr(
     1140            CCallHelpers::Address(calleeGPR, JSFunction::offsetOfExecutable()),
     1141            comparisonValueGPR);
     1142    }
     1143
    11391144    BinarySwitch binarySwitch(comparisonValueGPR, caseValues, BinarySwitch::IntPtr);
    11401145    CCallHelpers::JumpList done;

  • trunk/Source/JavaScriptCore/jit/Repatch.h

    r243886 r246372  
    5151void linkSlowFor(ExecState*, CallLinkInfo&);
    5252void unlinkFor(VM&, CallLinkInfo&);
    53 void linkVirtualFor(ExecState*, CallLinkInfo&);
    5453void linkPolymorphicCall(ExecState*, CallLinkInfo&, CallVariant);
    5554void resetGetByID(CodeBlock*, StructureStubInfo&, GetByIDKind);

  • trunk/Source/JavaScriptCore/jit/ThunkGenerators.cpp

    r245251 r246372  
    184184   
    185185#if USE(JSVALUE64)
    186     GPRReg tagMaskRegister = GPRInfo::tagMaskRegister;
    187186    if (callLinkInfo.isTailCall()) {
    188187        // Tail calls could have clobbered the GPRInfo::tagMaskRegister because they
    189188        // restore callee saved registers before getthing here. So, let's materialize
    190189        // the TagMask in a temp register and use the temp instead.
    191         tagMaskRegister = GPRInfo::regT4;
    192         jit.move(CCallHelpers::TrustedImm64(TagMask), tagMaskRegister);
    193     }
    194     slowCase.append(
    195         jit.branchTest64(CCallHelpers::NonZero, GPRInfo::regT0, tagMaskRegister));
     190        slowCase.append(jit.branchIfNotCell(GPRInfo::regT0, DoNotHaveTagRegisters));
     191    } else
     192        slowCase.append(jit.branchIfNotCell(GPRInfo::regT0));
    196193#else
    197194    slowCase.append(jit.branchIfNotCell(GPRInfo::regT1));
    198195#endif
    199     auto notJSFunction = jit.branchIfNotType(GPRInfo::regT0, JSFunctionType);
     196    auto notJSFunction = jit.branchIfNotFunction(GPRInfo::regT0);
    200197   
    201198    // Now we know we have a JSFunction.
Note: See TracChangeset for help on using the changeset viewer.