Changeset 244324 in webkit
- Timestamp:
- Apr 15, 2019 7:41:38 PM (5 years ago)
- Location:
- trunk
- Files:
-
- 1 added
- 23 edited
Legend:
- Unmodified
- Added
- Removed
-
trunk/JSTests/ChangeLog
r244314 r244324 1 2019-04-15 Saam barati <sbarati@apple.com> 2 3 Modify how we do SetArgument when we inline varargs calls 4 https://bugs.webkit.org/show_bug.cgi?id=196712 5 <rdar://problem/49605012> 6 7 Reviewed by Michael Saboff. 8 9 * stress/get-stack-wrong-type-when-inline-varargs.js: Added. 10 (foo): 11 1 12 2019-04-15 Saam barati <sbarati@apple.com> 2 13 -
trunk/Source/JavaScriptCore/ChangeLog
r244323 r244324 1 2019-04-15 Saam barati <sbarati@apple.com> 2 3 Modify how we do SetArgument when we inline varargs calls 4 https://bugs.webkit.org/show_bug.cgi?id=196712 5 <rdar://problem/49605012> 6 7 Reviewed by Michael Saboff. 8 9 When we inline varargs calls, we guarantee that the number of arguments that 10 go on the stack are somewhere between the "mandatoryMinimum" and the "limit - 1". 11 However, we can't statically guarantee that the arguments between these two 12 ranges was filled out by Load/ForwardVarargs. This is because in the general 13 case we don't know the argument count statically. 14 15 However, we used to always emit SetArgumentDefinitely up to "limit - 1" for 16 all arguments, even when some arguments aren't guaranteed to be in a valid 17 state. Emitting these SetArgumentDefinitely were helpful because they let us 18 handle variable liveness and OSR exit metadata. However, when we converted 19 to SSA, we ended up emitting a GetStack for each such SetArgumentDefinitely. 20 21 This is wrong, as we can't guarantee such SetArgumentDefinitely nodes are 22 actually looking at a range of the stack that are guaranteed to be initialized. 23 This patch introduces a new form of SetArgument node: SetArgumentMaybe. In terms 24 of OSR exit metadata and variable liveness tracking, it behaves like SetArgumentDefinitely. 25 26 However, it differs in a couple key ways: 27 1. In ThreadedCPS, GetLocal(@SetArgumentMaybe) is invalid IR, as this implies 28 you might be loading uninitialized stack. (This same rule applies when you do 29 the full data flow reachability analysis over CPS Phis.) If someone logically 30 wanted to emit code like this, the correct node to emit would be GetArgument, 31 not GetLocal. For similar reasons, PhantomLocal(@SetArgumentMaybe) is also 32 invalid IR. 33 2. To track liveness, Flush(@SetArgumentMaybe) is valid, and is the main user 34 of SetArgumentMaybe. 35 3. In SSA conversion, we don't lower SetArgumentMaybe to GetStack, as there 36 should be no data flow user of SetArgumentMaybe. 37 38 SetArgumentDefinitely guarantees that the stack slot is initialized. 39 SetArgumentMaybe makes no such guarantee. 40 41 * dfg/DFGAbstractInterpreterInlines.h: 42 (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects): 43 * dfg/DFGByteCodeParser.cpp: 44 (JSC::DFG::ByteCodeParser::handleVarargsInlining): 45 * dfg/DFGCPSRethreadingPhase.cpp: 46 (JSC::DFG::CPSRethreadingPhase::freeUnnecessaryNodes): 47 (JSC::DFG::CPSRethreadingPhase::canonicalizeGetLocalFor): 48 (JSC::DFG::CPSRethreadingPhase::canonicalizeFlushOrPhantomLocalFor): 49 (JSC::DFG::CPSRethreadingPhase::canonicalizeLocalsInBlock): 50 (JSC::DFG::CPSRethreadingPhase::propagatePhis): 51 (JSC::DFG::CPSRethreadingPhase::computeIsFlushed): 52 * dfg/DFGClobberize.h: 53 (JSC::DFG::clobberize): 54 * dfg/DFGCommon.h: 55 * dfg/DFGDoesGC.cpp: 56 (JSC::DFG::doesGC): 57 * dfg/DFGFixupPhase.cpp: 58 (JSC::DFG::FixupPhase::fixupNode): 59 * dfg/DFGInPlaceAbstractState.cpp: 60 (JSC::DFG::InPlaceAbstractState::endBasicBlock): 61 * dfg/DFGLiveCatchVariablePreservationPhase.cpp: 62 (JSC::DFG::LiveCatchVariablePreservationPhase::handleBlockForTryCatch): 63 * dfg/DFGMaximalFlushInsertionPhase.cpp: 64 (JSC::DFG::MaximalFlushInsertionPhase::treatRegularBlock): 65 (JSC::DFG::MaximalFlushInsertionPhase::treatRootBlock): 66 * dfg/DFGMayExit.cpp: 67 * dfg/DFGNode.cpp: 68 (JSC::DFG::Node::hasVariableAccessData): 69 * dfg/DFGNodeType.h: 70 * dfg/DFGPhantomInsertionPhase.cpp: 71 * dfg/DFGPredictionPropagationPhase.cpp: 72 * dfg/DFGSSAConversionPhase.cpp: 73 (JSC::DFG::SSAConversionPhase::run): 74 * dfg/DFGSafeToExecute.h: 75 (JSC::DFG::safeToExecute): 76 * dfg/DFGSpeculativeJIT32_64.cpp: 77 (JSC::DFG::SpeculativeJIT::compile): 78 * dfg/DFGSpeculativeJIT64.cpp: 79 (JSC::DFG::SpeculativeJIT::compile): 80 * dfg/DFGValidate.cpp: 81 * ftl/FTLCapabilities.cpp: 82 (JSC::FTL::canCompile): 83 1 84 2019-04-15 Commit Queue <commit-queue@webkit.org> 2 85 -
trunk/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
r244313 r244324 324 324 325 325 case SetArgumentDefinitely: 326 // Assert that the state of arguments has been set. SetArgumentDefinitely means 326 case SetArgumentMaybe: 327 // Assert that the state of arguments has been set. SetArgumentDefinitely/SetArgumentMaybe means 327 328 // that someone set the argument values out-of-band, and currently this always means setting to a 328 329 // non-clear value. -
trunk/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
r244193 r244324 1864 1864 1865 1865 set(VirtualRegister(argumentStart), get(thisArgument), ImmediateNakedSet); 1866 unsigned numSetArguments = 0; 1866 1867 for (unsigned argument = 1; argument < maxNumArguments; ++argument) { 1867 1868 VariableAccessData* variable = newVariableAccessData(VirtualRegister(remappedArgumentStart + argument)); … … 1883 1884 } 1884 1885 1885 Node* setArgument = addToGraph( SetArgumentDefinitely, OpInfo(variable));1886 Node* setArgument = addToGraph(numSetArguments >= mandatoryMinimum ? SetArgumentMaybe : SetArgumentDefinitely, OpInfo(variable)); 1886 1887 m_currentBlock->variablesAtTail.setOperand(variable->local(), setArgument); 1888 ++numSetArguments; 1887 1889 } 1888 1890 }; -
trunk/Source/JavaScriptCore/dfg/DFGCPSRethreadingPhase.cpp
r244193 r244324 95 95 } 96 96 switch (node->child1()->op()) { 97 case SetArgumentMaybe: 98 DFG_CRASH(m_graph, node, "Invalid Phantom(@SetArgumentMaybe)"); 99 break; 97 100 case Phi: 98 101 case SetArgumentDefinitely: … … 170 173 return; 171 174 } 175 ASSERT(otherNode->op() != SetArgumentMaybe); 172 176 ASSERT(otherNode->op() == SetLocal || otherNode->op() == SetArgumentDefinitely); 173 177 break; … … 176 180 } 177 181 182 ASSERT(otherNode->op() != SetArgumentMaybe); 178 183 ASSERT(otherNode->op() == SetLocal || otherNode->op() == SetArgumentDefinitely || otherNode->op() == GetLocal); 179 184 ASSERT(otherNode->variableAccessData() == variable); … … 231 236 } 232 237 233 ASSERT(otherNode->op() == Phi || otherNode->op() == SetLocal || otherNode->op() == SetArgumentDefinitely );238 ASSERT(otherNode->op() == Phi || otherNode->op() == SetLocal || otherNode->op() == SetArgumentDefinitely || otherNode->op() == SetArgumentMaybe); 234 239 235 240 if (nodeType == PhantomLocal && otherNode->op() == SetLocal) { … … 298 303 // 299 304 // Head variable: describes what is live at the head of the basic block. 300 // Head variable links may refer to Flush, PhantomLocal, Phi, or SetArgumentDefinitely .301 // SetArgumentDefinitely may only appear in the root block.305 // Head variable links may refer to Flush, PhantomLocal, Phi, or SetArgumentDefinitely/SetArgumentMaybe. 306 // SetArgumentDefinitely/SetArgumentMaybe may only appear in the root block. 302 307 // 303 308 // Tail variable: the last thing that happened to the variable in the block. 304 // It may be a Flush, PhantomLocal, GetLocal, SetLocal, SetArgumentDefinitely , or Phi.305 // SetArgumentDefinitely may only appear in the root block. Note that if there ever309 // It may be a Flush, PhantomLocal, GetLocal, SetLocal, SetArgumentDefinitely/SetArgumentMaybe, or Phi. 310 // SetArgumentDefinitely/SetArgumentMaybe may only appear in the root block. Note that if there ever 306 311 // was a GetLocal to the variable, and it was followed by PhantomLocals and 307 312 // Flushes but not SetLocals, then the tail variable will be the GetLocal. … … 312 317 // 313 318 // Child of GetLocal: the operation that the GetLocal keeps alive. It may be 314 // a Phi from the current block. For arguments, it may be a SetArgumentDefinitely. 319 // a Phi from the current block. For arguments, it may be a SetArgumentDefinitely 320 // but it can't be a SetArgumentMaybe. 315 321 // 316 322 // Child of SetLocal: must be a value producing node. 317 323 // 318 324 // Child of Flush: it may be a Phi from the current block or a SetLocal. For 319 // arguments it may also be a SetArgumentDefinitely .325 // arguments it may also be a SetArgumentDefinitely/SetArgumentMaybe. 320 326 // 321 327 // Child of PhantomLocal: it may be a Phi from the current block. For 322 // arguments it may also be a SetArgumentDefinitely .328 // arguments it may also be a SetArgumentDefinitely/SetArgumentMaybe. 323 329 // 324 330 // Children of Phi: other Phis in the same basic block, or any of the 325 // following from predecessor blocks: SetLocal, Phi, or SetArgumentDefinitely .331 // following from predecessor blocks: SetLocal, Phi, or SetArgumentDefinitely/SetArgumentMaybe. 326 332 // These are computed by looking at the tail variables of the predecessor blocks 327 // and either using it directly (if it's a SetLocal, Phi, or SetArgumentDefinitely ) or333 // and either using it directly (if it's a SetLocal, Phi, or SetArgumentDefinitely/SetArgumentMaybe) or 328 334 // loading that nodes child (if it's a GetLocal, PhanomLocal, or Flush - all 329 // of these will have children that are SetLocal, Phi, or SetArgumentDefinitely ).335 // of these will have children that are SetLocal, Phi, or SetArgumentDefinitely/SetArgumentMaybe). 330 336 331 337 switch (node->op()) { … … 347 353 348 354 case SetArgumentDefinitely: 355 case SetArgumentMaybe: 349 356 canonicalizeSet(node); 350 357 break; … … 421 428 variableInPrevious->op() == SetLocal 422 429 || variableInPrevious->op() == Phi 423 || variableInPrevious->op() == SetArgumentDefinitely); 430 || variableInPrevious->op() == SetArgumentDefinitely 431 || variableInPrevious->op() == SetArgumentMaybe); 424 432 425 433 if (!currentPhi->child1()) { … … 484 492 case SetLocal: 485 493 case SetArgumentDefinitely: 494 case SetArgumentMaybe: 486 495 break; 487 496 -
trunk/Source/JavaScriptCore/dfg/DFGClobberize.h
r244193 r244324 452 452 case PhantomLocal: 453 453 case SetArgumentDefinitely: 454 case SetArgumentMaybe: 454 455 case Jump: 455 456 case Branch: -
trunk/Source/JavaScriptCore/dfg/DFGCommon.h
r244193 r244324 174 174 // not be stored to the stack necessarily. This implies that Phantom can 175 175 // reference nodes that have no result, as long as those nodes are valid 176 // GetLocal children (i.e. Phi, SetLocal, SetArgumentDefinitely ).176 // GetLocal children (i.e. Phi, SetLocal, SetArgumentDefinitely, SetArgumentMaybe). 177 177 // 178 178 // LoadStore form also implies that Phis need not have children. By default, -
trunk/Source/JavaScriptCore/dfg/DFGDoesGC.cpp
r244193 r244324 80 80 case PhantomLocal: 81 81 case SetArgumentDefinitely: 82 case SetArgumentMaybe: 82 83 case ArithBitNot: 83 84 case ArithBitAnd: -
trunk/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
r244238 r244324 2326 2326 // Have these no-op cases here to ensure that nobody forgets to add handlers for new opcodes. 2327 2327 case SetArgumentDefinitely: 2328 case SetArgumentMaybe: 2328 2329 case JSConstant: 2329 2330 case LazyJSConstant: -
trunk/Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp
r244193 r244324 236 236 case Phi: 237 237 case SetArgumentDefinitely: 238 case SetArgumentMaybe: 238 239 case PhantomLocal: 239 240 case Flush: { -
trunk/Source/JavaScriptCore/dfg/DFGLiveCatchVariablePreservationPhase.cpp
r244193 r244324 199 199 } 200 200 201 if (currentExceptionHandler && (node->op() == SetLocal || node->op() == SetArgumentDefinitely )) {201 if (currentExceptionHandler && (node->op() == SetLocal || node->op() == SetArgumentDefinitely || node->op() == SetArgumentMaybe)) { 202 202 InlineCallFrame* inlineCallFrame = node->origin.semantic.inlineCallFrame(); 203 203 if (inlineCallFrame) -
trunk/Source/JavaScriptCore/dfg/DFGMaximalFlushInsertionPhase.cpp
r244193 r244324 77 77 } 78 78 79 if (node->op() == SetLocal || (node->op() == SetArgumentDefinitely && !isPrimordialSetArgument) ) {79 if (node->op() == SetLocal || (node->op() == SetArgumentDefinitely && !isPrimordialSetArgument) || node->op() == SetArgumentMaybe) { 80 80 VirtualRegister operand = node->local(); 81 81 VariableAccessData* flushAccessData = currentBlockAccessData.operand(operand); … … 137 137 for (unsigned i = 0; i < block->variablesAtTail.numberOfLocals(); i++) { 138 138 VirtualRegister operand = virtualRegisterForLocal(i); 139 DFG_ASSERT(m_graph, nullptr, initialAccessNodes.operand(operand)->op() == Flush); // We should have inserted a Flush before any SetLocal/SetArgumentDefinitely for the local that we are analyzing now.139 DFG_ASSERT(m_graph, nullptr, initialAccessNodes.operand(operand)->op() == Flush); // We should have inserted a Flush before any SetLocal/SetArgumentDefinitely/SetArgumentMaybe for the local that we are analyzing now. 140 140 VariableAccessData* accessData = initialAccessData.operand(operand); 141 141 DFG_ASSERT(m_graph, nullptr, accessData); -
trunk/Source/JavaScriptCore/dfg/DFGMayExit.cpp
r244193 r244324 49 49 // generally make things a lot better but it might introduce subtle bugs. 50 50 case SetArgumentDefinitely: 51 case SetArgumentMaybe: 51 52 case JSConstant: 52 53 case DoubleConstant: -
trunk/Source/JavaScriptCore/dfg/DFGNode.cpp
r244193 r244324 75 75 case SetLocal: 76 76 case SetArgumentDefinitely: 77 case SetArgumentMaybe: 77 78 case Flush: 78 79 case PhantomLocal: -
trunk/Source/JavaScriptCore/dfg/DFGNodeType.h
r244193 r244324 104 104 macro(CheckTierUpAtReturn, NodeMustGenerate) \ 105 105 \ 106 /* Marker for an argument being set at the prologue of a function. */\106 /* Marker for an argument being set at the prologue of a function. The argument is guaranteed to be set after this node. */\ 107 107 macro(SetArgumentDefinitely, 0) \ 108 /* A marker like the above that we use to track variable liveness and OSR exit state. However, it's not guaranteed to be set. To verify it was set, you'd need to check the actual argument length. We use this for varargs when we're unsure how many argument may actually end up on the stack. */\ 109 macro(SetArgumentMaybe, 0) \ 108 110 \ 109 111 /* Marker of location in the IR where we may possibly perform jump replacement to */\ -
trunk/Source/JavaScriptCore/dfg/DFGPhantomInsertionPhase.cpp
r244193 r244324 118 118 case GetLocal: 119 119 case SetArgumentDefinitely: 120 case SetArgumentMaybe: 120 121 m_values.operand(node->local()) = nullptr; 121 122 break; -
trunk/Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
r244238 r244324 1255 1255 case ForceOSRExit: 1256 1256 case SetArgumentDefinitely: 1257 case SetArgumentMaybe: 1257 1258 case SetFunctionName: 1258 1259 case CheckStructure: -
trunk/Source/JavaScriptCore/dfg/DFGSSAConversionPhase.cpp
r244193 r244324 262 262 // 263 263 // - SetArgumentDefinitely is removed. Note that GetStack nodes have already been inserted. 264 // 265 // - SetArgumentMaybe is removed. It should not have any data flow uses. 264 266 Operands<Node*> valueForOperand(OperandsLike, m_graph.block(0)->variablesAtHead); 265 267 for (BasicBlock* block : m_graph.blocksInPreOrder()) { … … 394 396 } 395 397 398 case SetArgumentMaybe: { 399 node->remove(m_graph); 400 break; 401 } 402 396 403 default: 397 404 break; -
trunk/Source/JavaScriptCore/dfg/DFGSafeToExecute.h
r244314 r244324 199 199 case PhantomLocal: 200 200 case SetArgumentDefinitely: 201 case SetArgumentMaybe: 201 202 case ArithBitNot: 202 203 case ArithBitAnd: -
trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
r244193 r244324 1974 1974 1975 1975 case SetArgumentDefinitely: 1976 case SetArgumentMaybe: 1976 1977 // This is a no-op; it just marks the fact that the argument is being used. 1977 1978 // But it may be profitable to use this as a hook to run speculation checks -
trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
r244193 r244324 2073 2073 2074 2074 case SetArgumentDefinitely: 2075 case SetArgumentMaybe: 2075 2076 // This is a no-op; it just marks the fact that the argument is being used. 2076 2077 // But it may be profitable to use this as a hook to run speculation checks -
trunk/Source/JavaScriptCore/dfg/DFGValidate.cpp
r244193 r244324 495 495 edge->op() == SetLocal 496 496 || edge->op() == SetArgumentDefinitely 497 || edge->op() == Flush497 || edge->op() == SetArgumentMaybe 498 498 || edge->op() == Phi); 499 499 … … 506 506 edge->op() == SetLocal 507 507 || edge->op() == SetArgumentDefinitely 508 || edge->op() == Flush);508 || edge->op() == SetArgumentMaybe); 509 509 510 510 continue; … … 538 538 prevNode->op() == SetLocal 539 539 || prevNode->op() == SetArgumentDefinitely 540 || prevNode->op() == SetArgumentMaybe 540 541 || prevNode->op() == Phi); 541 542 if (prevNode == edge.node()) { … … 667 668 VALIDATE((node, block), getLocalPositions.operand(node->local()) == notSet); 668 669 VALIDATE((node, block), !!node->child1()); 670 VALIDATE((node, block), node->child1()->op() == SetArgumentDefinitely || node->child1()->op() == Phi); 669 671 } 670 672 getLocalPositions.operand(node->local()) = i; … … 683 685 setLocalPositions.operand(node->local()) = notSet; 684 686 break; 687 case SetArgumentMaybe: 688 break; 689 case Flush: 690 case PhantomLocal: 691 if (m_graph.m_form == ThreadedCPS) { 692 VALIDATE((node, block), 693 node->child1()->op() == Phi 694 || node->child1()->op() == SetLocal 695 || node->child1()->op() == SetArgumentDefinitely 696 || node->child1()->op() == SetArgumentMaybe); 697 if (node->op() == PhantomLocal) 698 VALIDATE((node, block), node->child1()->op() != SetArgumentMaybe); 699 } 700 break; 685 701 default: 686 702 break; … … 698 714 checkOperand( 699 715 block, getLocalPositions, setLocalPositions, virtualRegisterForLocal(i)); 716 } 717 } 718 719 if (m_graph.m_form == ThreadedCPS) { 720 Vector<Node*> worklist; 721 HashSet<Node*> seen; 722 for (BasicBlock* block : m_graph.blocksInNaturalOrder()) { 723 for (Node* node : *block) { 724 if (node->op() == GetLocal || node->op() == PhantomLocal) { 725 worklist.append(node); 726 auto addResult = seen.add(node); 727 VALIDATE((node, block), addResult.isNewEntry); 728 } 729 } 730 } 731 732 while (worklist.size()) { 733 Node* node = worklist.takeLast(); 734 switch (node->op()) { 735 case PhantomLocal: 736 case GetLocal: { 737 Node* child = node->child1().node(); 738 if (seen.add(child).isNewEntry) 739 worklist.append(child); 740 break; 741 } 742 case Phi: { 743 for (unsigned i = 0; i < m_graph.numChildren(node); ++i) { 744 Edge edge = m_graph.child(node, i); 745 if (!edge) 746 continue; 747 if (seen.add(edge.node()).isNewEntry) 748 worklist.append(edge.node()); 749 } 750 break; 751 } 752 case SetLocal: 753 case SetArgumentDefinitely: 754 break; 755 case SetArgumentMaybe: 756 VALIDATE((node), !"Should not reach SetArgumentMaybe. GetLocal that has data flow that reaches a SetArgumentMaybe is invalid IR."); 757 break; 758 default: 759 VALIDATE((node), !"Unexecpted node type."); 760 break; 761 } 700 762 } 701 763 } … … 741 803 case SetLocal: 742 804 case SetArgumentDefinitely: 805 case SetArgumentMaybe: 743 806 case Phantom: 744 807 VALIDATE((node), !"bad node type for SSA"); -
trunk/Source/JavaScriptCore/ftl/FTLCapabilities.cpp
r244193 r244324 58 58 case PhantomLocal: 59 59 case SetArgumentDefinitely: 60 case SetArgumentMaybe: 60 61 case Return: 61 62 case ArithBitNot:
Note: See TracChangeset
for help on using the changeset viewer.