InPlaceAbstractState should filter variables at the tail from a GetLocal by their...
authorkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 23 May 2018 23:04:58 +0000 (23:04 +0000)
committerkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 23 May 2018 23:04:58 +0000 (23:04 +0000)
https://bugs.webkit.org/show_bug.cgi?id=185923

Reviewed by Saam Barati.

Previously, we could confuse AI by overly broadening a type. This happens when a block in a
loop has a local mutated following a GetLocal but never SetLocaled to the stack. For example,

Block 1:
@1: GetLocal(loc42, FlushedInt32);
@2: PutStructure(Check: Cell: @1);
@3: Jump(Block 1);

Would cause us to claim that loc42 could be either an int32 or a some cell. However,
the type of an local cannot change without writing to it.

This fixes a crash in destructuring-rest-element.js

* dfg/DFGInPlaceAbstractState.cpp:
(JSC::DFG::InPlaceAbstractState::endBasicBlock):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp

index c5d066a..217fdeb 100644 (file)
@@ -1,3 +1,26 @@
+2018-05-23  Keith Miller  <keith_miller@apple.com>
+
+        InPlaceAbstractState should filter variables at the tail from a GetLocal by their flush format
+        https://bugs.webkit.org/show_bug.cgi?id=185923
+
+        Reviewed by Saam Barati.
+
+        Previously, we could confuse AI by overly broadening a type. This happens when a block in a
+        loop has a local mutated following a GetLocal but never SetLocaled to the stack. For example,
+
+        Block 1:
+        @1: GetLocal(loc42, FlushedInt32);
+        @2: PutStructure(Check: Cell: @1);
+        @3: Jump(Block 1);
+
+        Would cause us to claim that loc42 could be either an int32 or a some cell. However,
+        the type of an local cannot change without writing to it.
+
+        This fixes a crash in destructuring-rest-element.js
+
+        * dfg/DFGInPlaceAbstractState.cpp:
+        (JSC::DFG::InPlaceAbstractState::endBasicBlock):
+
 2018-05-23  Filip Pizlo  <fpizlo@apple.com>
 
         Speed up JetStream/base64
index 8d4a656..550c97a 100644 (file)
@@ -236,21 +236,39 @@ bool InPlaceAbstractState::endBasicBlock()
             case Phi:
             case SetArgument:
             case PhantomLocal:
-            case Flush:
+            case Flush: {
                 // The block transfers the value from head to tail.
                 destination = variableAt(index);
                 break;
+            }
                 
-            case GetLocal:
+            case GetLocal: {
                 // The block refines the value with additional speculations.
                 destination = forNode(node);
+
+                // We need to make sure that we don't broaden the type beyond what the flush
+                // format says it will be. The value may claim to have changed abstract state
+                // but it's type cannot change without a store. For example:
+                //
+                // Block #1:
+                // 0: GetLocal(loc42, FlushFormatInt32)
+                // 1: PutStructure(Check: Cell: @0, ArrayStructure)
+                // ...
+                // 2: Branch(T: #1, F: #2)
+                //
+                // In this case the AbstractState of @0 will say it's an SpecArray but the only
+                // reason that would have happened is because we would have exited the cell check.
+
+                FlushFormat flushFormat = node->variableAccessData()->flushFormat();
+                destination.filter(typeFilterFor(flushFormat));
                 break;
-                
-            case SetLocal:
+            }
+            case SetLocal: {
                 // The block sets the variable, and potentially refines it, both
                 // before and after setting it.
                 destination = forNode(node->child1());
                 break;
+            }
                 
             default:
                 RELEASE_ASSERT_NOT_REACHED();