[JSC] LazyJSValue should be robust for empty JSValue
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Aug 2019 22:20:54 +0000 (22:20 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 2 Aug 2019 22:20:54 +0000 (22:20 +0000)
https://bugs.webkit.org/show_bug.cgi?id=200388

Reviewed by Saam Barati.

JSTests:

* stress/switch-constant-child-becomes-empty.js: Added.
(foo):

Source/JavaScriptCore:

If the Switch DFG node is preceded by ForceOSRExit or something that invalidates the basic block,
it can take a FrozenValue as a child which includes empty value instead of string, number etc.
If this Switch node is kept and we reached to DFGCFGSimplificationPhase, it will use this FrozenValue.
However, LazyJSValue using this FrozenValue strongly assumes that FrozenValue is never holding empty value.
But this assumption is wrong. This patch makes LazyJSValue robust for empty value.

* dfg/DFGLazyJSValue.cpp:
(JSC::DFG::LazyJSValue::tryGetStringImpl const):
(JSC::DFG::LazyJSValue::tryGetString const):
(JSC::DFG::LazyJSValue::strictEqual const):
(JSC::DFG::LazyJSValue::switchLookupValue const):

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

JSTests/ChangeLog
JSTests/stress/switch-constant-child-becomes-empty.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGLazyJSValue.cpp

index 5a95477..a9bef96 100644 (file)
@@ -1,3 +1,13 @@
+2019-08-02  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] LazyJSValue should be robust for empty JSValue
+        https://bugs.webkit.org/show_bug.cgi?id=200388
+
+        Reviewed by Saam Barati.
+
+        * stress/switch-constant-child-becomes-empty.js: Added.
+        (foo):
+
 2019-08-01  Yusuke Suzuki  <ysuzuki@apple.com>
 
         GetterSetter type confusion during DFG compilation
diff --git a/JSTests/stress/switch-constant-child-becomes-empty.js b/JSTests/stress/switch-constant-child-becomes-empty.js
new file mode 100644 (file)
index 0000000..736df17
--- /dev/null
@@ -0,0 +1,17 @@
+//@ runDefault("--useConcurrentJIT=0")
+function foo(x) {
+    switch (x) {
+    case "a":
+    case "a":
+    case "a":
+        for (let j = 0; j <100; j++) {
+            let j=foo(j);
+        }
+    default:
+        return 2;
+    }
+}
+
+for (let i = 0; i <100000; i++) {
+    foo("ab");
+}
index 18d1a31..64bf3b3 100644 (file)
@@ -1,3 +1,22 @@
+2019-08-02  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] LazyJSValue should be robust for empty JSValue
+        https://bugs.webkit.org/show_bug.cgi?id=200388
+
+        Reviewed by Saam Barati.
+
+        If the Switch DFG node is preceded by ForceOSRExit or something that invalidates the basic block,
+        it can take a FrozenValue as a child which includes empty value instead of string, number etc.
+        If this Switch node is kept and we reached to DFGCFGSimplificationPhase, it will use this FrozenValue.
+        However, LazyJSValue using this FrozenValue strongly assumes that FrozenValue is never holding empty value.
+        But this assumption is wrong. This patch makes LazyJSValue robust for empty value.
+
+        * dfg/DFGLazyJSValue.cpp:
+        (JSC::DFG::LazyJSValue::tryGetStringImpl const):
+        (JSC::DFG::LazyJSValue::tryGetString const):
+        (JSC::DFG::LazyJSValue::strictEqual const):
+        (JSC::DFG::LazyJSValue::switchLookupValue const):
+
 2019-08-02  Devin Rousso  <drousso@apple.com>
 
         Web Inspector: Storage: disable related agents when the tab is closed
index 583ee5b..2458c46 100644 (file)
@@ -100,9 +100,11 @@ const StringImpl* LazyJSValue::tryGetStringImpl(VM& vm) const
             return string->tryGetValueImpl();
         return nullptr;
 
-    default:
+    case SingleCharacterString:
         return nullptr;
     }
+    RELEASE_ASSERT_NOT_REACHED();
+    return nullptr;
 }
 
 String LazyJSValue::tryGetString(Graph& graph) const
@@ -114,7 +116,8 @@ String LazyJSValue::tryGetString(Graph& graph) const
     case SingleCharacterString:
         return String(&u.character, 1);
 
-    default:
+    case KnownValue:
+    case KnownStringImpl:
         if (const StringImpl* string = tryGetStringImpl(graph.m_vm)) {
             unsigned ginormousStringLength = 10000;
             if (string->length() > ginormousStringLength)
@@ -128,6 +131,8 @@ String LazyJSValue::tryGetString(Graph& graph) const
         
         return String();
     }
+    RELEASE_ASSERT_NOT_REACHED();
+    return String();
 }
 
 TriState LazyJSValue::strictEqual(const LazyJSValue& other) const
@@ -135,14 +140,23 @@ TriState LazyJSValue::strictEqual(const LazyJSValue& other) const
     switch (m_kind) {
     case KnownValue:
         switch (other.m_kind) {
-        case KnownValue:
+        case KnownValue: {
+            if (!value()->value() || !other.value()->value())
+                return value()->value() == other.value()->value() ? TrueTriState : FalseTriState;
             return JSValue::pureStrictEqual(value()->value(), other.value()->value());
-        case SingleCharacterString:
+        }
+        case SingleCharacterString: {
+            if (!value()->value())
+                return FalseTriState;
             return equalToSingleCharacter(value()->value(), other.character());
+        }
         case KnownStringImpl:
-        case NewStringImpl:
+        case NewStringImpl: {
+            if (!value()->value())
+                return FalseTriState;
             return equalToStringImpl(value()->value(), other.stringImpl());
         }
+        }
         break;
     case SingleCharacterString:
         switch (other.m_kind) {
@@ -153,7 +167,7 @@ TriState LazyJSValue::strictEqual(const LazyJSValue& other) const
             if (other.stringImpl()->length() != 1)
                 return FalseTriState;
             return triState(other.stringImpl()->at(0) == character());
-        default:
+        case KnownValue:
             return other.strictEqual(*this);
         }
         break;
@@ -163,7 +177,8 @@ TriState LazyJSValue::strictEqual(const LazyJSValue& other) const
         case KnownStringImpl:
         case NewStringImpl:
             return triState(WTF::equal(stringImpl(), other.stringImpl()));
-        default:
+        case SingleCharacterString:
+        case KnownValue:
             return other.strictEqual(*this);
         }
         break;
@@ -181,9 +196,13 @@ uintptr_t LazyJSValue::switchLookupValue(SwitchKind kind) const
     case KnownValue:
         switch (kind) {
         case SwitchImm:
-            return value()->value().asInt32();
+            if (value()->value())
+                return value()->value().asInt32();
+            return 0;
         case SwitchCell:
-            return bitwise_cast<uintptr_t>(value()->value().asCell());
+            if (value()->value())
+                return bitwise_cast<uintptr_t>(value()->value().asCell());
+            return 0;
         default:
             RELEASE_ASSERT_NOT_REACHED();
             return 0;
@@ -196,10 +215,13 @@ uintptr_t LazyJSValue::switchLookupValue(SwitchKind kind) const
             RELEASE_ASSERT_NOT_REACHED();
             return 0;
         }
-    default:
+    case KnownStringImpl:
+    case NewStringImpl:
         RELEASE_ASSERT_NOT_REACHED();
         return 0;
     }
+    RELEASE_ASSERT_NOT_REACHED();
+    return 0;
 }
 
 void LazyJSValue::emit(CCallHelpers& jit, JSValueRegs result) const