Title: [247532] trunk
- Revision
- 247532
- Author
- mark....@apple.com
- Date
- 2019-07-17 13:30:52 -0700 (Wed, 17 Jul 2019)
Log 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/_javascript_Core:
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:
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (247531 => 247532)
--- trunk/JSTests/ChangeLog 2019-07-17 20:27:30 UTC (rev 247531)
+++ trunk/JSTests/ChangeLog 2019-07-17 20:30:52 UTC (rev 247532)
@@ -1,3 +1,13 @@
+2019-07-16 Mark Lam <mark....@apple.com>
+
+ 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.
+
+ * stress/arguments-elimination-should-insert-KillStacks-before-added-PutStacks.js: Added.
+
2019-07-16 Keith Miller <keith_mil...@apple.com>
Unreviewed, test262 gardening.
Added: trunk/JSTests/stress/arguments-elimination-should-insert-KillStacks-before-added-PutStacks.js (0 => 247532)
--- trunk/JSTests/stress/arguments-elimination-should-insert-KillStacks-before-added-PutStacks.js (rev 0)
+++ trunk/JSTests/stress/arguments-elimination-should-insert-KillStacks-before-added-PutStacks.js 2019-07-17 20:30:52 UTC (rev 247532)
@@ -0,0 +1,25 @@
+//@ runDefault("--thresholdForJITAfterWarmUp=10", "--thresholdForFTLOptimizeAfterWarmUp=1000", "--useConcurrentJIT=false")
+
+'use strict';
+
+function inlinee(value) {
+ for (let i = 0; i < arguments.length; i++) {
+ }
+ let tmp = value + 1;
+}
+
+function reflect() {
+ return inlinee.apply(undefined, arguments);
+}
+
+function test(arr) {
+ let object = inlinee.apply(undefined, arr);
+ reflect();
+}
+
+for (let i = 0; i < 10000; i++) {
+ let arr = [];
+ for (let j = 0; j < 1 + i % 100; j++)
+ arr.push(1);
+ test(arr);
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (247531 => 247532)
--- trunk/Source/_javascript_Core/ChangeLog 2019-07-17 20:27:30 UTC (rev 247531)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-07-17 20:30:52 UTC (rev 247532)
@@ -1,3 +1,65 @@
+2019-07-17 Mark Lam <mark....@apple.com>
+
+ 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.
+
+ 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:
+
2019-07-17 Commit Queue <commit-qu...@webkit.org>
Unreviewed, rolling out r247505.
Modified: trunk/Source/_javascript_Core/dfg/DFGArgumentsEliminationPhase.cpp (247531 => 247532)
--- trunk/Source/_javascript_Core/dfg/DFGArgumentsEliminationPhase.cpp 2019-07-17 20:27:30 UTC (rev 247531)
+++ trunk/Source/_javascript_Core/dfg/DFGArgumentsEliminationPhase.cpp 2019-07-17 20:30:52 UTC (rev 247532)
@@ -861,6 +861,9 @@
nodeIndex, node->origin.withExitOK(canExit),
jsNumber(argumentCountIncludingThis));
insertionSet.insertNode(
+ nodeIndex, SpecNone, KillStack, node->origin.takeValidExit(canExit),
+ OpInfo(varargsData->count.offset()));
+ insertionSet.insertNode(
nodeIndex, SpecNone, MovHint, node->origin.takeValidExit(canExit),
OpInfo(varargsData->count.offset()), Edge(argumentCountIncludingThisNode));
insertionSet.insertNode(
@@ -875,6 +878,8 @@
m_graph.m_stackAccessData.add(reg, FlushedJSValue);
insertionSet.insertNode(
+ nodeIndex, SpecNone, KillStack, node->origin.takeValidExit(canExit), OpInfo(reg.offset()));
+ insertionSet.insertNode(
nodeIndex, SpecNone, MovHint, node->origin.takeValidExit(canExit),
OpInfo(reg.offset()), Edge(value));
insertionSet.insertNode(
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
