Skip to content

Commit

Permalink
Array iterator creation intrinsics need ToThis
Browse files Browse the repository at this point in the history
https://bugs.webkit.org/show_bug.cgi?id=263408
rdar://113898245

Reviewed by Yusuke Suzuki.

Currently, we don't ToThis the 'this' value when we intrinsicify
the various Array iterator creation functions, which we should.
This patch also changes `clobbersExitState` to say exit state
is not clobbered if a node only writes to `HeapObjectCount`.
Our previous behavior was overly conservative, which caused
assertion failures as the `ToObject` following the `ToThis`
would get converted to a `Check(Object)` when exit was invalid.

* JSTests/stress/array-iterator-to-this.js: Added.
(opt):
(main):
* Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::handleIntrinsicCall):
* Source/JavaScriptCore/dfg/DFGClobbersExitState.cpp:
(JSC::DFG::clobbersExitState):

Originally-landed-as: 267815.357@safari-7617-branch (ae764a8). rdar://119597428
Canonical link: https://commits.webkit.org/272163@main
  • Loading branch information
kmiller68 authored and robert-jenner committed Dec 16, 2023
1 parent 7b97f35 commit ecb7da6
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 5 deletions.
27 changes: 27 additions & 0 deletions JSTests/stress/array-iterator-to-this.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
function opt(f, length = 5) {
(function () {
f;
length;
})();

return f();
}

function main() {
opt(function () {});

for (let i = 0; i < 10000; i++) {
let threw = false;
try {
const iterator = opt(Array.prototype.keys);
print([...iterator]);
} catch {
threw = true;
}

if (!threw)
throw new Error();
}
}

main();
3 changes: 2 additions & 1 deletion Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2586,9 +2586,10 @@ auto ByteCodeParser::handleIntrinsicCall(Node* callee, Operand resultOperand, Ca
// Add the constant before exit becomes invalid because we may want to insert (redundant) checks on it in Fixup.
Node* kindNode = jsConstant(jsNumber(static_cast<uint32_t>(*kind)));

Node* thisValue = addToGraph(ToThis, OpInfo(ECMAMode::strict()), OpInfo(getPrediction()), get(virtualRegisterForArgumentIncludingThis(0, registerOffset)));
// We don't have an existing error string.
unsigned errorStringIndex = UINT32_MAX;
Node* object = addToGraph(ToObject, OpInfo(errorStringIndex), OpInfo(SpecNone), get(virtualRegisterForArgumentIncludingThis(0, registerOffset)));
Node* object = addToGraph(ToObject, OpInfo(errorStringIndex), OpInfo(SpecNone), thisValue);

Node* iterator = addToGraph(NewInternalFieldObject, OpInfo(m_graph.registerStructure(globalObject->arrayIteratorStructure())));

Expand Down
8 changes: 4 additions & 4 deletions Source/JavaScriptCore/dfg/DFGClobbersExitState.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,11 @@ bool clobbersExitState(Graph& graph, Node* node)
clobberize(
graph, node, NoOpClobberize(),
[&] (const AbstractHeap& heap) {
// There shouldn't be such a thing as a strict subtype of SideState. That's what allows
// us to use a fast != check, below.
ASSERT(!heap.isStrictSubtypeOf(SideState));
// There shouldn't be such a thing as a strict subtype of SideState or HeapObjectCount.
// That's what allows us to use a fast != check, below.
ASSERT(!heap.isStrictSubtypeOf(SideState) && !heap.isStrictSubtypeOf(HeapObjectCount));

if (heap != SideState)
if (heap != SideState && heap != HeapObjectCount)
result = true;
},
NoOpClobberize());
Expand Down

0 comments on commit ecb7da6

Please sign in to comment.