Changeset 248185 in webkit

Timestamp:

Aug 2, 2019 3:20:54 PM (5 years ago)

Author:

ysuzuki@apple.com

Message:

[JSC] LazyJSValue should be robust for empty JSValue
​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):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/switch-constant-child-becomes-empty.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/dfg/DFGLazyJSValue.cpp (modified) (8 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/dfg/DFGLazyJSValue.cpp

@@ -101 +101 @@
         return nullptr;
 
-    default:
+    case SingleCharacterString:
         return nullptr;
     }
+    RELEASE_ASSERT_NOT_REACHED();
+    return nullptr;
 }
 
@@ -115 +117 @@
         return String(&u.character, 1);
 
-    default:
+    case KnownValue:
+    case KnownStringImpl:
         if (const StringImpl* string = tryGetStringImpl(graph.m_vm)) {
             unsigned ginormousStringLength = 10000;
@@ -129 +132 @@
         return String();
     }
+    RELEASE_ASSERT_NOT_REACHED();
+    return String();
 }
 
@@ -136 +141 @@
     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;
@@ -154 +168 @@
                 return FalseTriState;
             return triState(other.stringImpl()->at(0) == character());
-        default:
+        case KnownValue:
             return other.strictEqual(*this);
         }
@@ -164 +178 @@
         case NewStringImpl:
             return triState(WTF::equal(stringImpl(), other.stringImpl()));
-        default:
+        case SingleCharacterString:
+        case KnownValue:
             return other.strictEqual(*this);
         }
@@ -182 +197 @@
         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();
@@ -197 +216 @@
             return 0;
         }
-    default:
+    case KnownStringImpl:
+    case NewStringImpl:
         RELEASE_ASSERT_NOT_REACHED();
         return 0;
     }
+    RELEASE_ASSERT_NOT_REACHED();
+    return 0;
 }