Bug 245468

<rdar://problem/100497797>

Created attachment 462916 [details] Patch

Elaborating on the explanation: in SpeculativeJIT::compileNewArray, in specifically the slow path (reached when we have invalidated the isHavingABadTime watchpoint), we explicitly use() each operand after we write it to the buffer: > case ALL_ARRAY_STORAGE_INDEXING_TYPES: { > JSValueOperand operand(this, use, ManualOperandSpeculation); > JSValueRegs operandRegs = operand.jsValueRegs(); > if (hasInt32(node->indexingType())) { > DFG_TYPE_CHECK( > operandRegs, use, SpecInt32Only, > m_jit.branchIfNotInt32(operandRegs)); > } > m_jit.storeValue(operandRegs, buffer + operandIdx); > operand.use(); > break; > } This is an optimization to avoid spilling operands unnecessarily when we call the new-array operation further down, by manually managing the node lifetimes. If a later operand fails the DFG_TYPE_CHECK, however, we may OSRExit. At that point, the exit information will be collected after earlier operands were used, potentially leaving it dead, resulting in the undefined value we see in this bug. This patch resolves the issue by doing two separate passes - first, we traverse the operands and emit the necessary type checks; second, we traverse the operands again and write them to the buffer. In this scheme, we generate the exit information prior to using any of the operands, so they should all remain live during this period.

Comment on attachment 462916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=462916&action=review r=me > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:9694 > + for (unsigned operandIdx = 0; operandIdx < node->numChildren(); ++operandIdx) { > + Edge use = m_graph.m_varArgChildren[node->firstChild() + operandIdx]; > + JSValueOperand operand(this, use, ManualOperandSpeculation); > + JSValueRegs operandRegs = operand.jsValueRegs(); > + if (hasInt32(node->indexingType())) { > + DFG_TYPE_CHECK( > + operandRegs, use, SpecInt32Only, > + m_jit.branchIfNotInt32(operandRegs)); > + } > + } Let's put it under `if (hasInt32(node->indexingType()))` And remove inner `if (hasInt32(node->indexingType()))`.

Created attachment 462917 [details] Patch

Comment on attachment 462917 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=462917&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:9657 > case ALL_DOUBLE_INDEXING_TYPES: { In WebKit coding style, `case` is aligned to `switch`. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:9684 > case ALL_INT32_INDEXING_TYPES: > case ALL_CONTIGUOUS_INDEXING_TYPES: > case ALL_ARRAY_STORAGE_INDEXING_TYPES: { Ditto.

Comment on attachment 462917 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=462917&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:9704 > default: Ditto.

d_degazio@apple.com: Pull request: https://github.com/WebKit/WebKit/pull/5211

*** Bug 246307 has been marked as a duplicate of this bug. ***

Comment on attachment 462917 [details] Patch Let's land this via the github PR.

Committed 256611@main (104b2a1546ed): <https://commits.webkit.org/256611@main> Reviewed commits have been landed. Closing PR #5211 and removing active labels.