Skip to content

Commit

Permalink
Cherry-pick 259548.261@safari-7615-branch (89f3375). https://bugs.web…
Browse files Browse the repository at this point in the history
…kit.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
  • Loading branch information
Constellation authored and aperezdc committed Apr 3, 2023
1 parent dcdef92 commit 1c1cec3
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 5 deletions.
14 changes: 14 additions & 0 deletions 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();
}
25 changes: 20 additions & 5 deletions Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp
Expand Up @@ -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();
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -723,7 +740,7 @@ class ArgumentsEliminationPhase : public Phase {
break;
}

calculator.executeNode(node);
runAvailabilityAnalysis(node);
}
}

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -1099,6 +1114,7 @@ class ArgumentsEliminationPhase : public Phase {
}
storeValue(undefined, storeIndex);
}
storeArgumentCountIncludingThis(argumentCountIncludingThis);

node->remove(m_graph);
node->origin.exitOK = canExit;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down

0 comments on commit 1c1cec3

Please sign in to comment.