- Revision
- 247608
- Author
- kocsen_ch...@apple.com
- Date
- 2019-07-18 13:24:46 -0700 (Thu, 18 Jul 2019)
Log Message
Cherry-pick r247532. rdar://problem/53228435
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:
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@247532 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Modified Paths
Added Paths
Diff
Modified: branches/safari-608-branch/JSTests/ChangeLog (247607 => 247608)
--- branches/safari-608-branch/JSTests/ChangeLog 2019-07-18 20:24:44 UTC (rev 247607)
+++ branches/safari-608-branch/JSTests/ChangeLog 2019-07-18 20:24:46 UTC (rev 247608)
@@ -1,3 +1,87 @@
+2019-07-17 Kocsen Chung <kocsen_ch...@apple.com>
+
+ Cherry-pick r247532. rdar://problem/53228435
+
+ 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:
+
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@247532 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 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-12 Justin Michaud <justin_mich...@apple.com>
B3 should reduce (integer) Sub(Neg(x), y) to Neg(Add(x, y))
Added: branches/safari-608-branch/JSTests/stress/arguments-elimination-should-insert-KillStacks-before-added-PutStacks.js (0 => 247608)
--- branches/safari-608-branch/JSTests/stress/arguments-elimination-should-insert-KillStacks-before-added-PutStacks.js (rev 0)
+++ branches/safari-608-branch/JSTests/stress/arguments-elimination-should-insert-KillStacks-before-added-PutStacks.js 2019-07-18 20:24:46 UTC (rev 247608)
@@ -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: branches/safari-608-branch/Source/_javascript_Core/ChangeLog (247607 => 247608)
--- branches/safari-608-branch/Source/_javascript_Core/ChangeLog 2019-07-18 20:24:44 UTC (rev 247607)
+++ branches/safari-608-branch/Source/_javascript_Core/ChangeLog 2019-07-18 20:24:46 UTC (rev 247608)
@@ -1,5 +1,141 @@
2019-07-17 Kocsen Chung <kocsen_ch...@apple.com>
+ Cherry-pick r247532. rdar://problem/53228435
+
+ 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:
+
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@247532 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 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 Kocsen Chung <kocsen_ch...@apple.com>
+
Cherry-pick r247474. rdar://problem/53229615
JSGlobalObject type macros should support feature flags and WeakRef should have one
Modified: branches/safari-608-branch/Source/_javascript_Core/dfg/DFGArgumentsEliminationPhase.cpp (247607 => 247608)
--- branches/safari-608-branch/Source/_javascript_Core/dfg/DFGArgumentsEliminationPhase.cpp 2019-07-18 20:24:44 UTC (rev 247607)
+++ branches/safari-608-branch/Source/_javascript_Core/dfg/DFGArgumentsEliminationPhase.cpp 2019-07-18 20:24:46 UTC (rev 247608)
@@ -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(
