InPlaceAbstractState::endBasicBlock rule for SetLocal should filter the value based...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 26 Nov 2018 20:29:33 +0000 (20:29 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 26 Nov 2018 20:29:33 +0000 (20:29 +0000)
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):

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

JSTests/ChangeLog
JSTests/stress/end-basic-block-set-local-should-filter-type.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp

index 0d2fdb9..353d9cf 100644 (file)
@@ -1,5 +1,17 @@
 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>
+
         Object allocation sinking phase needs to iterate each scope offset instead of just iterating the symbol table's hashmap when handling an activation
         https://bugs.webkit.org/show_bug.cgi?id=191958
         <rdar://problem/46221877>
diff --git a/JSTests/stress/end-basic-block-set-local-should-filter-type.js b/JSTests/stress/end-basic-block-set-local-should-filter-type.js
new file mode 100644 (file)
index 0000000..cdd0035
--- /dev/null
@@ -0,0 +1,16 @@
+function bar() {
+    let x = 0;
+    foo(0);
+    if (x) {
+    }
+}
+function foo(a) {
+    let x = a[0]
+    a[0] = 0;
+    return;
+    a
+}
+foo([0]);
+for (var i = 0; i < 10000; ++i) {
+    bar();
+}
index d7878fb..401caff 100644 (file)
@@ -1,5 +1,36 @@
 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>
+
         Object allocation sinking phase needs to iterate each scope offset instead of just iterating the symbol table's hashmap when handling an activation
         https://bugs.webkit.org/show_bug.cgi?id=191958
         <rdar://problem/46221877>
index 550c97a..115283a 100644 (file)
@@ -265,8 +265,23 @@ bool InPlaceAbstractState::endBasicBlock()
             }
             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;
             }