ObjectAllocationSinkingPhase shouldn't insert hints for allocations which are no...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 1 Oct 2019 22:55:46 +0000 (22:55 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 1 Oct 2019 22:55:46 +0000 (22:55 +0000)
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/JavaScriptCore:

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:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@250585 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/allocation-sinking-hints-are-valid-ssa-2.js [new file with mode: 0644]
JSTests/stress/allocation-sinking-hints-are-valid-ssa.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp
Source/JavaScriptCore/dfg/DFGValidate.cpp

index d397c5d..c085e5f 100644 (file)
@@ -1,3 +1,20 @@
+2019-10-01  Saam Barati  <sbarati@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_miller@apple.com>
 
         skip test until we figure out why it's timing out
diff --git a/JSTests/stress/allocation-sinking-hints-are-valid-ssa-2.js b/JSTests/stress/allocation-sinking-hints-are-valid-ssa-2.js
new file mode 100644 (file)
index 0000000..8738443
--- /dev/null
@@ -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();
diff --git a/JSTests/stress/allocation-sinking-hints-are-valid-ssa.js b/JSTests/stress/allocation-sinking-hints-are-valid-ssa.js
new file mode 100644 (file)
index 0000000..8738443
--- /dev/null
@@ -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();
index 7d18441..fc30a03 100644 (file)
@@ -1,3 +1,24 @@
+2019-10-01  Saam Barati  <sbarati@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  <ysuzuki@apple.com>
 
         [JSC] Place VM* in TLS
index de0ec63..cfc3d7b 100644 (file)
@@ -1815,9 +1815,11 @@ private:
                     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());
+                    }
                 }
             }
 
index e3141e6..8c04191 100644 (file)
@@ -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 @@ private:
         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 @@ private:
             bool didSeeExitOK = false;
             bool isOSRExited = false;
             
+            HashSet<Node*> nodesInThisBlock;
+
             for (auto* node : *block) {
                 didSeeExitOK |= node->origin.exitOK;
                 switch (node->op()) {
@@ -906,7 +913,13 @@ private:
                         });
                     break;
                 }
+
                 isOSRExited |= node->isPseudoTerminal();
+
+                m_graph.doToChildren(node, [&] (Edge child) {
+                    VALIDATE((node), dominators.strictlyDominates(child->owner, block) || nodesInThisBlock.contains(child.node()));
+                });
+                nodesInThisBlock.add(node);
             }
         }
     }