intersectionOfPastValuesAtHead must filter values after they've observed an invalidat...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 18 Aug 2018 02:05:09 +0000 (02:05 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 18 Aug 2018 02:05:09 +0000 (02:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=188707
<rdar://problem/43015442>

Reviewed by Mark Lam.

JSTests:

* stress/cfa-expected-values-must-set-clobbered-to-false.js: Added.
(foo):
(let.comp.valueOf):
(result):

Source/JavaScriptCore:

We use the values in intersectionOfPastValuesAtHead to verify that it is safe to
OSR enter at the head of a block. We verify it's safe to OSR enter by checking
that each incoming value is compatible with its corresponding AbstractValue.

The bug is that we were sometimes filtering the intersectionOfPastValuesAtHead
with abstract values that were clobbererd. This meant that the value we're
verifying with at OSR entry effectively has an infinite structure set because
it's clobbered. So, imagine we have code like this:
```
---> We OSR enter here, and we're clobbered here
InvalidationPoint
GetByOffset(@base)
```

The abstract value for @base inside intersectionOfPastValuesAtHead has a
clobberred structure set, so we'd allow an incoming object with any
structure. However, this is wrong because the invalidation point is no
longer fulfilling its promise that it filters the structure that @base has.

We fix this by filtering the AbstractValues in intersectionOfPastValuesAtHead
as if the incoming value may be live past an InvalidationPoint.
This places a stricter requirement that to safely OSR enter at any basic
block, all incoming values must be compatible as if they lived past
the execution of an invalidation point.

* dfg/DFGCFAPhase.cpp:
(JSC::DFG::CFAPhase::run):

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

JSTests/ChangeLog
JSTests/stress/cfa-expected-values-must-set-clobbered-to-false.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGCFAPhase.cpp

index 88c2011..49011a0 100644 (file)
@@ -1,3 +1,16 @@
+2018-08-17  Saam barati  <sbarati@apple.com>
+
+        intersectionOfPastValuesAtHead must filter values after they've observed an invalidation point
+        https://bugs.webkit.org/show_bug.cgi?id=188707
+        <rdar://problem/43015442>
+
+        Reviewed by Mark Lam.
+
+        * stress/cfa-expected-values-must-set-clobbered-to-false.js: Added.
+        (foo):
+        (let.comp.valueOf):
+        (result):
+
 2018-08-10  Keith Miller  <keith_miller@apple.com>
 
         Slicing an ArrayBuffer with a long number returns an ArrayBuffer with byteLength zero
diff --git a/JSTests/stress/cfa-expected-values-must-set-clobbered-to-false.js b/JSTests/stress/cfa-expected-values-must-set-clobbered-to-false.js
new file mode 100644 (file)
index 0000000..99f282e
--- /dev/null
@@ -0,0 +1,38 @@
+//@ runDefault("--useFTLJIT=0", "--useConcurrentJIT=false")
+
+let num = 150;
+
+function foo(comp, o, b) {
+    let sum = o.f;
+    if (b)
+        OSRExit();
+    for (let i = 0; i < comp; ++i) {
+        sum += o.f;
+    }
+    return sum;
+}
+noInline(foo);
+
+let o = {f:25};
+let o2 = {f:25, g:44};
+o2.f = 45;
+o2.f = 45;
+o2.f = 45;
+o2.f = 45;
+let comp = {
+    valueOf() { return num; }
+}
+
+foo(comp, o2, true);
+foo(comp, o2, true);
+for (let i = 0; i < 500; ++i) {
+    foo(comp, o2, false);
+}
+
+let o3 = {g:24, f:73};
+num = 10000000;
+let result = foo(comp, o3, false);
+
+if (result !== (num + 1)*73) {
+    throw new Error("Bad: " + result);
+}
index 48a6aae..a474cc1 100644 (file)
@@ -1,3 +1,39 @@
+2018-08-17  Saam barati  <sbarati@apple.com>
+
+        intersectionOfPastValuesAtHead must filter values after they've observed an invalidation point
+        https://bugs.webkit.org/show_bug.cgi?id=188707
+        <rdar://problem/43015442>
+
+        Reviewed by Mark Lam.
+
+        We use the values in intersectionOfPastValuesAtHead to verify that it is safe to
+        OSR enter at the head of a block. We verify it's safe to OSR enter by checking
+        that each incoming value is compatible with its corresponding AbstractValue.
+        
+        The bug is that we were sometimes filtering the intersectionOfPastValuesAtHead
+        with abstract values that were clobbererd. This meant that the value we're
+        verifying with at OSR entry effectively has an infinite structure set because
+        it's clobbered. So, imagine we have code like this:
+        ```
+        ---> We OSR enter here, and we're clobbered here
+        InvalidationPoint
+        GetByOffset(@base)
+        ```
+        
+        The abstract value for @base inside intersectionOfPastValuesAtHead has a
+        clobberred structure set, so we'd allow an incoming object with any
+        structure. However, this is wrong because the invalidation point is no
+        longer fulfilling its promise that it filters the structure that @base has.
+        
+        We fix this by filtering the AbstractValues in intersectionOfPastValuesAtHead
+        as if the incoming value may be live past an InvalidationPoint.
+        This places a stricter requirement that to safely OSR enter at any basic
+        block, all incoming values must be compatible as if they lived past
+        the execution of an invalidation point.
+
+        * dfg/DFGCFAPhase.cpp:
+        (JSC::DFG::CFAPhase::run):
+
 2018-08-17  Yusuke Suzuki  <yusukesuzuki@slowstart.org> and Fujii Hironori  <Hironori.Fujii@sony.com>
 
         [JSC] Add GPRReg::InvalidGPRReg and FPRReg::InvalidFPRReg
index fa9c1bc..27232f8 100644 (file)
@@ -141,8 +141,15 @@ public:
                     continue;
                 
                 block->intersectionOfCFAHasVisited &= block->cfaHasVisited;
-                for (unsigned i = block->intersectionOfPastValuesAtHead.size(); i--;)
-                    block->intersectionOfPastValuesAtHead[i].filter(block->valuesAtHead[i]);
+                for (unsigned i = block->intersectionOfPastValuesAtHead.size(); i--;) {
+                    AbstractValue value = block->valuesAtHead[i];
+                    // We need to guarantee that when we do an OSR entry, we validate the incoming
+                    // value as if it could be live past an invalidation point. Otherwise, we may
+                    // OSR enter with a value with the wrong structure, and an InvalidationPoint's
+                    // promise of filtering the structure set of certain values is no longer upheld.
+                    value.m_structure.observeInvalidationPoint();
+                    block->intersectionOfPastValuesAtHead[i].filter(value);
+                }
             }
         }