[JSC] OSR entry should respect abstract values in addition to flush formats
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 Mar 2019 02:34:28 +0000 (02:34 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 Mar 2019 02:34:28 +0000 (02:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195653

Reviewed by Mark Lam.

JSTests:

* stress/osr-entry-locals-none.js: Added.

Source/JavaScriptCore:

Let's consider the following graph.

Block #0
    ...
    27:< 2:loc13> JSConstant(JS|UseAsOther, StringIdent, Strong:String (atomic) (identifier): , StructureID: 42679, bc#10, ExitValid)
    ...
    28:< 2:loc13> ArithPow(DoubleRep:@437<Double>, Int32:@27, Double|UseAsOther, BytecodeDouble, Exits, bc#10, ExitValid)
    29:<!0:->     MovHint(DoubleRep:@28<Double>, MustGen, loc7, W:SideState, ClobbersExit, bc#10, ExitValid)
    30:< 1:->     SetLocal(DoubleRep:@28<Double>, loc7(M<Double>/FlushedDouble), machine:loc6, W:Stack(-8), bc#10, exit: bc#14, ExitValid)  predicting BytecodeDouble
    ...
    73:<!0:->     Jump(MustGen, T:#1, W:SideState, bc#71, ExitValid)

Block #1 (bc#71): (OSR target) pred, #0
    ...
   102:<!2:loc15> GetLocal(Check:Untyped:@400, Double|MustGen|PureInt, BytecodeDouble, loc7(M<Double>/FlushedDouble), machine:loc6, R:Stack(-8), bc#120, ExitValid)  predicting BytecodeDouble
    ...

CFA at @28 says it is invalid since there are type contradiction (Int32:@27 v.s. StringIdent). So, of course, we do not propagate #0's type information to #1 since we become invalid state.
However, #1 is still reachable since it is an OSR target. Since #0 was only the predecessor of #1, loc7's type information becomes None at the head of #1.
Since loc7's AbstractValue is None, @102 GetLocal emits breakpoint. It is OK as long as OSR entry fails because AbstractValue validation requires the given value is None type.

The issue here is that we skipped AbstractValue validation when we have FlushFormat information. Since loc7 has FlushedDouble format, DFG OSR entry code does not validate it against AbstractValue,
which is None. Then, we hit the breakpoint emitted by @102.

This patch performs AbstractValue validation against values even if we have FlushFormat. We should correctly configure AbstractValue for OSR entry's locals too to avoid unnecessary OSR entry
failures in the future but anyway validating locals with AbstractValue is correct behavior here since DFGSpeculativeJIT relies on that.

* dfg/DFGOSREntry.cpp:
(JSC::DFG::prepareOSREntry):

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

JSTests/ChangeLog
JSTests/stress/osr-entry-locals-none.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGOSREntry.cpp

index 1d0fe28..a6300d9 100644 (file)
@@ -1,3 +1,12 @@
+2019-03-12  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] OSR entry should respect abstract values in addition to flush formats
+        https://bugs.webkit.org/show_bug.cgi?id=195653
+
+        Reviewed by Mark Lam.
+
+        * stress/osr-entry-locals-none.js: Added.
+
 2019-03-12  Michael Saboff  <msaboff@apple.com>
 
         REGRESSION (iOS 12.2): Webpage using CoffeeScript crashes
diff --git a/JSTests/stress/osr-entry-locals-none.js b/JSTests/stress/osr-entry-locals-none.js
new file mode 100644 (file)
index 0000000..2f75dbb
--- /dev/null
@@ -0,0 +1,33 @@
+//@ runDefault("--jitPolicyScale=0")
+let obj = {
+    p: [{
+        q: [{
+            x: 0,
+            w: [{
+                y: [{
+                    a: [ {
+                        b: [{
+                            c: [{
+                                d: [{}, {}, {}]
+                            }]
+                        }]
+                    }]
+                }, {}, {}]
+            }]
+        }]
+    }]
+};
+
+function dumpObj(obj) {
+    let output = '' ** ''
+    mallocInALoop();
+    mallocInALoop();
+    for (let item in obj) {
+        let child = obj[item];
+        output += dumpObj(child);
+    }
+    return output;
+}
+noInline(dumpObj);
+
+dumpObj(obj);
index 08bdaaf..d1b8c12 100644 (file)
@@ -1,3 +1,40 @@
+2019-03-12  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] OSR entry should respect abstract values in addition to flush formats
+        https://bugs.webkit.org/show_bug.cgi?id=195653
+
+        Reviewed by Mark Lam.
+
+        Let's consider the following graph.
+
+        Block #0
+            ...
+            27:< 2:loc13> JSConstant(JS|UseAsOther, StringIdent, Strong:String (atomic) (identifier): , StructureID: 42679, bc#10, ExitValid)
+            ...
+            28:< 2:loc13> ArithPow(DoubleRep:@437<Double>, Int32:@27, Double|UseAsOther, BytecodeDouble, Exits, bc#10, ExitValid)
+            29:<!0:->     MovHint(DoubleRep:@28<Double>, MustGen, loc7, W:SideState, ClobbersExit, bc#10, ExitValid)
+            30:< 1:->     SetLocal(DoubleRep:@28<Double>, loc7(M<Double>/FlushedDouble), machine:loc6, W:Stack(-8), bc#10, exit: bc#14, ExitValid)  predicting BytecodeDouble
+            ...
+            73:<!0:->     Jump(MustGen, T:#1, W:SideState, bc#71, ExitValid)
+
+        Block #1 (bc#71): (OSR target) pred, #0
+            ...
+           102:<!2:loc15> GetLocal(Check:Untyped:@400, Double|MustGen|PureInt, BytecodeDouble, loc7(M<Double>/FlushedDouble), machine:loc6, R:Stack(-8), bc#120, ExitValid)  predicting BytecodeDouble
+            ...
+
+        CFA at @28 says it is invalid since there are type contradiction (Int32:@27 v.s. StringIdent). So, of course, we do not propagate #0's type information to #1 since we become invalid state.
+        However, #1 is still reachable since it is an OSR target. Since #0 was only the predecessor of #1, loc7's type information becomes None at the head of #1.
+        Since loc7's AbstractValue is None, @102 GetLocal emits breakpoint. It is OK as long as OSR entry fails because AbstractValue validation requires the given value is None type.
+
+        The issue here is that we skipped AbstractValue validation when we have FlushFormat information. Since loc7 has FlushedDouble format, DFG OSR entry code does not validate it against AbstractValue,
+        which is None. Then, we hit the breakpoint emitted by @102.
+
+        This patch performs AbstractValue validation against values even if we have FlushFormat. We should correctly configure AbstractValue for OSR entry's locals too to avoid unnecessary OSR entry
+        failures in the future but anyway validating locals with AbstractValue is correct behavior here since DFGSpeculativeJIT relies on that.
+
+        * dfg/DFGOSREntry.cpp:
+        (JSC::DFG::prepareOSREntry):
+
 2019-03-12  Michael Saboff  <msaboff@apple.com>
 
         REGRESSION (iOS 12.2): Webpage using CoffeeScript crashes
index d1f69ab..cbf2bac 100644 (file)
@@ -203,38 +203,39 @@ void* prepareOSREntry(ExecState* exec, CodeBlock* codeBlock, unsigned bytecodeIn
     
     for (size_t local = 0; local < entry->m_expectedValues.numberOfLocals(); ++local) {
         int localOffset = virtualRegisterForLocal(local).offset();
+        JSValue value = exec->registers()[localOffset].asanUnsafeJSValue();
+        if (!entry->m_expectedValues.local(local).validate(value)) {
+            if (Options::verboseOSR()) {
+                dataLog(
+                    "    OSR failed because variable ", VirtualRegister(localOffset), " is ",
+                    value, ", expected ",
+                    entry->m_expectedValues.local(local), ".\n");
+            }
+            return 0;
+        }
         if (entry->m_localsForcedDouble.get(local)) {
-            if (!exec->registers()[localOffset].asanUnsafeJSValue().isNumber()) {
+            if (!value.isNumber()) {
                 if (Options::verboseOSR()) {
                     dataLog(
                         "    OSR failed because variable ", localOffset, " is ",
-                        exec->registers()[localOffset].asanUnsafeJSValue(), ", expected number.\n");
+                        value, ", expected number.\n");
                 }
                 return 0;
             }
             continue;
         }
         if (entry->m_localsForcedAnyInt.get(local)) {
-            if (!exec->registers()[localOffset].asanUnsafeJSValue().isAnyInt()) {
+            if (!value) {
                 if (Options::verboseOSR()) {
                     dataLog(
                         "    OSR failed because variable ", localOffset, " is ",
-                        exec->registers()[localOffset].asanUnsafeJSValue(), ", expected ",
+                        value, ", expected ",
                         "machine int.\n");
                 }
                 return 0;
             }
             continue;
         }
-        if (!entry->m_expectedValues.local(local).validate(exec->registers()[localOffset].asanUnsafeJSValue())) {
-            if (Options::verboseOSR()) {
-                dataLog(
-                    "    OSR failed because variable ", VirtualRegister(localOffset), " is ",
-                    exec->registers()[localOffset].asanUnsafeJSValue(), ", expected ",
-                    entry->m_expectedValues.local(local), ".\n");
-            }
-            return 0;
-        }
     }
 
     // 2) Check the stack height. The DFG JIT may require a taller stack than the