Merge r250585 - ObjectAllocationSinkingPhase shouldn't insert hints f…

…or 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/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:

JSTests/ChangeLog

@@ -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-09-16  Saam Barati  <sbarati@apple.com>
 
         JSObject::putInlineSlow should not ignore "__proto__" for Proxy

JSTests/stress/allocation-sinking-hints-are-valid-ssa-2.js

@@ -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();

JSTests/stress/allocation-sinking-hints-are-valid-ssa.js

@@ -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();

Source/JavaScriptCore/ChangeLog

@@ -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-09-16  Saam Barati  <sbarati@apple.com>
 
         JSObject::putInlineSlow should not ignore "__proto__" for Proxy

Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp

@@ -1815,9 +1815,11 @@ class ObjectAllocationSinkingPhase : public Phase {
                     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());
+                    }
                 }
             }
 

Source/JavaScriptCore/dfg/DFGValidate.cpp

@@ -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 @@ class Validate {
         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 @@ class Validate {
             bool didSeeExitOK = false;
             bool isOSRExited = false;
 
+            HashSet<Node*> nodesInThisBlock;
+
             for (auto* node : *block) {
                 didSeeExitOK |= node->origin.exitOK;
                 switch (node->op()) {
@@ -906,7 +913,13 @@ class Validate {
                         });
                     break;
                 }
+
                 isOSRExited |= node->isPseudoTerminal();
+
+                m_graph.doToChildren(node, [&] (Edge child) {
+                    VALIDATE((node), dominators.strictlyDominates(child->owner, block) || nodesInThisBlock.contains(child.node()));
+                });
+                nodesInThisBlock.add(node);
             }
         }
     }