Fix endless OSR exits when creating a rope that contains an object that ToPrimitive...
authorkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 4 Nov 2015 21:46:10 +0000 (21:46 +0000)
committerkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 4 Nov 2015 21:46:10 +0000 (21:46 +0000)
https://bugs.webkit.org/show_bug.cgi?id=150583

Reviewed by Benjamin Poulain.

Source/JavaScriptCore:

Before we assumed that the result of ToPrimitive on any object was a string.
This had a couple of negative effects. First, the result ToPrimitive on an
object can be overridden to be any primitive type. In fact, as of ES6, ToPrimitive,
when part of a addition expression, will type hint a number value. Second, even after
repeatedly exiting with a bad type we would continue to think that the result
of ToPrimitive would be a string so we continue to convert StrCats into MakeRope.

The fix is to make Prediction Propagation match the behavior of Fixup and move
canOptimizeStringObjectAccess to DFGGraph.

* bytecode/SpeculatedType.h:
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::attemptToForceStringArrayModeByToStringConversion):
(JSC::DFG::FixupPhase::fixupToPrimitive):
(JSC::DFG::FixupPhase::fixupToStringOrCallStringConstructor):
(JSC::DFG::FixupPhase::attemptToMakeFastStringAdd):
(JSC::DFG::FixupPhase::isStringPrototypeMethodSane): Deleted.
(JSC::DFG::FixupPhase::canOptimizeStringObjectAccess): Deleted.
* dfg/DFGGraph.cpp:
(JSC::DFG::Graph::isStringPrototypeMethodSane):
(JSC::DFG::Graph::canOptimizeStringObjectAccess):
* dfg/DFGGraph.h:
* dfg/DFGPredictionPropagationPhase.cpp:
(JSC::DFG::PredictionPropagationPhase::resultOfToPrimitive):
(JSC::DFG::resultOfToPrimitive): Deleted.

* bytecode/SpeculatedType.h:
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::attemptToForceStringArrayModeByToStringConversion):
(JSC::DFG::FixupPhase::fixupToPrimitive):
(JSC::DFG::FixupPhase::fixupToStringOrCallStringConstructor):
(JSC::DFG::FixupPhase::attemptToMakeFastStringAdd):
(JSC::DFG::FixupPhase::isStringPrototypeMethodSane): Deleted.
(JSC::DFG::FixupPhase::canOptimizeStringObjectAccess): Deleted.
* dfg/DFGGraph.cpp:
(JSC::DFG::Graph::isStringPrototypeMethodSane):
(JSC::DFG::Graph::canOptimizeStringObjectAccess):
* dfg/DFGGraph.h:
* dfg/DFGPredictionPropagationPhase.cpp:
(JSC::DFG::PredictionPropagationPhase::resultOfToPrimitive):
(JSC::DFG::resultOfToPrimitive): Deleted.
* tests/stress/string-rope-with-custom-valueof.js: Added.
(catNumber):
(number.valueOf):
(catBool):
(bool.valueOf):
(catUndefined):
(undef.valueOf):
(catRandom):
(random.valueOf):

LayoutTests:

Created a regression test to look for OSRing in string concatenation when
valueOf returns a non-string primitive.

* js/regress/script-tests/string-rope-with-object.js: Added.
(body.f):
(body.String.prototype.valueOf):
(body.bar.valueOf):
(body):
* js/regress/string-rope-with-object-expected.txt: Added.
* js/regress/string-rope-with-object.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/js/regress/script-tests/string-rope-with-object.js [new file with mode: 0644]
LayoutTests/js/regress/string-rope-with-object-expected.txt [new file with mode: 0644]
LayoutTests/js/regress/string-rope-with-object.html [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/SpeculatedType.h
Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
Source/JavaScriptCore/dfg/DFGGraph.cpp
Source/JavaScriptCore/dfg/DFGGraph.h
Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
Source/JavaScriptCore/tests/stress/string-rope-with-custom-valueof.js [new file with mode: 0644]

index 00068b8..98ce577 100644 (file)
@@ -1,3 +1,21 @@
+2015-10-30  Keith Miller  <keith_miller@apple.com>
+
+        Fix endless OSR exits when creating a rope that contains an object that ToPrimitive's to a number.
+        https://bugs.webkit.org/show_bug.cgi?id=150583
+
+        Reviewed by Benjamin Poulain.
+
+        Created a regression test to look for OSRing in string concatenation when
+        valueOf returns a non-string primitive.
+
+        * js/regress/script-tests/string-rope-with-object.js: Added.
+        (body.f):
+        (body.String.prototype.valueOf):
+        (body.bar.valueOf):
+        (body):
+        * js/regress/string-rope-with-object-expected.txt: Added.
+        * js/regress/string-rope-with-object.html: Added.
+
 2015-11-04  Ryan Haddad  <ryanhaddad@apple.com>
 
         Rebaselining LayoutTest fast/text/international/thai-baht-space.html on win
diff --git a/LayoutTests/js/regress/script-tests/string-rope-with-object.js b/LayoutTests/js/regress/script-tests/string-rope-with-object.js
new file mode 100644 (file)
index 0000000..bce9eb1
--- /dev/null
@@ -0,0 +1,24 @@
+function body() {
+    function f(s,t) {
+        let total = 0;
+        for (let i = 0; i < 2000; i++) {
+            total += i;
+        }
+        return "a" + "b" + s + t;
+    }
+    noInline(f);
+
+    var foo = new String(10);
+    String.prototype.valueOf = function() { return 1; };
+
+    var count = 0;
+    var bar = { valueOf: function() { return count++; } };
+    (function wrapper() {
+        for (var i = 0; i < 10000; i++) {
+            if (f(foo,bar) !== "ab1" + i)
+                throw "bad";
+        }
+    })();
+
+}
+body();
diff --git a/LayoutTests/js/regress/string-rope-with-object-expected.txt b/LayoutTests/js/regress/string-rope-with-object-expected.txt
new file mode 100644 (file)
index 0000000..1fa2b8b
--- /dev/null
@@ -0,0 +1,10 @@
+JSRegress/string-rope-with-object
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS no exception thrown
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/js/regress/string-rope-with-object.html b/LayoutTests/js/regress/string-rope-with-object.html
new file mode 100644 (file)
index 0000000..d4d30d2
--- /dev/null
@@ -0,0 +1,12 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../../resources/js-test-pre.js"></script>
+</head>
+<body>
+<script src="../../resources/regress-pre.js"></script>
+<script src="script-tests/string-rope-with-object.js"></script>
+<script src="../../resources/regress-post.js"></script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
index 0a1e769..7d165ac 100644 (file)
@@ -1,3 +1,61 @@
+2015-10-30  Keith Miller  <keith_miller@apple.com>
+
+        Fix endless OSR exits when creating a rope that contains an object that ToPrimitive's to a number.
+        https://bugs.webkit.org/show_bug.cgi?id=150583
+
+        Reviewed by Benjamin Poulain.
+
+        Before we assumed that the result of ToPrimitive on any object was a string.
+        This had a couple of negative effects. First, the result ToPrimitive on an
+        object can be overridden to be any primitive type. In fact, as of ES6, ToPrimitive,
+        when part of a addition expression, will type hint a number value. Second, even after
+        repeatedly exiting with a bad type we would continue to think that the result
+        of ToPrimitive would be a string so we continue to convert StrCats into MakeRope.
+
+        The fix is to make Prediction Propagation match the behavior of Fixup and move
+        canOptimizeStringObjectAccess to DFGGraph.
+
+        * bytecode/SpeculatedType.h:
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::attemptToForceStringArrayModeByToStringConversion):
+        (JSC::DFG::FixupPhase::fixupToPrimitive):
+        (JSC::DFG::FixupPhase::fixupToStringOrCallStringConstructor):
+        (JSC::DFG::FixupPhase::attemptToMakeFastStringAdd):
+        (JSC::DFG::FixupPhase::isStringPrototypeMethodSane): Deleted.
+        (JSC::DFG::FixupPhase::canOptimizeStringObjectAccess): Deleted.
+        * dfg/DFGGraph.cpp:
+        (JSC::DFG::Graph::isStringPrototypeMethodSane):
+        (JSC::DFG::Graph::canOptimizeStringObjectAccess):
+        * dfg/DFGGraph.h:
+        * dfg/DFGPredictionPropagationPhase.cpp:
+        (JSC::DFG::PredictionPropagationPhase::resultOfToPrimitive):
+        (JSC::DFG::resultOfToPrimitive): Deleted.
+
+        * bytecode/SpeculatedType.h:
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::attemptToForceStringArrayModeByToStringConversion):
+        (JSC::DFG::FixupPhase::fixupToPrimitive):
+        (JSC::DFG::FixupPhase::fixupToStringOrCallStringConstructor):
+        (JSC::DFG::FixupPhase::attemptToMakeFastStringAdd):
+        (JSC::DFG::FixupPhase::isStringPrototypeMethodSane): Deleted.
+        (JSC::DFG::FixupPhase::canOptimizeStringObjectAccess): Deleted.
+        * dfg/DFGGraph.cpp:
+        (JSC::DFG::Graph::isStringPrototypeMethodSane):
+        (JSC::DFG::Graph::canOptimizeStringObjectAccess):
+        * dfg/DFGGraph.h:
+        * dfg/DFGPredictionPropagationPhase.cpp:
+        (JSC::DFG::PredictionPropagationPhase::resultOfToPrimitive):
+        (JSC::DFG::resultOfToPrimitive): Deleted.
+        * tests/stress/string-rope-with-custom-valueof.js: Added.
+        (catNumber):
+        (number.valueOf):
+        (catBool):
+        (bool.valueOf):
+        (catUndefined):
+        (undef.valueOf):
+        (catRandom):
+        (random.valueOf):
+
 2015-11-04  Xabier Rodriguez Calvar  <calvaris@igalia.com>
 
         Remove bogus global internal functions for properties and prototype retrieval
index dac8bcc..63d00b3 100644 (file)
@@ -85,6 +85,7 @@ static const SpeculatedType SpecBoolean            = 0x10000000; // It's definit
 static const SpeculatedType SpecOther              = 0x20000000; // It's definitely either Null or Undefined.
 static const SpeculatedType SpecMisc               = 0x30000000; // It's definitely either a boolean, Null, or Undefined.
 static const SpeculatedType SpecHeapTop            = 0x3bbfffff; // It can be any of the above, except for SpecInt52 and SpecDoubleImpureNaN.
+static const SpeculatedType SpecPrimitive          = 0x3bbf0000; // It's any non-Object JSValue. This is (~SpecObject & SpecHeapTop)
 static const SpeculatedType SpecEmpty              = 0x40000000; // It's definitely an empty value marker.
 static const SpeculatedType SpecBytecodeTop        = 0x7bbfffff; // It can be any of the above, except for SpecInt52 and SpecDoubleImpureNaN. This is (SpecHeapTop | SpecEmpty).
 static const SpeculatedType SpecFullTop            = 0x7fffffff; // It can be any of the above plus anything the DFG chooses.
index 491fbc1..c4f36f9 100644 (file)
@@ -1447,7 +1447,7 @@ private:
     {
         ASSERT(arrayMode == ArrayMode(Array::Generic));
         
-        if (!canOptimizeStringObjectAccess(node->origin.semantic))
+        if (!m_graph.canOptimizeStringObjectAccess(node->origin.semantic))
             return;
         
         createToString<useKind>(node, node->child1());
@@ -1529,14 +1529,14 @@ private:
         }
         
         if (node->child1()->shouldSpeculateStringObject()
-            && canOptimizeStringObjectAccess(node->origin.semantic)) {
+            && m_graph.canOptimizeStringObjectAccess(node->origin.semantic)) {
             fixEdge<StringObjectUse>(node->child1());
             node->convertToToString();
             return;
         }
         
         if (node->child1()->shouldSpeculateStringOrStringObject()
-            && canOptimizeStringObjectAccess(node->origin.semantic)) {
+            && m_graph.canOptimizeStringObjectAccess(node->origin.semantic)) {
             fixEdge<StringOrStringObjectUse>(node->child1());
             node->convertToToString();
             return;
@@ -1552,13 +1552,13 @@ private:
         }
         
         if (node->child1()->shouldSpeculateStringObject()
-            && canOptimizeStringObjectAccess(node->origin.semantic)) {
+            && m_graph.canOptimizeStringObjectAccess(node->origin.semantic)) {
             fixEdge<StringObjectUse>(node->child1());
             return;
         }
         
         if (node->child1()->shouldSpeculateStringOrStringObject()
-            && canOptimizeStringObjectAccess(node->origin.semantic)) {
+            && m_graph.canOptimizeStringObjectAccess(node->origin.semantic)) {
             fixEdge<StringOrStringObjectUse>(node->child1());
             return;
         }
@@ -1577,7 +1577,7 @@ private:
             [&] (Edge& edge) {
                 if (edge->shouldSpeculateString())
                     return;
-                if (canOptimizeStringObjectAccess(node->origin.semantic)) {
+                if (m_graph.canOptimizeStringObjectAccess(node->origin.semantic)) {
                     if (edge->shouldSpeculateStringObject())
                         return;
                     if (edge->shouldSpeculateStringOrStringObject())
@@ -1595,7 +1595,7 @@ private:
                     convertStringAddUse<StringUse>(node, edge);
                     return;
                 }
-                ASSERT(canOptimizeStringObjectAccess(node->origin.semantic));
+                ASSERT(m_graph.canOptimizeStringObjectAccess(node->origin.semantic));
                 if (edge->shouldSpeculateStringObject()) {
                     convertStringAddUse<StringObjectUse>(node, edge);
                     return;
@@ -1610,65 +1610,7 @@ private:
         convertToMakeRope(node);
         return true;
     }
-    
-    bool isStringPrototypeMethodSane(
-        JSObject* stringPrototype, Structure* stringPrototypeStructure, UniquedStringImpl* uid)
-    {
-        unsigned attributesUnused;
-        PropertyOffset offset =
-            stringPrototypeStructure->getConcurrently(uid, attributesUnused);
-        if (!isValidOffset(offset))
-            return false;
-        
-        JSValue value = m_graph.tryGetConstantProperty(
-            stringPrototype, stringPrototypeStructure, offset);
-        if (!value)
-            return false;
-        
-        JSFunction* function = jsDynamicCast<JSFunction*>(value);
-        if (!function)
-            return false;
-        
-        if (function->executable()->intrinsicFor(CodeForCall) != StringPrototypeValueOfIntrinsic)
-            return false;
-        
-        return true;
-    }
-    
-    bool canOptimizeStringObjectAccess(const CodeOrigin& codeOrigin)
-    {
-        if (m_graph.hasExitSite(codeOrigin, NotStringObject))
-            return false;
-        
-        Structure* stringObjectStructure = m_graph.globalObjectFor(codeOrigin)->stringObjectStructure();
-        m_graph.registerStructure(stringObjectStructure);
-        ASSERT(stringObjectStructure->storedPrototype().isObject());
-        ASSERT(stringObjectStructure->storedPrototype().asCell()->classInfo() == StringPrototype::info());
-
-        FrozenValue* stringPrototypeObjectValue =
-            m_graph.freeze(stringObjectStructure->storedPrototype());
-        StringPrototype* stringPrototypeObject =
-            stringPrototypeObjectValue->dynamicCast<StringPrototype*>();
-        Structure* stringPrototypeStructure = stringPrototypeObjectValue->structure();
-        if (m_graph.registerStructure(stringPrototypeStructure) != StructureRegisteredAndWatched)
-            return false;
-        
-        if (stringPrototypeStructure->isDictionary())
-            return false;
-        
-        // We're being conservative here. We want DFG's ToString on StringObject to be
-        // used in both numeric contexts (that would call valueOf()) and string contexts
-        // (that would call toString()). We don't want the DFG to have to distinguish
-        // between the two, just because that seems like it would get confusing. So we
-        // just require both methods to be sane.
-        if (!isStringPrototypeMethodSane(stringPrototypeObject, stringPrototypeStructure, vm().propertyNames->valueOf.impl()))
-            return false;
-        if (!isStringPrototypeMethodSane(stringPrototypeObject, stringPrototypeStructure, vm().propertyNames->toString.impl()))
-            return false;
-        
-        return true;
-    }
-    
+
     void fixupGetAndSetLocalsInBlock(BasicBlock* block)
     {
         if (!block)
index 0c80b67..50c7e86 100644 (file)
@@ -1479,6 +1479,59 @@ MethodOfGettingAValueProfile Graph::methodOfGettingAValueProfileFor(Node* node)
     return MethodOfGettingAValueProfile();
 }
 
+bool Graph::isStringPrototypeMethodSane(JSObject* stringPrototype, Structure* stringPrototypeStructure, UniquedStringImpl* uid)
+{
+    unsigned attributesUnused;
+    PropertyOffset offset = stringPrototypeStructure->getConcurrently(uid, attributesUnused);
+    if (!isValidOffset(offset))
+        return false;
+
+    JSValue value = tryGetConstantProperty(stringPrototype, stringPrototypeStructure, offset);
+    if (!value)
+        return false;
+
+    JSFunction* function = jsDynamicCast<JSFunction*>(value);
+    if (!function)
+        return false;
+
+    if (function->executable()->intrinsicFor(CodeForCall) != StringPrototypeValueOfIntrinsic)
+        return false;
+    
+    return true;
+}
+
+bool Graph::canOptimizeStringObjectAccess(const CodeOrigin& codeOrigin)
+{
+    if (hasExitSite(codeOrigin, NotStringObject))
+        return false;
+
+    Structure* stringObjectStructure = globalObjectFor(codeOrigin)->stringObjectStructure();
+    registerStructure(stringObjectStructure);
+    ASSERT(stringObjectStructure->storedPrototype().isObject());
+    ASSERT(stringObjectStructure->storedPrototype().asCell()->classInfo() == StringPrototype::info());
+
+    FrozenValue* stringPrototypeObjectValue = freeze(stringObjectStructure->storedPrototype());
+    StringPrototype* stringPrototypeObject = stringPrototypeObjectValue->dynamicCast<StringPrototype*>();
+    Structure* stringPrototypeStructure = stringPrototypeObjectValue->structure();
+    if (registerStructure(stringPrototypeStructure) != StructureRegisteredAndWatched)
+        return false;
+
+    if (stringPrototypeStructure->isDictionary())
+        return false;
+
+    // We're being conservative here. We want DFG's ToString on StringObject to be
+    // used in both numeric contexts (that would call valueOf()) and string contexts
+    // (that would call toString()). We don't want the DFG to have to distinguish
+    // between the two, just because that seems like it would get confusing. So we
+    // just require both methods to be sane.
+    if (!isStringPrototypeMethodSane(stringPrototypeObject, stringPrototypeStructure, m_vm.propertyNames->valueOf.impl()))
+        return false;
+    if (!isStringPrototypeMethodSane(stringPrototypeObject, stringPrototypeStructure, m_vm.propertyNames->toString.impl()))
+        return false;
+
+    return true;
+}
+
 } } // namespace JSC::DFG
 
 #endif // ENABLE(DFG_JIT)
index 7ca0ac7..1ff1b35 100644 (file)
@@ -318,6 +318,8 @@ public:
             && negate->canSpeculateInt52(pass);
     }
 
+    bool canOptimizeStringObjectAccess(const CodeOrigin&);
+
     bool roundShouldSpeculateInt32(Node* arithRound, PredictionPass pass)
     {
         ASSERT(arithRound->op() == ArithRound);
@@ -913,7 +915,9 @@ public:
     bool m_hasDebuggerEnabled;
     bool m_hasExceptionHandlers { false };
 private:
-    
+
+    bool isStringPrototypeMethodSane(JSObject* stringPrototype, Structure* stringPrototypeStructure, UniquedStringImpl*);
+
     void handleSuccessor(Vector<BasicBlock*, 16>& worklist, BasicBlock*, BasicBlock* successor);
     
     AddSpeculationMode addImmediateShouldSpeculateInt32(Node* add, bool variableShouldSpeculateInt32, Node* operand, Node*immediate, RareCaseProfilingSource source)
index d72da97..e65f6e4 100644 (file)
 
 namespace JSC { namespace DFG {
 
-SpeculatedType resultOfToPrimitive(SpeculatedType type)
-{
-    if (type & SpecObject) {
-        // Objects get turned into strings. So if the input has hints of objectness,
-        // the output will have hinsts of stringiness.
-        return mergeSpeculations(type & ~SpecObject, SpecString);
-    }
-    
-    return type;
-}
-
 class PredictionPropagationPhase : public Phase {
 public:
     PredictionPropagationPhase(Graph& graph)
@@ -908,6 +897,20 @@ private:
         for (unsigned i = 0; i < m_graph.m_argumentPositions.size(); ++i)
             m_changed |= m_graph.m_argumentPositions[i].mergeArgumentPredictionAwareness();
     }
+
+    SpeculatedType resultOfToPrimitive(SpeculatedType type)
+    {
+        if (type & SpecObject) {
+            // We try to be optimistic here about StringObjects since it's unlikely that
+            // someone overrides the valueOf or toString methods.
+            if (type & SpecStringObject && m_graph.canOptimizeStringObjectAccess(m_currentNode->origin.semantic))
+                return mergeSpeculations(type & ~SpecObject, SpecString);
+
+            return mergeSpeculations(type & ~SpecObject, SpecPrimitive);
+        }
+
+        return type;
+    }
     
     Node* m_currentNode;
     bool m_changed;
diff --git a/Source/JavaScriptCore/tests/stress/string-rope-with-custom-valueof.js b/Source/JavaScriptCore/tests/stress/string-rope-with-custom-valueof.js
new file mode 100644 (file)
index 0000000..0eb7f80
--- /dev/null
@@ -0,0 +1,61 @@
+// This file tests the concatenating of known strings with different objects with overridden valueOf functions.
+// Note: we intentionally do not test Symbols since they cannot be appended to strings...
+
+function catNumber(obj) {
+    return "test" + "things" + obj;
+}
+noInline(catNumber);
+
+number = { valueOf: function() { return 1; } };
+
+function catBool(obj) {
+    return "test" + "things" + obj;
+}
+noInline(catBool);
+
+bool = { valueOf: function() { return true; } };
+
+function catUndefined(obj) {
+    return "test" + "things" + obj;
+}
+noInline(catUndefined);
+
+undef = { valueOf: function() { return undefined; } };
+
+function catRandom(obj) {
+    return "test" + "things" + obj;
+}
+noInline(catRandom);
+
+i = 0;
+random = { valueOf: function() {
+    switch (i % 3) {
+    case 0:
+        return number.valueOf();
+    case 1:
+        return bool.valueOf();
+    case 2:
+        return undef.valueOf();
+    }
+} };
+
+for (i = 0; i < 100000; i++) {
+    if (catNumber(number) !== "testthings1")
+        throw "bad number";
+    if (catBool(bool) !== "testthingstrue")
+        throw "bad bool";
+    if (catUndefined(undef) !== "testthingsundefined")
+        throw "bad undefined";
+    if (catRandom(random) !== "testthings" + random.valueOf())
+        throw "bad random";
+}
+
+// Try passing new types to each of the other functions.
+for (i = 0; i < 100000; i++) {
+    if (catUndefined(number) !== "testthings1")
+        throw "bad number";
+    if (catNumber(bool) !== "testthingstrue")
+        throw "bad bool";
+    if (catBool(undef) !== "testthingsundefined")
+        throw "bad undefined";
+}