- Revision
- 250585
- Author
- sbar...@apple.com
- Date
- 2019-10-01 15:55:46 -0700 (Tue, 01 Oct 2019)
Log Message
ObjectAllocationSinkingPhase shouldn't insert hints for allocations which are no longer valid
https://bugs.webkit.org/show_bug.cgi?id=199361
<rdar://problem/52454940>
Reviewed by Yusuke Suzuki.
JSTests:
* stress/allocation-sinking-hints-are-valid-ssa-2.js: Added.
(main.fn):
(main.executor):
(main):
* stress/allocation-sinking-hints-are-valid-ssa.js: Added.
(main.fn):
(main.executor):
(main):
Source/_javascript_Core:
In a prior fix to the object allocation sinking phase, I added code where we
made sure to insert PutHints over Phis for fields of an object at control flow
merge points. However, that code didn't consider that the base of the PutHint
may no longer be a valid heap location. This could cause us to emit invalid
SSA code by referring to a node which does not dominate the PutHint location.
This patch fixes the bug to only emit the PutHints when valid.
This patch also makes it so that DFGValidate actually validates that the graph
is in valid SSA form. E.g, any use of a node N must be dominated by N.
* dfg/DFGObjectAllocationSinkingPhase.cpp:
* dfg/DFGValidate.cpp:
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (250584 => 250585)
--- trunk/JSTests/ChangeLog 2019-10-01 22:47:57 UTC (rev 250584)
+++ trunk/JSTests/ChangeLog 2019-10-01 22:55:46 UTC (rev 250585)
@@ -1,3 +1,20 @@
+2019-10-01 Saam Barati <sbar...@apple.com>
+
+ ObjectAllocationSinkingPhase shouldn't insert hints for allocations which are no longer valid
+ https://bugs.webkit.org/show_bug.cgi?id=199361
+ <rdar://problem/52454940>
+
+ Reviewed by Yusuke Suzuki.
+
+ * stress/allocation-sinking-hints-are-valid-ssa-2.js: Added.
+ (main.fn):
+ (main.executor):
+ (main):
+ * stress/allocation-sinking-hints-are-valid-ssa.js: Added.
+ (main.fn):
+ (main.executor):
+ (main):
+
2019-10-01 Keith Miller <keith_mil...@apple.com>
skip test until we figure out why it's timing out
Added: trunk/JSTests/stress/allocation-sinking-hints-are-valid-ssa-2.js (0 => 250585)
--- trunk/JSTests/stress/allocation-sinking-hints-are-valid-ssa-2.js (rev 0)
+++ trunk/JSTests/stress/allocation-sinking-hints-are-valid-ssa-2.js 2019-10-01 22:55:46 UTC (rev 250585)
@@ -0,0 +1,31 @@
+function main() {
+ const arr = [0];
+ function executor(resolve, ...reject) {
+ arr;
+ if (resolve > arr) {
+ const fn = () => {
+ return fn;
+ };
+ for (const _ of arr) {
+ function fn() {}
+ arr.toString(arr, arr, arr, arr, arr, arr);
+ throw new Error();
+ }
+ } else {
+ for (const _ of [arr]) {
+ arr.toString();
+ }
+ const fn = () => {};
+ }
+ new Promise(executor, arr);
+ let some = {};
+ with(arr) {}
+ return reject;
+ }
+ executor();
+
+ for (let i = 0; i < 100; i++) {
+ executor();
+ }
+}
+main();
Added: trunk/JSTests/stress/allocation-sinking-hints-are-valid-ssa.js (0 => 250585)
--- trunk/JSTests/stress/allocation-sinking-hints-are-valid-ssa.js (rev 0)
+++ trunk/JSTests/stress/allocation-sinking-hints-are-valid-ssa.js 2019-10-01 22:55:46 UTC (rev 250585)
@@ -0,0 +1,31 @@
+function main() {
+ const arr = [0];
+ function executor(resolve, ...reject) {
+ arr;
+ if (resolve > arr) {
+ const fn = () => {
+ return fn;
+ };
+ for (const _ of arr) {
+ function fn() {}
+ arr.toString(arr, arr, arr, arr, arr, arr);
+ throw new Error();
+ }
+ } else {
+ for (const _ of [arr]) {
+ arr.toString();
+ }
+ const fn = () => {};
+ }
+ new Promise(executor, arr);
+ let some = {};
+ with(arr) {}
+ return reject;
+ }
+ executor();
+
+ for (let i = 0; i < 100; i++) {
+ executor();
+ }
+}
+main();
Modified: trunk/Source/_javascript_Core/ChangeLog (250584 => 250585)
--- trunk/Source/_javascript_Core/ChangeLog 2019-10-01 22:47:57 UTC (rev 250584)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-10-01 22:55:46 UTC (rev 250585)
@@ -1,3 +1,24 @@
+2019-10-01 Saam Barati <sbar...@apple.com>
+
+ ObjectAllocationSinkingPhase shouldn't insert hints for allocations which are no longer valid
+ https://bugs.webkit.org/show_bug.cgi?id=199361
+ <rdar://problem/52454940>
+
+ Reviewed by Yusuke Suzuki.
+
+ In a prior fix to the object allocation sinking phase, I added code where we
+ made sure to insert PutHints over Phis for fields of an object at control flow
+ merge points. However, that code didn't consider that the base of the PutHint
+ may no longer be a valid heap location. This could cause us to emit invalid
+ SSA code by referring to a node which does not dominate the PutHint location.
+ This patch fixes the bug to only emit the PutHints when valid.
+
+ This patch also makes it so that DFGValidate actually validates that the graph
+ is in valid SSA form. E.g, any use of a node N must be dominated by N.
+
+ * dfg/DFGObjectAllocationSinkingPhase.cpp:
+ * dfg/DFGValidate.cpp:
+
2019-10-01 Yusuke Suzuki <ysuz...@apple.com>
[JSC] Place VM* in TLS
Modified: trunk/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp (250584 => 250585)
--- trunk/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp 2019-10-01 22:47:57 UTC (rev 250584)
+++ trunk/Source/_javascript_Core/dfg/DFGObjectAllocationSinkingPhase.cpp 2019-10-01 22:55:46 UTC (rev 250585)
@@ -1815,9 +1815,11 @@
availabilityCalculator.m_availability, identifier, phiDef->value());
for (PromotedHeapLocation location : hintsForPhi[variable->index()]) {
- m_insertionSet.insert(0,
- location.createHint(m_graph, block->at(0)->origin.withInvalidExit(), phiDef->value()));
- m_localMapping.set(location, phiDef->value());
+ if (m_heap.onlyLocalAllocation(location.base())) {
+ m_insertionSet.insert(0,
+ location.createHint(m_graph, block->at(0)->origin.withInvalidExit(), phiDef->value()));
+ m_localMapping.set(location, phiDef->value());
+ }
}
}
Modified: trunk/Source/_javascript_Core/dfg/DFGValidate.cpp (250584 => 250585)
--- trunk/Source/_javascript_Core/dfg/DFGValidate.cpp 2019-10-01 22:47:57 UTC (rev 250584)
+++ trunk/Source/_javascript_Core/dfg/DFGValidate.cpp 2019-10-01 22:55:46 UTC (rev 250585)
@@ -31,6 +31,7 @@
#include "CodeBlockWithJITType.h"
#include "DFGClobberize.h"
#include "DFGClobbersExitState.h"
+#include "DFGDominators.h"
#include "DFGMayExit.h"
#include "JSCInlines.h"
#include <wtf/Assertions.h>
@@ -775,6 +776,10 @@
VALIDATE((), !m_graph.m_argumentFormats.isEmpty()); // We always have at least one entrypoint.
VALIDATE((), m_graph.m_rootToArguments.isEmpty()); // This is only used in CPS.
+ m_graph.initializeNodeOwners();
+
+ auto& dominators = m_graph.ensureSSADominators();
+
for (unsigned entrypointIndex : m_graph.m_entrypointIndexToCatchBytecodeOffset.keys())
VALIDATE((), entrypointIndex > 0); // By convention, 0 is the entrypoint index for the op_enter entrypoint, which can not be in a catch.
@@ -788,6 +793,8 @@
bool didSeeExitOK = false;
bool isOSRExited = false;
+ HashSet<Node*> nodesInThisBlock;
+
for (auto* node : *block) {
didSeeExitOK |= node->origin.exitOK;
switch (node->op()) {
@@ -906,7 +913,13 @@
});
break;
}
+
isOSRExited |= node->isPseudoTerminal();
+
+ m_graph.doToChildren(node, [&] (Edge child) {
+ VALIDATE((node), dominators.strictlyDominates(child->owner, block) || nodesInThisBlock.contains(child.node()));
+ });
+ nodesInThisBlock.add(node);
}
}
}
