From 1c1cec3e742615d58e4d4e1b007dea6a5118ce38 Mon Sep 17 00:00:00 2001 From: Yusuke Suzuki Date: Wed, 22 Feb 2023 20:12:13 -0800 Subject: [PATCH] Cherry-pick 259548.261@safari-7615-branch (89f337538ec6). https://bugs.webkit.org/show_bug.cgi?id=252787 [JSC] Loaded argument can be conflict with newly defined argument in arguments elimination phase https://bugs.webkit.org/show_bug.cgi?id=252787 rdar://105671461 Reviewed by Mark Lam and Ryan Haddad. We now correctly analyze interference between eliminated arguments and newly created arguments. But this interference is analyzed based on the state *just before executing Node's effect". For example, LoadVarargs elimination can get stackslots and put stackslots. And these stackslots can interfere if stackslots are alive and not-interfered when LoadVarargs starts. @a: CreateDirectArguments (loc0, loc1, loc2) ... LoadVarargs @a, (loc2, loc3, loc4) Load @a content, and define loc2, loc3, loc4 stackslots. We check whether LoadVarargs itself is not interfering with the candidate's slots. This is because LoadVarargs can be lowered to the sequence of PutStacks and we may OSR exit in the middle of these PutStacks. So we would like to ensure that these PutStacks are not interfering with the candidate nodes. We need this check only for LoadVarargs since it is the only node which can be lowered to PutStacks in this phase. And let's run storeArgumentCountIncludingThis at last since this is not included in interference analysis since it is putting a constant. * JSTests/stress/loaded-argument-conflict-with-new-argument.js: Added. (foo): (bar): (baz): * Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp: Canonical link: https://commits.webkit.org/259548.261@safari-7615-branch --- ...ded-argument-conflict-with-new-argument.js | 14 +++++++++++ .../dfg/DFGArgumentsEliminationPhase.cpp | 25 +++++++++++++++---- 2 files changed, 34 insertions(+), 5 deletions(-) create mode 100644 JSTests/stress/loaded-argument-conflict-with-new-argument.js diff --git a/JSTests/stress/loaded-argument-conflict-with-new-argument.js b/JSTests/stress/loaded-argument-conflict-with-new-argument.js new file mode 100644 index 000000000000..01e035b34a3f --- /dev/null +++ b/JSTests/stress/loaded-argument-conflict-with-new-argument.js @@ -0,0 +1,14 @@ +function foo(a0, a1, a2, a3, a4, a5, a6, a7, a8) { + return arguments; +} + +function bar() { + let args = foo(0, 0); + baz.apply(null, args); +} + +function baz() {} + +for (let i = 0; i < 1000000; ++i) { + bar(); +} diff --git a/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp b/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp index 632072a5ed0e..7af3a1a58fbd 100644 --- a/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp +++ b/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp @@ -577,6 +577,14 @@ class ArgumentsEliminationPhase : public Phase { for (unsigned nodeIndex = 0; nodeIndex < block->size(); ++nodeIndex) { Node* node = block->at(nodeIndex); + bool alreadyRun = false; + auto runAvailabilityAnalysis = [&](Node* node) { + if (alreadyRun) + return; + calculator.executeNode(node); + alreadyRun = true; + }; + switch (node->op()) { case GetFromArguments: { Node* candidate = node->child1().node(); @@ -667,6 +675,15 @@ class ArgumentsEliminationPhase : public Phase { dataLogLnIf(DFGArgumentsEliminationPhaseInternal::verbose, "eliminating candidate: ", candidate, " because it is clobbered by ", node); transitivelyRemoveCandidate(candidate); } + + // Only LoadVarargs can be lowered into sequence of PutStacks. And these PutStacks can interfere with the candidates' StackSlots. + // This is problematic: when OSR exit happens in the middle of PutStack sequence in LoadVarargs, then we need to materialize phantom candidates, and it will see already clobbered stack slots. + // So, here, we also check whether the candidates do not interfere after "LoadVarargs" happens. + runAvailabilityAnalysis(node); + if (computeInterference(candidate, calculator.m_availability)) { + dataLogLnIf(DFGArgumentsEliminationPhaseInternal::verbose, "eliminating candidate: ", candidate, " because it is clobbered by LoadVarargs on the same node ", node); + transitivelyRemoveCandidate(candidate); + } break; } @@ -723,7 +740,7 @@ class ArgumentsEliminationPhase : public Phase { break; } - calculator.executeNode(node); + runAvailabilityAnalysis(node); } } @@ -1037,8 +1054,6 @@ class ArgumentsEliminationPhase : public Phase { unsigned argumentCountIncludingThis = 1 + countNumberOfArguments(candidate); // |this| if (argumentCountIncludingThis <= varargsData->limit) { - storeArgumentCountIncludingThis(argumentCountIncludingThis); - DFG_ASSERT(m_graph, node, varargsData->limit - 1 >= varargsData->mandatoryMinimum, varargsData->limit, varargsData->mandatoryMinimum); // Define our limit to exclude "this", since that's a bit easier to reason about. unsigned limit = varargsData->limit - 1; @@ -1099,6 +1114,7 @@ class ArgumentsEliminationPhase : public Phase { } storeValue(undefined, storeIndex); } + storeArgumentCountIncludingThis(argumentCountIncludingThis); node->remove(m_graph); node->origin.exitOK = canExit; @@ -1122,8 +1138,6 @@ class ArgumentsEliminationPhase : public Phase { RELEASE_ASSERT(argumentCountIncludingThis >= 1); if (argumentCountIncludingThis <= varargsData->limit) { - storeArgumentCountIncludingThis(argumentCountIncludingThis); - DFG_ASSERT(m_graph, node, varargsData->limit - 1 >= varargsData->mandatoryMinimum, varargsData->limit, varargsData->mandatoryMinimum); // Define our limit to exclude "this", since that's a bit easier to reason about. unsigned limit = varargsData->limit - 1; @@ -1161,6 +1175,7 @@ class ArgumentsEliminationPhase : public Phase { // Now that we have a value, store it. storeValue(value, storeIndex); } + storeArgumentCountIncludingThis(argumentCountIncludingThis); node->remove(m_graph); node->origin.exitOK = canExit;