[webkit-changes] [266813] trunk

Wed, 09 Sep 2020 21:49:24 -0700

Title: [266813] trunk
Revision
266813
Author
sbar...@apple.com
Date
2020-09-09 21:47:38 -0700 (Wed, 09 Sep 2020)

Log Message

OutOfBoundsSaneChain operations should use their own heap locations
https://bugs.webkit.org/show_bug.cgi?id=216328
<rdar://problem/68568039>

Reviewed by Keith Miller.

JSTests:

* stress/out-of-bounds-sane-chain-need-their-own-heap-location.js: Added.
(foo):

Source/_javascript_Core:

There is code in local CSE that does some basic bounds check elimination
for PutByVal. It does this analysis by seeing if a particular heap location
is already defined, and if so, it eliminates the bounds check for the
PutByVal. This doesn't work for OutOfBoundsSaneChain for the obvious reason
that these GetByVals are not proven to be in bounds. So GetByVal's in the
OutOfBoundsSaneChain mode reusing non OutOfBoundsSaneChain heap locations
can lead to a bug where we mistakenly remove a bounds check. The fix is to
have all OutOfBoundsSaneChain operations use distinct heaps, and for CSE to
not query those heaps.

* dfg/DFGArrayMode.h:
(JSC::DFG::ArrayMode::isAnySaneChain const): Deleted.
* dfg/DFGClobberize.h:
(JSC::DFG::clobberize):
* dfg/DFGHeapLocation.cpp:
(WTF::printInternal):
* dfg/DFGHeapLocation.h:

Modified Paths

Added Paths

Diff

Modified: trunk/JSTests/ChangeLog (266812 => 266813)


--- trunk/JSTests/ChangeLog	2020-09-10 03:40:23 UTC (rev 266812)
+++ trunk/JSTests/ChangeLog	2020-09-10 04:47:38 UTC (rev 266813)
@@ -1,3 +1,14 @@
+2020-09-09  Saam Barati  <sbar...@apple.com>
+
+        OutOfBoundsSaneChain operations should use their own heap locations
+        https://bugs.webkit.org/show_bug.cgi?id=216328
+        <rdar://problem/68568039>
+
+        Reviewed by Keith Miller.
+
+        * stress/out-of-bounds-sane-chain-need-their-own-heap-location.js: Added.
+        (foo):
+
 2020-09-09  Alexey Shvayka  <shvaikal...@gmail.com>
 
         Don't emitDirectBinding() if there is a [...rest] element binding

Added: trunk/JSTests/stress/out-of-bounds-sane-chain-need-their-own-heap-location.js (0 => 266813)


--- trunk/JSTests/stress/out-of-bounds-sane-chain-need-their-own-heap-location.js	                        (rev 0)
+++ trunk/JSTests/stress/out-of-bounds-sane-chain-need-their-own-heap-location.js	2020-09-10 04:47:38 UTC (rev 266813)
@@ -0,0 +1,13 @@
+const a0 = [0];
+function foo() {
+    for (let i=1; i<100; i++) {
+        a0[i];
+        a0[i] = undefined;
+        let x = [];
+        for (let j = 0; j < 20; j++) {}
+    }
+}
+
+for (let i=0; i<10000; i++) {
+    foo();
+}

Modified: trunk/Source/_javascript_Core/ChangeLog (266812 => 266813)


--- trunk/Source/_javascript_Core/ChangeLog	2020-09-10 03:40:23 UTC (rev 266812)
+++ trunk/Source/_javascript_Core/ChangeLog	2020-09-10 04:47:38 UTC (rev 266813)
@@ -1,3 +1,29 @@
+2020-09-09  Saam Barati  <sbar...@apple.com>
+
+        OutOfBoundsSaneChain operations should use their own heap locations
+        https://bugs.webkit.org/show_bug.cgi?id=216328
+        <rdar://problem/68568039>
+
+        Reviewed by Keith Miller.
+
+        There is code in local CSE that does some basic bounds check elimination
+        for PutByVal. It does this analysis by seeing if a particular heap location
+        is already defined, and if so, it eliminates the bounds check for the
+        PutByVal. This doesn't work for OutOfBoundsSaneChain for the obvious reason
+        that these GetByVals are not proven to be in bounds. So GetByVal's in the
+        OutOfBoundsSaneChain mode reusing non OutOfBoundsSaneChain heap locations
+        can lead to a bug where we mistakenly remove a bounds check. The fix is to
+        have all OutOfBoundsSaneChain operations use distinct heaps, and for CSE to
+        not query those heaps.
+
+        * dfg/DFGArrayMode.h:
+        (JSC::DFG::ArrayMode::isAnySaneChain const): Deleted.
+        * dfg/DFGClobberize.h:
+        (JSC::DFG::clobberize):
+        * dfg/DFGHeapLocation.cpp:
+        (WTF::printInternal):
+        * dfg/DFGHeapLocation.h:
+
 2020-09-09  Keith Miller  <keith_mil...@apple.com>
 
         BigInt should PACCage its data pointer

Modified: trunk/Source/_javascript_Core/dfg/DFGArrayMode.h (266812 => 266813)


--- trunk/Source/_javascript_Core/dfg/DFGArrayMode.h	2020-09-10 03:40:23 UTC (rev 266812)
+++ trunk/Source/_javascript_Core/dfg/DFGArrayMode.h	2020-09-10 04:47:38 UTC (rev 266813)
@@ -287,11 +287,6 @@
         return speculation() == Array::OutOfBoundsSaneChain;
     }
 
-    bool isAnySaneChain() const
-    {
-        return isInBoundsSaneChain() || isOutOfBoundsSaneChain();
-    }
-    
     bool isOutOfBounds() const
     {
         return speculation() == Array::OutOfBounds || speculation() == Array::OutOfBoundsSaneChain;

Modified: trunk/Source/_javascript_Core/dfg/DFGClobberize.h (266812 => 266813)


--- trunk/Source/_javascript_Core/dfg/DFGClobberize.h	2020-09-10 03:40:23 UTC (rev 266812)
+++ trunk/Source/_javascript_Core/dfg/DFGClobberize.h	2020-09-10 04:47:38 UTC (rev 266813)
@@ -937,7 +937,7 @@
             if (mode.isInBounds() || mode.isOutOfBoundsSaneChain()) {
                 read(Butterfly_publicLength);
                 read(IndexedInt32Properties);
-                LocationKind kind = mode.isOutOfBoundsSaneChain() ? IndexedPropertyInt32OrOtherLoc : indexedPropertyLoc;
+                LocationKind kind = mode.isOutOfBoundsSaneChain() ? IndexedPropertyInt32OutOfBoundsSaneChainLoc : indexedPropertyLoc;
                 def(HeapLocation(kind, IndexedInt32Properties, graph.varArgChild(node, 0), graph.varArgChild(node, 1)), LazyNode(node));
                 return;
             }
@@ -949,11 +949,16 @@
                 read(Butterfly_publicLength);
                 read(IndexedDoubleProperties);
                 LocationKind kind;
-                if (node->hasDoubleResult())
-                    kind = mode.isAnySaneChain() ? IndexedPropertyDoubleSaneChainLoc : IndexedPropertyDoubleLoc;
-                else {
+                if (node->hasDoubleResult()) {
+                    if (mode.isInBoundsSaneChain())
+                        kind = IndexedPropertyDoubleSaneChainLoc;
+                    else if (mode.isOutOfBoundsSaneChain())
+                        kind = IndexedPropertyDoubleOutOfBoundsSaneChainLoc;
+                    else
+                        kind = IndexedPropertyDoubleLoc;
+                } else {
                     ASSERT(mode.isOutOfBoundsSaneChain());
-                    kind = IndexedPropertyDoubleOrOtherSaneChainLoc;
+                    kind = IndexedPropertyDoubleOrOtherOutOfBoundsSaneChainLoc;
                 }
                 def(HeapLocation(kind, IndexedDoubleProperties, graph.varArgChild(node, 0), graph.varArgChild(node, 1)), LazyNode(node));
                 return;
@@ -965,7 +970,7 @@
             if (mode.isInBounds() || mode.isOutOfBoundsSaneChain()) {
                 read(Butterfly_publicLength);
                 read(IndexedContiguousProperties);
-                def(HeapLocation(indexedPropertyLoc, IndexedContiguousProperties, graph.varArgChild(node, 0), graph.varArgChild(node, 1)), LazyNode(node));
+                def(HeapLocation(mode.isOutOfBoundsSaneChain() ? IndexedPropertyJSOutOfBoundsSaneChainLoc : indexedPropertyLoc, IndexedContiguousProperties, graph.varArgChild(node, 0), graph.varArgChild(node, 1)), LazyNode(node));
                 return;
             }
             clobberTop();
@@ -1054,7 +1059,7 @@
             if (node->arrayMode().mayStoreToHole())
                 write(Butterfly_publicLength);
             def(HeapLocation(indexedPropertyLoc, IndexedInt32Properties, base, index), LazyNode(value));
-            def(HeapLocation(IndexedPropertyInt32OrOtherLoc, IndexedInt32Properties, base, index), LazyNode(value));
+            def(HeapLocation(IndexedPropertyInt32OutOfBoundsSaneChainLoc, IndexedInt32Properties, base, index), LazyNode(value));
             return;
             
         case Array::Double:
@@ -1070,6 +1075,7 @@
                 write(Butterfly_publicLength);
             def(HeapLocation(IndexedPropertyDoubleLoc, IndexedDoubleProperties, base, index), LazyNode(value));
             def(HeapLocation(IndexedPropertyDoubleSaneChainLoc, IndexedDoubleProperties, base, index), LazyNode(value));
+            def(HeapLocation(IndexedPropertyDoubleOutOfBoundsSaneChainLoc, IndexedDoubleProperties, base, index), LazyNode(value));
             return;
             
         case Array::Contiguous:
@@ -1084,6 +1090,7 @@
             if (node->arrayMode().mayStoreToHole())
                 write(Butterfly_publicLength);
             def(HeapLocation(indexedPropertyLoc, IndexedContiguousProperties, base, index), LazyNode(value));
+            def(HeapLocation(IndexedPropertyJSOutOfBoundsSaneChainLoc, IndexedContiguousProperties, base, index), LazyNode(value));
             return;
             
         case Array::ArrayStorage:

Modified: trunk/Source/_javascript_Core/dfg/DFGHeapLocation.cpp (266812 => 266813)


--- trunk/Source/_javascript_Core/dfg/DFGHeapLocation.cpp	2020-09-10 03:40:23 UTC (rev 266812)
+++ trunk/Source/_javascript_Core/dfg/DFGHeapLocation.cpp	2020-09-10 04:47:38 UTC (rev 266813)
@@ -142,16 +142,20 @@
         out.print("IndexedPropertyDoubleSaneChainLoc");
         return;
 
-    case IndexedPropertyDoubleOrOtherSaneChainLoc:
-        out.print("IndexedPropertyDoubleOrOtherSaneChainLoc");
+    case IndexedPropertyDoubleOutOfBoundsSaneChainLoc:
+        out.print("IndexedPropertyDoubleOutOfBoundsSaneChainLoc");
         return;
 
+    case IndexedPropertyDoubleOrOtherOutOfBoundsSaneChainLoc:
+        out.print("IndexedPropertyDoubleOrOtherOutOfBoundsSaneChainLoc");
+        return;
+
     case IndexedPropertyInt32Loc:
         out.print("IndexedPropertyInt32Loc");
         return;
 
-    case IndexedPropertyInt32OrOtherLoc:
-        out.print("IndexedPropertyInt32OrOtherLoc");
+    case IndexedPropertyInt32OutOfBoundsSaneChainLoc:
+        out.print("IndexedPropertyInt32OutOfBoundsSaneChainLoc");
         return;
 
     case IndexedPropertyInt52Loc:
@@ -162,6 +166,10 @@
         out.print("IndexedPropertyJSLoc");
         return;
 
+    case IndexedPropertyJSOutOfBoundsSaneChainLoc:
+        out.print("IndexedPropertyJSOutOfBoundsSaneChainLoc");
+        return;
+
     case IndexedPropertyStorageLoc:
         out.print("IndexedPropertyStorageLoc");
         return;

Modified: trunk/Source/_javascript_Core/dfg/DFGHeapLocation.h (266812 => 266813)


--- trunk/Source/_javascript_Core/dfg/DFGHeapLocation.h	2020-09-10 03:40:23 UTC (rev 266812)
+++ trunk/Source/_javascript_Core/dfg/DFGHeapLocation.h	2020-09-10 04:47:38 UTC (rev 266813)
@@ -49,10 +49,12 @@
     HasIndexedPropertyLoc,
     IndexedPropertyDoubleLoc,
     IndexedPropertyDoubleSaneChainLoc,
-    IndexedPropertyDoubleOrOtherSaneChainLoc,
+    IndexedPropertyDoubleOutOfBoundsSaneChainLoc,
+    IndexedPropertyDoubleOrOtherOutOfBoundsSaneChainLoc,
     IndexedPropertyInt32Loc,
-    IndexedPropertyInt32OrOtherLoc,
+    IndexedPropertyInt32OutOfBoundsSaneChainLoc,
     IndexedPropertyInt52Loc,
+    IndexedPropertyJSOutOfBoundsSaneChainLoc,
     IndexedPropertyJSLoc,
     IndexedPropertyStorageLoc,
     InvalidationPointLoc,

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to