Changeset 244864 in webkit


Ignore:
Timestamp:
May 1, 2019 7:40:44 PM (5 years ago)
Author:
ysuzuki@apple.com
Message:

[JSC] Inlining Getter/Setter should care availability of ad-hocly constructed frame
​https://bugs.webkit.org/show_bug.cgi?id=197405

Reviewed by Saam Barati.

JSTests:

  • stress/getter-setter-inlining-should-emit-movhint.js: Added.

(foo):
(test):
(i.o.get f):
(i.o.set f):

Source/JavaScriptCore:

When inlining getter and setter calls, we setup a stack frame which does not appear in the bytecode.
Because Inlining can switch on executable, we could have a graph like this.

BB#0

...
30: GetSetter
31: MovHint(loc10)
32: SetLocal(loc10)
33: MovHint(loc9)
34: SetLocal(loc9)
...
37: GetExecutable(@30)
...
41: Switch(@37)

BB#2

42: GetLocal(loc12, bc#7 of caller)
...
--> callee: loc9 and loc10 are arguments of callee.

...
<HERE, exit to callee, loc9 and loc10 are required in the bytecode>

When we prune OSR availability at the beginning of BB#2 (bc#7 in the caller), we prune loc9 and loc10's liveness because the caller does not actually have loc9 and loc10.
However, when we begin executing the callee, we need OSR exit to be aware of where it can recover the arguments to the setter, loc9 and loc10.

This patch inserts MovHint at the beginning of callee for a getter / setter stack frame to make arguments (loc9 and loc10 in the above example) recoverable from OSR exit.
We also move arity fixup DFG nodes from the caller to the callee, since moved arguments are not live in the caller too.

Interestingly, this fix also reveals the existing issue in LiveCatchVariablePreservationPhase. We emitted Flush for |this| of InlineCallFrame blindly if we saw InlineCallFrame
inside a block which is covered by catch handler. But this is wrong because inlined function can finish its execution within the block, and |this| is completely unrelated to
the catch handler if the catch handler is in the outer callee. We already collect all the live locals at the catch handler. And this locals must include arguments too if the
catch handler is in inlined function. So, we should not emit Flush for each |this| of seen InlineCallFrame. This emitted Flush may connect unrelated locals in the catch handler
to the locals that is only defined and used in the inlined function, and it leads to the results like DFG says the local is live while the bytecode says the local is dead.
This results in reading and using garbage in OSR entry because DFG OSR entry needs to fill live DFG values from the stack.

  • dfg/DFGByteCodeParser.cpp:

(JSC::DFG::ByteCodeParser::inlineCall):
(JSC::DFG::ByteCodeParser::handleGetById):
(JSC::DFG::ByteCodeParser::handlePutById):

  • dfg/DFGLiveCatchVariablePreservationPhase.cpp:

(JSC::DFG::LiveCatchVariablePreservationPhase::handleBlockForTryCatch):

Location:
trunk
Files:
1 added
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r244862 r244864  
     12019-05-01  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        [JSC] Inlining Getter/Setter should care availability of ad-hocly constructed frame
     4        https://bugs.webkit.org/show_bug.cgi?id=197405
     5
     6        Reviewed by Saam Barati.
     7
     8        * stress/getter-setter-inlining-should-emit-movhint.js: Added.
     9        (foo):
     10        (test):
     11        (i.o.get f):
     12        (i.o.set f):
     13
    1142019-05-01  Michael Saboff  <msaboff@apple.com>
    215

  • trunk/Source/JavaScriptCore/ChangeLog

    r244862 r244864  
     12019-05-01  Yusuke Suzuki  <ysuzuki@apple.com>
     2
     3        [JSC] Inlining Getter/Setter should care availability of ad-hocly constructed frame
     4        https://bugs.webkit.org/show_bug.cgi?id=197405
     5
     6        Reviewed by Saam Barati.
     7
     8        When inlining getter and setter calls, we setup a stack frame which does not appear in the bytecode.
     9        Because Inlining can switch on executable, we could have a graph like this.
     10
     11        BB#0
     12            ...
     13            30: GetSetter
     14            31: MovHint(loc10)
     15            32: SetLocal(loc10)
     16            33: MovHint(loc9)
     17            34: SetLocal(loc9)
     18            ...
     19            37: GetExecutable(@30)
     20            ...
     21            41: Switch(@37)
     22
     23        BB#2
     24            42: GetLocal(loc12, bc#7 of caller)
     25            ...
     26            --> callee: loc9 and loc10 are arguments of callee.
     27              ...
     28              <HERE, exit to callee, loc9 and loc10 are required in the bytecode>
     29
     30        When we prune OSR availability at the beginning of BB#2 (bc#7 in the caller), we prune loc9 and loc10's liveness because the caller does not actually have loc9 and loc10.
     31        However, when we begin executing the callee, we need OSR exit to be aware of where it can recover the arguments to the setter, loc9 and loc10.
     32
     33        This patch inserts MovHint at the beginning of callee for a getter / setter stack frame to make arguments (loc9 and loc10 in the above example) recoverable from OSR exit.
     34        We also move arity fixup DFG nodes from the caller to the callee, since moved arguments are not live in the caller too.
     35
     36        Interestingly, this fix also reveals the existing issue in LiveCatchVariablePreservationPhase. We emitted Flush for |this| of InlineCallFrame blindly if we saw InlineCallFrame
     37        inside a block which is covered by catch handler. But this is wrong because inlined function can finish its execution within the block, and |this| is completely unrelated to
     38        the catch handler if the catch handler is in the outer callee. We already collect all the live locals at the catch handler. And this locals must include arguments too if the
     39        catch handler is in inlined function. So, we should not emit Flush for each |this| of seen InlineCallFrame. This emitted Flush may connect unrelated locals in the catch handler
     40        to the locals that is only defined and used in the inlined function, and it leads to the results like DFG says the local is live while the bytecode says the local is dead.
     41        This results in reading and using garbage in OSR entry because DFG OSR entry needs to fill live DFG values from the stack.
     42
     43        * dfg/DFGByteCodeParser.cpp:
     44        (JSC::DFG::ByteCodeParser::inlineCall):
     45        (JSC::DFG::ByteCodeParser::handleGetById):
     46        (JSC::DFG::ByteCodeParser::handlePutById):
     47        * dfg/DFGLiveCatchVariablePreservationPhase.cpp:
     48        (JSC::DFG::LiveCatchVariablePreservationPhase::handleBlockForTryCatch):
     49
    1502019-05-01  Michael Saboff  <msaboff@apple.com>
    251

  • trunk/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

    r244811 r244864  
    16091609    }
    16101610
     1611    InlineStackEntry* callerStackTop = m_inlineStackTop;
     1612    InlineStackEntry inlineStackEntry(this, codeBlock, codeBlock, callee.function(), result,
     1613        (VirtualRegister)inlineCallFrameStart, argumentCountIncludingThis, kind, continuationBlock);
     1614
     1615    // This is where the actual inlining really happens.
     1616    unsigned oldIndex = m_currentIndex;
     1617    m_currentIndex = 0;
     1618
     1619    switch (kind) {
     1620    case InlineCallFrame::GetterCall:
     1621    case InlineCallFrame::SetterCall: {
     1622        // When inlining getter and setter calls, we setup a stack frame which does not appear in the bytecode.
     1623        // Because Inlining can switch on executable, we could have a graph like this.
     1624        //
     1625        // BB#0
     1626        //     ...
     1627        //     30: GetSetter
     1628        //     31: MovHint(loc10)
     1629        //     32: SetLocal(loc10)
     1630        //     33: MovHint(loc9)
     1631        //     34: SetLocal(loc9)
     1632        //     ...
     1633        //     37: GetExecutable(@30)
     1634        //     ...
     1635        //     41: Switch(@37)
     1636        //
     1637        // BB#2
     1638        //     42: GetLocal(loc12, bc#7 of caller)
     1639        //     ...
     1640        //     --> callee: loc9 and loc10 are arguments of callee.
     1641        //       ...
     1642        //       <HERE, exit to callee, loc9 and loc10 are required in the bytecode>
     1643        //
     1644        // When we prune OSR availability at the beginning of BB#2 (bc#7 in the caller), we prune loc9 and loc10's liveness because the caller does not actually have loc9 and loc10.
     1645        // However, when we begin executing the callee, we need OSR exit to be aware of where it can recover the arguments to the setter, loc9 and loc10. The MovHints in the inlined
     1646        // callee make it so that if we exit at <HERE>, we can recover loc9 and loc10.
     1647        for (int index = 0; index < argumentCountIncludingThis; ++index) {
     1648            VirtualRegister argumentToGet = callerStackTop->remapOperand(virtualRegisterForArgument(index, registerOffset));
     1649            addToGraph(MovHint, OpInfo(argumentToGet.offset()), getDirect(argumentToGet));
     1650        }
     1651        break;
     1652    }
     1653    default:
     1654        break;
     1655    }
     1656
    16111657    if (arityFixupCount) {
    16121658        // Note: we do arity fixup in two phases:
    16131659        // 1. We get all the values we need and MovHint them to the expected locals.
    1614         // 2. We SetLocal them inside the callee's CodeOrigin. This way, if we exit, the callee's
     1660        // 2. We SetLocal them after that. This way, if we exit, the callee's
    16151661        //    frame is already set up. If any SetLocal exits, we have a valid exit state.
    16161662        //    This is required because if we didn't do this in two phases, we may exit in
    1617         //    the middle of arity fixup from the caller's CodeOrigin. This is unsound because if
    1618         //    we did the SetLocals in the caller's frame, the memcpy may clobber needed parts
    1619         //    of the frame right before exiting. For example, consider if we need to pad two args:
     1663        //    the middle of arity fixup from the callee's CodeOrigin. This is unsound because exited
     1664        //    code does not have arity fixup so that remaining necessary fixups are not executed.
     1665        //    For example, consider if we need to pad two args:
    16201666        //    [arg3][arg2][arg1][arg0]
    16211667        //    [fix ][fix ][arg3][arg2][arg1][arg0]
    16221668        //    We memcpy starting from arg0 in the direction of arg3. If we were to exit at a type check
    1623         //    for arg3's SetLocal in the caller's CodeOrigin, we'd exit with a frame like so:
     1669        //    for arg3's SetLocal in the callee's CodeOrigin, we'd exit with a frame like so:
    16241670        //    [arg3][arg2][arg1][arg2][arg1][arg0]
    1625         //    And the caller would then just end up thinking its argument are:
    1626         //    [arg3][arg2][arg1][arg2]
     1671        //    Since we do not perform arity fixup in the callee, this is the frame used by the callee.
     1672        //    And the callee would then just end up thinking its argument are:
     1673        //    [fix ][fix ][arg3][arg2][arg1][arg0]
    16271674        //    which is incorrect.
    16281675
    … …  
    16431690        if (registerOffsetAfterFixup != registerOffset) {
    16441691            for (int index = 0; index < argumentCountIncludingThis; ++index) {
    1645                 Node* value = get(virtualRegisterForArgument(index, registerOffset));
    1646                 VirtualRegister argumentToSet = m_inlineStackTop->remapOperand(virtualRegisterForArgument(index, registerOffsetAfterFixup));
     1692                VirtualRegister argumentToGet = callerStackTop->remapOperand(virtualRegisterForArgument(index, registerOffset));
     1693                Node* value = getDirect(argumentToGet);
     1694                VirtualRegister argumentToSet = m_inlineStackTop->remapOperand(virtualRegisterForArgument(index));
    16471695                addToGraph(MovHint, OpInfo(argumentToSet.offset()), value);
    16481696                m_setLocalQueue.append(DelayedSetLocal { currentCodeOrigin(), argumentToSet, value, ImmediateNakedSet });
    … …  
    16501698        }
    16511699        for (int index = 0; index < arityFixupCount; ++index) {
    1652             VirtualRegister argumentToSet = m_inlineStackTop->remapOperand(virtualRegisterForArgument(argumentCountIncludingThis + index, registerOffsetAfterFixup));
     1700            VirtualRegister argumentToSet = m_inlineStackTop->remapOperand(virtualRegisterForArgument(argumentCountIncludingThis + index));
    16531701            addToGraph(MovHint, OpInfo(argumentToSet.offset()), undefined);
    16541702            m_setLocalQueue.append(DelayedSetLocal { currentCodeOrigin(), argumentToSet, undefined, ImmediateNakedSet });
    … …  
    16561704
    16571705        // At this point, it's OK to OSR exit because we finished setting up
    1658         // our callee's frame. We emit an ExitOK below from the callee's CodeOrigin.
    1659     }
    1660 
    1661     InlineStackEntry inlineStackEntry(this, codeBlock, codeBlock, callee.function(), result,
    1662         (VirtualRegister)inlineCallFrameStart, argumentCountIncludingThis, kind, continuationBlock);
    1663 
    1664     // This is where the actual inlining really happens.
    1665     unsigned oldIndex = m_currentIndex;
    1666     m_currentIndex = 0;
     1706        // our callee's frame. We emit an ExitOK below.
     1707    }
    16671708
    16681709    // At this point, it's again OK to OSR exit.
    … …  
    43724413    //    since we only really care about 'this' in this case. But we're not going to take that
    43734414    //    shortcut.
    4374     int nextRegister = registerOffset + CallFrame::headerSizeInRegisters;
    4375     set(VirtualRegister(nextRegister++), base, ImmediateNakedSet);
     4415    set(virtualRegisterForArgument(0, registerOffset), base, ImmediateNakedSet);
    43764416
    43774417    // We've set some locals, but they are not user-visible. It's still OK to exit from here.
    … …  
    45564596                VirtualRegister(registerOffset)).toLocal());
    45574597   
    4558         int nextRegister = registerOffset + CallFrame::headerSizeInRegisters;
    4559         set(VirtualRegister(nextRegister++), base, ImmediateNakedSet);
    4560         set(VirtualRegister(nextRegister++), value, ImmediateNakedSet);
     4598        set(virtualRegisterForArgument(0, registerOffset), base, ImmediateNakedSet);
     4599        set(virtualRegisterForArgument(1, registerOffset), value, ImmediateNakedSet);
    45614600
    45624601        // We've set some locals, but they are not user-visible. It's still OK to exit from here.

  • trunk/Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp

    r244324 r244864  
    158158
    159159        Operands<VariableAccessData*> currentBlockAccessData(block->variablesAtTail.numberOfArguments(), block->variablesAtTail.numberOfLocals(), nullptr);
    160         HashSet<InlineCallFrame*> seenInlineCallFrames;
    161160
    162161        auto flushEverything = [&] (NodeOrigin origin, unsigned index) {
    163162            RELEASE_ASSERT(currentExceptionHandler);
    164             auto flush = [&] (VirtualRegister operand, bool alwaysInsert) {
    165                 if ((operand.isLocal() && liveAtCatchHead[operand.toLocal()])
    166                     || operand.isArgument()
    167                     || alwaysInsert) {
     163            auto flush = [&] (VirtualRegister operand) {
     164                if ((operand.isLocal() && liveAtCatchHead[operand.toLocal()]) || operand.isArgument()) {
    168165
    169166                    ASSERT(isValidFlushLocation(block, index, operand));
    … …  
    181178
    182179            for (unsigned local = 0; local < block->variablesAtTail.numberOfLocals(); local++)
    183                 flush(virtualRegisterForLocal(local), false);
    184             for (InlineCallFrame* inlineCallFrame : seenInlineCallFrames)
    185                 flush(VirtualRegister(inlineCallFrame->stackOffset + CallFrame::thisArgumentOffset()), true);
    186             flush(VirtualRegister(CallFrame::thisArgumentOffset()), true);
    187 
    188             seenInlineCallFrames.clear();
     180                flush(virtualRegisterForLocal(local));
     181            flush(VirtualRegister(CallFrame::thisArgumentOffset()));
    189182        };
    190183
    … …  
    200193
    201194            if (currentExceptionHandler && (node->op() == SetLocal || node->op() == SetArgumentDefinitely || node->op() == SetArgumentMaybe)) {
    202                 InlineCallFrame* inlineCallFrame = node->origin.semantic.inlineCallFrame();
    203                 if (inlineCallFrame)
    204                     seenInlineCallFrames.add(inlineCallFrame);
    205195                VirtualRegister operand = node->local();
    206 
    207                 int stackOffset = inlineCallFrame ? inlineCallFrame->stackOffset : 0;
    208                 if ((operand.isLocal() && liveAtCatchHead[operand.toLocal()])
    209                     || operand.isArgument()
    210                     || (operand.offset() == stackOffset + CallFrame::thisArgumentOffset())) {
     196                if ((operand.isLocal() && liveAtCatchHead[operand.toLocal()]) || operand.isArgument()) {
    211197
    212198                    ASSERT(isValidFlushLocation(block, nodeIndex, operand));
Note: See TracChangeset for help on using the changeset viewer.