Changeset 232134 in webkit

Timestamp:

May 23, 2018, 4:04:58 PM (8 years ago)

Author:

keith_miller@apple.com

Message:

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):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• dfg/DFGInPlaceAbstractState.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp

@@ -237 +237 @@
             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: