...
The code in question is: ``` // Now, insert type conversions if necessary. m_graph.doToChildren( node, [&] (Edge& edge) { Node* result = nullptr; switch (edge.useKind()) { case DoubleRepUse: case DoubleRepRealUse: case DoubleRepAnyIntUse: { if (edge->hasDoubleResult()) break; ASSERT(indexForChecks != UINT_MAX); if (edge->isNumberConstant()) { result = m_insertionSet.insertNode( indexForChecks, SpecBytecodeDouble, DoubleConstant, originForChecks, OpInfo(m_graph.freeze(jsDoubleNumber(edge->asNumber())))); } else if (edge->hasInt52Result()) { result = m_insertionSet.insertNode( indexForChecks, SpecAnyIntAsDouble, DoubleRep, originForChecks, Edge(edge.node(), Int52RepUse)); } else { UseKind useKind; if (edge->shouldSpeculateDoubleReal()) useKind = RealNumberUse; else if (edge->shouldSpeculateNumber()) useKind = NumberUse; else useKind = NotCellUse; result = m_insertionSet.insertNode( indexForChecks, SpecBytecodeDouble, DoubleRep, originForChecks, Edge(edge.node(), useKind)); } edge.setNode(result); break; } case Int52RepUse: { if (edge->hasInt52Result()) break; ASSERT(indexForChecks != UINT_MAX); if (edge->isAnyIntConstant()) { result = m_insertionSet.insertNode( indexForChecks, SpecAnyInt, Int52Constant, originForChecks, OpInfo(edge->constant())); } else if (edge->hasDoubleResult()) { result = m_insertionSet.insertNode( indexForChecks, SpecAnyInt, Int52Rep, originForChecks, Edge(edge.node(), DoubleRepAnyIntUse)); } else if (edge->shouldSpeculateInt32ForArithmetic()) { result = m_insertionSet.insertNode( indexForChecks, SpecInt32Only, Int52Rep, originForChecks, Edge(edge.node(), Int32Use)); } else { result = m_insertionSet.insertNode( indexForChecks, SpecAnyInt, Int52Rep, originForChecks, Edge(edge.node(), AnyIntUse)); } edge.setNode(result); break; } default: { if (!edge->hasDoubleResult() && !edge->hasInt52Result()) break; ASSERT(indexForChecks != UINT_MAX); if (edge->hasDoubleResult()) { result = m_insertionSet.insertNode( indexForChecks, SpecBytecodeDouble, ValueRep, originForChecks, Edge(edge.node(), DoubleRepUse)); } else { result = m_insertionSet.insertNode( indexForChecks, SpecInt32Only | SpecAnyIntAsDouble, ValueRep, originForChecks, Edge(edge.node(), Int52RepUse)); } edge.setNode(result); break; } } // It's remotely possible that this node cannot do type checks, but we now have a // type check on this node. We don't have to handle the general form of this // problem. It only arises when ByteCodeParser emits an immediate SetLocal, rather // than a delayed one. So, we only worry about those checks that we may have put on // a SetLocal. Note that "indexForChecks != indexInBlock" is just another way of // saying "!node->origin.exitOK". if (indexForChecks != indexInBlock && mayHaveTypeCheck(edge.useKind())) { UseKind knownUseKind; switch (edge.useKind()) { case Int32Use: knownUseKind = KnownInt32Use; break; case CellUse: knownUseKind = KnownCellUse; break; case BooleanUse: knownUseKind = KnownBooleanUse; break; default: // This can only arise if we have a Check node, and in that case, we can // just remove the original check. DFG_ASSERT(m_graph, node, node->op() == Check); knownUseKind = UntypedUse; break; } ASSERT(indexForChecks != UINT_MAX); m_insertionSet.insertNode( indexForChecks, SpecNone, Check, originForChecks, edge); edge.setUseKind(knownUseKind); } }); ``` Specifically, this is the case that's generating invalid IR: ``` default: { if (!edge->hasDoubleResult() && !edge->hasInt52Result()) break; ASSERT(indexForChecks != UINT_MAX); if (edge->hasDoubleResult()) { result = m_insertionSet.insertNode( indexForChecks, SpecBytecodeDouble, ValueRep, originForChecks, Edge(edge.node(), DoubleRepUse)); } else { result = m_insertionSet.insertNode( indexForChecks, SpecInt32Only | SpecAnyIntAsDouble, ValueRep, originForChecks, Edge(edge.node(), Int52RepUse)); } edge.setNode(result); break; ``` We see that we're using indexForChecks here. I haven't thought through if it's safe in the other cases, but I'm pretty sure it's unsafe here, as case in point in the crashing program. I think we can safely use indexInBlock here, since we know these ValueRep's will not OSR exit.
Created attachment 320810 [details] IR dump Look at node @264 after fixup phase. It's a ValueRep inserted at indexForChecks, even though that occurs *before* the node it uses, @71
(In reply to Saam Barati from comment #1) > The code in question is: > ``` > // Now, insert type conversions if necessary. > m_graph.doToChildren( > node, > [&] (Edge& edge) { > Node* result = nullptr; > > switch (edge.useKind()) { > case DoubleRepUse: > case DoubleRepRealUse: > case DoubleRepAnyIntUse: { > if (edge->hasDoubleResult()) > break; > > ASSERT(indexForChecks != UINT_MAX); > if (edge->isNumberConstant()) { > result = m_insertionSet.insertNode( > indexForChecks, SpecBytecodeDouble, > DoubleConstant, originForChecks, > > OpInfo(m_graph.freeze(jsDoubleNumber(edge->asNumber())))); > } else if (edge->hasInt52Result()) { > result = m_insertionSet.insertNode( > indexForChecks, SpecAnyIntAsDouble, > DoubleRep, originForChecks, > Edge(edge.node(), Int52RepUse)); > } else { > UseKind useKind; > if (edge->shouldSpeculateDoubleReal()) > useKind = RealNumberUse; > else if (edge->shouldSpeculateNumber()) > useKind = NumberUse; > else > useKind = NotCellUse; > > result = m_insertionSet.insertNode( > indexForChecks, SpecBytecodeDouble, > DoubleRep, originForChecks, > Edge(edge.node(), useKind)); > } > > edge.setNode(result); > break; > } > > case Int52RepUse: { > if (edge->hasInt52Result()) > break; > > ASSERT(indexForChecks != UINT_MAX); > if (edge->isAnyIntConstant()) { > result = m_insertionSet.insertNode( > indexForChecks, SpecAnyInt, Int52Constant, > originForChecks, > OpInfo(edge->constant())); > } else if (edge->hasDoubleResult()) { > result = m_insertionSet.insertNode( > indexForChecks, SpecAnyInt, Int52Rep, > originForChecks, > Edge(edge.node(), DoubleRepAnyIntUse)); > } else if > (edge->shouldSpeculateInt32ForArithmetic()) { > result = m_insertionSet.insertNode( > indexForChecks, SpecInt32Only, Int52Rep, > originForChecks, > Edge(edge.node(), Int32Use)); > } else { > result = m_insertionSet.insertNode( > indexForChecks, SpecAnyInt, Int52Rep, > originForChecks, > Edge(edge.node(), AnyIntUse)); > } > > edge.setNode(result); > break; > } > > default: { > if (!edge->hasDoubleResult() && > !edge->hasInt52Result()) > break; > > ASSERT(indexForChecks != UINT_MAX); > if (edge->hasDoubleResult()) { > result = m_insertionSet.insertNode( > indexForChecks, SpecBytecodeDouble, > ValueRep, originForChecks, > Edge(edge.node(), DoubleRepUse)); > } else { > result = m_insertionSet.insertNode( > indexForChecks, SpecInt32Only | > SpecAnyIntAsDouble, ValueRep, > originForChecks, Edge(edge.node(), > Int52RepUse)); > } > > edge.setNode(result); > break; > } } > > // It's remotely possible that this node cannot do type > checks, but we now have a > // type check on this node. We don't have to handle the > general form of this > // problem. It only arises when ByteCodeParser emits an > immediate SetLocal, rather > // than a delayed one. So, we only worry about those > checks that we may have put on > // a SetLocal. Note that "indexForChecks != > indexInBlock" is just another way of > // saying "!node->origin.exitOK". > if (indexForChecks != indexInBlock && > mayHaveTypeCheck(edge.useKind())) { > UseKind knownUseKind; > > switch (edge.useKind()) { > case Int32Use: > knownUseKind = KnownInt32Use; > break; > case CellUse: > knownUseKind = KnownCellUse; > break; > case BooleanUse: > knownUseKind = KnownBooleanUse; > break; > default: > // This can only arise if we have a Check node, > and in that case, we can > // just remove the original check. > DFG_ASSERT(m_graph, node, node->op() == Check); > knownUseKind = UntypedUse; > break; > } > > ASSERT(indexForChecks != UINT_MAX); > m_insertionSet.insertNode( > indexForChecks, SpecNone, Check, > originForChecks, edge); > > edge.setUseKind(knownUseKind); > } > }); > ``` > > Specifically, this is the case that's generating invalid IR: > ``` > default: { > if (!edge->hasDoubleResult() && > !edge->hasInt52Result()) > break; > > ASSERT(indexForChecks != UINT_MAX); > if (edge->hasDoubleResult()) { > result = m_insertionSet.insertNode( > indexForChecks, SpecBytecodeDouble, > ValueRep, originForChecks, > Edge(edge.node(), DoubleRepUse)); > } else { > result = m_insertionSet.insertNode( > indexForChecks, SpecInt32Only | > SpecAnyIntAsDouble, ValueRep, > originForChecks, Edge(edge.node(), > Int52RepUse)); > } > > edge.setNode(result); > break; > ``` > > We see that we're using indexForChecks here. I haven't thought through if > it's safe in the other cases, but I'm pretty sure it's unsafe here, as case > in point in the crashing program. I think we can safely use indexInBlock > here, since we know these ValueRep's will not OSR exit. But that wouldn't actually solve the problem. The other rep nodes do have exit cases. I think that you need to figure out very exactly why we are inserting a ValueRep before the thing it uses. What is that thing? Where did it come from? Why wasn't it produced by the previous bytecode?
(In reply to Saam Barati from comment #2) > Created attachment 320810 [details] > IR dump > > Look at node @264 after fixup phase. It's a ValueRep inserted at > indexForChecks, even though that occurs *before* the node it uses, @71 Aha! It's because it's an immediate SetLocal for setting up the stack frame of an inline callee. I think that you want to have ByteCodeParser emit some ExitOK's here, and make sure that Fixup knows that it can use those. There should be an ExitOK between the GetLocal at @71 and the SetLocal at @73. It's OK to exit everywhere in that thing because we'll just exit to the beginning of the call instruction, which is totally valid.
(In reply to Filip Pizlo from comment #4) > (In reply to Saam Barati from comment #2) > > Created attachment 320810 [details] > > IR dump > > > > Look at node @264 after fixup phase. It's a ValueRep inserted at > > indexForChecks, even though that occurs *before* the node it uses, @71 > > Aha! It's because it's an immediate SetLocal for setting up the stack frame > of an inline callee. > > I think that you want to have ByteCodeParser emit some ExitOK's here, and > make sure that Fixup knows that it can use those. There should be an ExitOK > between the GetLocal at @71 and the SetLocal at @73. It's OK to exit > everywhere in that thing because we'll just exit to the beginning of the > call instruction, which is totally valid. Yeah I agree. I think we should also not do this: ```result->variableAccessData()->mergeShouldNeverUnbox(true);``` inside the fill lambda. I agree that it should just be valid to exit anywhere in the arity fixup code.
Created attachment 320833 [details] patch
Comment on attachment 320833 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=320833&action=review r=me. > Source/JavaScriptCore/ChangeLog:14 > + Not doing this led to a bug where FixupPhase would insert a ValueRep of > + a node before the actual node. This is obviously invalid IR. I've added > + a new validation rule to catch this malformed IR. Crazy!
Comment on attachment 320833 [details] patch Clearing flags on attachment: 320833 Committed r222060: <http://trac.webkit.org/changeset/222060>
All reviewed patches have been landed. Closing bug.
<rdar://problem/34693334>