Changeset 238511 in webkit

Timestamp:

Nov 26, 2018 12:29:33 PM (5 years ago)

Author:

sbarati@apple.com

Message:

InPlaceAbstractState::endBasicBlock rule for SetLocal should filter the value based on the flush format
​https://bugs.webkit.org/show_bug.cgi?id=191956
<rdar://problem/45665806>

Reviewed by Yusuke Suzuki.

JSTests:

• stress/end-basic-block-set-local-should-filter-type.js: Added.

(bar):
(foo):

Source/JavaScriptCore:

This is a similar bug to what Keith fixed in r232134. The issue is if we have
a program like this:

a: JSConstant(jsNumber(0))
b: SetLocal(Int32:@a, loc1, FlushedInt32)
c: ArrayifyToStructure(Cell:@a)
d: Jump(...)

At the point in the program right after the Jump, a GetLocal for loc1
would return whatever the ArrayifyToStructure resulting type is. This breaks
the invariant that a GetLocal must return a value that is a subtype of its
FlushFormat. InPlaceAbstractState::endBasicBlock will know if a SetLocal is
the final node touching a local slot. If so, it'll see if any nodes later
in the block may have refined the type of the value stored in that slot. If
so, endBasicBlock() further refines the type to ensure that any GetLocals
loading from the same slot will result in having this more refined type.
However, we must ensure that this logic only considers types within the
hierarchy of the variable access data's FlushFormat, otherwise, we may
break the invariant that a GetLocal's type is a subtype of its FlushFormat.

• dfg/DFGInPlaceAbstractState.cpp:

(JSC::DFG::InPlaceAbstractState::endBasicBlock):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/end-basic-block-set-local-should-filter-type.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp (modified) (1 diff)

trunk/JSTests/ChangeLog

@@ +1 @@
+2018-11-26  Saam barati  <sbarati@apple.com>
+
+        InPlaceAbstractState::endBasicBlock rule for SetLocal should filter the value based on the flush format
+        https://bugs.webkit.org/show_bug.cgi?id=191956
+        <rdar://problem/45665806>
+
+        Reviewed by Yusuke Suzuki.
+
+        * stress/end-basic-block-set-local-should-filter-type.js: Added.
+        (bar):
+        (foo):
+
 2018-11-26  Saam barati  <sbarati@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2018-11-26  Saam barati  <sbarati@apple.com>
+
+        InPlaceAbstractState::endBasicBlock rule for SetLocal should filter the value based on the flush format
+        https://bugs.webkit.org/show_bug.cgi?id=191956
+        <rdar://problem/45665806>
+
+        Reviewed by Yusuke Suzuki.
+
+        This is a similar bug to what Keith fixed in r232134. The issue is if we have
+        a program like this:
+        
+        a: JSConstant(jsNumber(0))
+        b: SetLocal(Int32:@a, loc1, FlushedInt32)
+        c: ArrayifyToStructure(Cell:@a)
+        d: Jump(...)
+        
+        At the point in the program right after the Jump, a GetLocal for loc1
+        would return whatever the ArrayifyToStructure resulting type is. This breaks
+        the invariant that a GetLocal must return a value that is a subtype of its
+        FlushFormat. InPlaceAbstractState::endBasicBlock will know if a SetLocal is
+        the final node touching a local slot. If so, it'll see if any nodes later
+        in the block may have refined the type of the value stored in that slot. If
+        so, endBasicBlock() further refines the type to ensure that any GetLocals
+        loading from the same slot will result in having this more refined type.
+        However, we must ensure that this logic only considers types within the
+        hierarchy of the variable access data's FlushFormat, otherwise, we may
+        break the invariant that a GetLocal's type is a subtype of its FlushFormat.
+
+        * dfg/DFGInPlaceAbstractState.cpp:
+        (JSC::DFG::InPlaceAbstractState::endBasicBlock):
+
 2018-11-26  Saam barati  <sbarati@apple.com>
 

trunk/Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp

@@ -266 +266 @@
             case SetLocal: {
                 // The block sets the variable, and potentially refines it, both
-                // before and after setting it.
-                destination = forNode(node->child1());
+                // before and after setting it. Since the SetLocal already did
+                // a type check based on the flush format's type, we're only interested
+                // in refinements within that type hierarchy. Otherwise, we may end up
+                // saying that any GetLocals reachable from this basic block load something
+                // outside of that hierarchy, e.g:
+                //
+                // a: JSConstant(jsNumber(0))
+                // b: SetLocal(Int32:@a, loc1, FlushedInt32)
+                // c: ArrayifyToStructure(Cell:@a)
+                // d: Jump(...)
+                //
+                // In this example, we can't trust whatever type ArrayifyToStructure sets
+                // @a to. We're only interested in the subset of that type that intersects
+                // with Int32.
+                AbstractValue value = forNode(node->child1());
+                value.filter(typeFilterFor(node->variableAccessData()->flushFormat()));
+                destination = value;
                 break;
             }