Changeset 247532 in webkit


Ignore:
Timestamp:
Jul 17, 2019, 1:30:52 PM (6 years ago)
Author:
mark.lam@apple.com
Message:

ArgumentsEliminationPhase should insert KillStack nodes before PutStack nodes that it adds.
​https://bugs.webkit.org/show_bug.cgi?id=199821
<rdar://problem/52452328>

Reviewed by Filip Pizlo.

JSTests:

  • stress/arguments-elimination-should-insert-KillStacks-before-added-PutStacks.js: Added.

Source/JavaScriptCore:

Excluding the ArgumentsEliminationPhase, PutStack nodes are converted from SetLocal
nodes in the SSAConversionPhase. SetLocal nodes are always preceded by MovHint nodes,
and the SSAConversionPhase always inserts a KillStack node before a MovHint node.
Hence, a PutStack node is always preceded by a KillStack node.

However, the ArgumentsEliminationPhase can convert LoadVarargs nodes into a series
of one or more PutStacks nodes, and it prepends MovHint nodes before the PutStack
nodes. However, it neglects to prepend KillStack nodes as well. Since the
ArgumentsEliminationPhase runs after the SSAConversionPhase, the PutStack nodes
added during ArgumentsElimination will not be preceded by KillStack nodes.

This patch fixes this by inserting a KillStack in the ArgumentsEliminationPhase
before it inserts a MovHint and a PutStack node.

Consider this test case which can manifest the above issue as a crash:

function inlinee(value) {

...
let tmp = value + 1;

}

function reflect() {

return inlinee.apply(undefined, arguments);

}

function test(arr) {

let object = inlinee.apply(undefined, arr); Uses a lot of SetArgumentMaybe nodes.
reflect(); Calls with a LoadVararg, which gets converted into a PutStack of a constant.

}

In this test case, we have a scenario where a SetArgumentMaybe's stack
slot is reused as the stack slot for a PutStack later. Here, the PutStack will
put a constant undefined value. Coincidentally, the SetArgumentMaybe may also
initialize that stack slot to a constant undefined value. Note that by the time
the PutStack executes, the SetArgumentMaybe's stack slot is dead. The liveness of
these 2 values are distinct.

However, because we were missing a KillStack before the PutStack, OSR availability
analysis gets misled into thinking that the PutStack constant value is still in the
stack slot because the value left there by the SetArgumentMaybe hasn't been killed
off yet. As a result, OSR exit code will attempt to recover the PutStack's undefined
constant by loading from the stack slot instead of materializing it. Since
SetArgumentMaybe may not actually initialize the stack slot, we get a crash in OSR
exit when we try to recover the PutStack constant value from the stack slot, and
end up using what ever junk value we read from there.

Fixing the ArgumentsEliminationPhase to insert KillStack before the PutStack
removes this conflation of the PutStack's constant value with the SetArgumentMaybe's
constant value in the same stack slot. And, OSR availability analysis will no
longer be misled to load the PutStack's constant value from the stack, but will
materialize the constant instead.

  • dfg/DFGArgumentsEliminationPhase.cpp:
Location:
trunk
Files:
1 added
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r247485 r247532  
     12019-07-16  Mark Lam  <mark.lam@apple.com>
     2
     3        ArgumentsEliminationPhase should insert KillStack nodes before PutStack nodes that it adds.
     4        https://bugs.webkit.org/show_bug.cgi?id=199821
     5        <rdar://problem/52452328>
     6
     7        Reviewed by Filip Pizlo.
     8
     9        * stress/arguments-elimination-should-insert-KillStacks-before-added-PutStacks.js: Added.
     10
    1112019-07-16  Keith Miller  <keith_miller@apple.com>
    212

  • trunk/Source/JavaScriptCore/ChangeLog

    r247523 r247532  
     12019-07-17  Mark Lam  <mark.lam@apple.com>
     2
     3        ArgumentsEliminationPhase should insert KillStack nodes before PutStack nodes that it adds.
     4        https://bugs.webkit.org/show_bug.cgi?id=199821
     5        <rdar://problem/52452328>
     6
     7        Reviewed by Filip Pizlo.
     8
     9        Excluding the ArgumentsEliminationPhase, PutStack nodes are converted from SetLocal
     10        nodes in the SSAConversionPhase.  SetLocal nodes are always preceded by MovHint nodes,
     11        and the SSAConversionPhase always inserts a KillStack node before a MovHint node.
     12        Hence, a PutStack node is always preceded by a KillStack node.
     13
     14        However, the ArgumentsEliminationPhase can convert LoadVarargs nodes into a series
     15        of one or more PutStacks nodes, and it prepends MovHint nodes before the PutStack
     16        nodes.  However, it neglects to prepend KillStack nodes as well.  Since the
     17        ArgumentsEliminationPhase runs after the SSAConversionPhase, the PutStack nodes
     18        added during ArgumentsElimination will not be preceded by KillStack nodes.
     19
     20        This patch fixes this by inserting a KillStack in the ArgumentsEliminationPhase
     21        before it inserts a MovHint and a PutStack node.
     22
     23        Consider this test case which can manifest the above issue as a crash:
     24
     25            function inlinee(value) {
     26                ...
     27                let tmp = value + 1;
     28            }
     29
     30            function reflect() {
     31                return inlinee.apply(undefined, arguments);
     32            }
     33
     34            function test(arr) {
     35                let object = inlinee.apply(undefined, arr);   // Uses a lot of SetArgumentMaybe nodes.
     36                reflect();    // Calls with a LoadVararg, which gets converted into a PutStack of a constant.
     37            }
     38
     39        In this test case, we have a scenario where a SetArgumentMaybe's stack
     40        slot is reused as the stack slot for a PutStack later.  Here, the PutStack will
     41        put a constant undefined value.  Coincidentally, the SetArgumentMaybe may also
     42        initialize that stack slot to a constant undefined value.  Note that by the time
     43        the PutStack executes, the SetArgumentMaybe's stack slot is dead.  The liveness of
     44        these 2 values are distinct.
     45
     46        However, because we were missing a KillStack before the PutStack, OSR availability
     47        analysis gets misled into thinking that the PutStack constant value is still in the
     48        stack slot because the value left there by the SetArgumentMaybe hasn't been killed
     49        off yet.  As a result, OSR exit code will attempt to recover the PutStack's undefined
     50        constant by loading from the stack slot instead of materializing it.  Since
     51        SetArgumentMaybe may not actually initialize the stack slot, we get a crash in OSR
     52        exit when we try to recover the PutStack constant value from the stack slot, and
     53        end up using what ever junk value we read from there.
     54
     55        Fixing the ArgumentsEliminationPhase to insert KillStack before the PutStack
     56        removes this conflation of the PutStack's constant value with the SetArgumentMaybe's
     57        constant value in the same stack slot.  And, OSR availability analysis will no
     58        longer be misled to load the PutStack's constant value from the stack, but will
     59        materialize the constant instead.
     60
     61        * dfg/DFGArgumentsEliminationPhase.cpp:
     62
    1632019-07-17  Commit Queue  <commit-queue@webkit.org>
    264

  • trunk/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp

    r247183 r247532  
    862862                            jsNumber(argumentCountIncludingThis));
    863863                        insertionSet.insertNode(
     864                            nodeIndex, SpecNone, KillStack, node->origin.takeValidExit(canExit),
     865                            OpInfo(varargsData->count.offset()));
     866                        insertionSet.insertNode(
    864867                            nodeIndex, SpecNone, MovHint, node->origin.takeValidExit(canExit),
    865868                            OpInfo(varargsData->count.offset()), Edge(argumentCountIncludingThisNode));
    … …  
    875878                            m_graph.m_stackAccessData.add(reg, FlushedJSValue);
    876879                       
     880                        insertionSet.insertNode(
     881                            nodeIndex, SpecNone, KillStack, node->origin.takeValidExit(canExit), OpInfo(reg.offset()));
    877882                        insertionSet.insertNode(
    878883                            nodeIndex, SpecNone, MovHint, node->origin.takeValidExit(canExit),
Note: See TracChangeset for help on using the changeset viewer.