Creating a new blank document in icloud pages causes an AI error: Abstract value...
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 May 2015 23:57:17 +0000 (23:57 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 May 2015 23:57:17 +0000 (23:57 +0000)
https://bugs.webkit.org/show_bug.cgi?id=144856

Reviewed by Benjamin Poulain.

First I made fixTypeForRepresentation() print out better diagnostics when it dies.

Then I fixed the bug: Node::convertToIdentityOn(Node*) needs to make sure that when it
converts to a representation-changing node, it needs to use one of the UseKinds that such
a node expects. For example, DoubleRep(UntypedUse:) doesn't make sense; it needs to be
something like DoubleRep(NumberUse:) since it will speculate that the input is a number.

* dfg/DFGAbstractInterpreter.h:
(JSC::DFG::AbstractInterpreter::setBuiltInConstant):
* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGAbstractValue.cpp:
(JSC::DFG::AbstractValue::fixTypeForRepresentation):
* dfg/DFGAbstractValue.h:
* dfg/DFGInPlaceAbstractState.cpp:
(JSC::DFG::InPlaceAbstractState::initialize):
* dfg/DFGNode.cpp:
(JSC::DFG::Node::convertToIdentityOn):
* tests/stress/cloned-arguments-get-by-val-double-array.js: Added.
(foo):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGAbstractInterpreter.h
Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
Source/JavaScriptCore/dfg/DFGAbstractValue.cpp
Source/JavaScriptCore/dfg/DFGAbstractValue.h
Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp
Source/JavaScriptCore/dfg/DFGNode.cpp
Source/JavaScriptCore/tests/stress/cloned-arguments-get-by-val-double-array.js [new file with mode: 0644]

index 2b080f0..61e5e61 100644 (file)
@@ -1,3 +1,31 @@
+2015-05-13  Filip Pizlo  <fpizlo@apple.com>
+
+        Creating a new blank document in icloud pages causes an AI error: Abstract value (CellBytecodedoubleBoolOther, TOP, TOP) for double node has type outside SpecFullDouble.
+        https://bugs.webkit.org/show_bug.cgi?id=144856
+
+        Reviewed by Benjamin Poulain.
+        
+        First I made fixTypeForRepresentation() print out better diagnostics when it dies.
+        
+        Then I fixed the bug: Node::convertToIdentityOn(Node*) needs to make sure that when it
+        converts to a representation-changing node, it needs to use one of the UseKinds that such
+        a node expects. For example, DoubleRep(UntypedUse:) doesn't make sense; it needs to be
+        something like DoubleRep(NumberUse:) since it will speculate that the input is a number.
+
+        * dfg/DFGAbstractInterpreter.h:
+        (JSC::DFG::AbstractInterpreter::setBuiltInConstant):
+        * dfg/DFGAbstractInterpreterInlines.h:
+        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+        * dfg/DFGAbstractValue.cpp:
+        (JSC::DFG::AbstractValue::fixTypeForRepresentation):
+        * dfg/DFGAbstractValue.h:
+        * dfg/DFGInPlaceAbstractState.cpp:
+        (JSC::DFG::InPlaceAbstractState::initialize):
+        * dfg/DFGNode.cpp:
+        (JSC::DFG::Node::convertToIdentityOn):
+        * tests/stress/cloned-arguments-get-by-val-double-array.js: Added.
+        (foo):
+
 2015-05-13  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r184313.
index f372f64..b3ebd68 100644 (file)
@@ -168,7 +168,7 @@ private:
     {
         AbstractValue& abstractValue = forNode(node);
         abstractValue.set(m_graph, value, m_state.structureClobberState());
-        abstractValue.fixTypeForRepresentation(node);
+        abstractValue.fixTypeForRepresentation(m_graph, node);
     }
     
     void setConstant(Node* node, FrozenValue value)
index e00b468..761cf78 100644 (file)
@@ -348,7 +348,7 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
             break;
         }
         forNode(node).setType(m_graph, forNode(node->child1()).m_type);
-        forNode(node).fixTypeForRepresentation(node);
+        forNode(node).fixTypeForRepresentation(m_graph, node);
         break;
     }
         
@@ -371,7 +371,7 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
         }
         
         forNode(node).setType(m_graph, forNode(node->child1()).m_type & ~SpecDoubleImpureNaN);
-        forNode(node).fixTypeForRepresentation(node);
+        forNode(node).fixTypeForRepresentation(m_graph, node);
         break;
     }
         
index b219df6..fb25c5a 100644 (file)
@@ -136,7 +136,7 @@ void AbstractValue::setType(Graph& graph, SpeculatedType type)
     checkConsistency();
 }
 
-void AbstractValue::fixTypeForRepresentation(NodeFlags representation)
+void AbstractValue::fixTypeForRepresentation(Graph& graph, NodeFlags representation, Node* node)
 {
     if (representation == NodeResultDouble) {
         if (m_value) {
@@ -148,39 +148,30 @@ void AbstractValue::fixTypeForRepresentation(NodeFlags representation)
             m_type &= ~SpecMachineInt;
             m_type |= SpecInt52AsDouble;
         }
-        if (m_type & ~SpecFullDouble) {
-            startCrashing();
-            dataLog("Abstract value ", *this, " for double node has type outside SpecFullDouble.\n");
-            CRASH();
-        }
+        if (m_type & ~SpecFullDouble)
+            DFG_CRASH(graph, node, toCString("Abstract value ", *this, " for double node has type outside SpecFullDouble.\n").data());
     } else if (representation == NodeResultInt52) {
         if (m_type & SpecInt52AsDouble) {
             m_type &= ~SpecInt52AsDouble;
             m_type |= SpecInt52;
         }
-        if (m_type & ~SpecMachineInt) {
-            startCrashing();
-            dataLog("Abstract value ", *this, " for int52 node has type outside SpecMachineInt.\n");
-            CRASH();
-        }
+        if (m_type & ~SpecMachineInt)
+            DFG_CRASH(graph, node, toCString("Abstract value ", *this, " for int52 node has type outside SpecMachineInt.\n").data());
     } else {
         if (m_type & SpecInt52) {
             m_type &= ~SpecInt52;
             m_type |= SpecInt52AsDouble;
         }
-        if (m_type & ~SpecBytecodeTop) {
-            startCrashing();
-            dataLog("Abstract value ", *this, " for value node has type outside SpecBytecodeTop.\n");
-            CRASH();
-        }
+        if (m_type & ~SpecBytecodeTop)
+            DFG_CRASH(graph, node, toCString("Abstract value ", *this, " for value node has type outside SpecBytecodeTop.\n").data());
     }
     
     checkConsistency();
 }
 
-void AbstractValue::fixTypeForRepresentation(Node* node)
+void AbstractValue::fixTypeForRepresentation(Graph& graph, Node* node)
 {
-    fixTypeForRepresentation(node->result());
+    fixTypeForRepresentation(graph, node->result(), node);
 }
 
 FiltrationResult AbstractValue::filter(Graph& graph, const StructureSet& other)
index 4ebf331..bde4b9e 100644 (file)
@@ -213,8 +213,8 @@ struct AbstractValue {
         checkConsistency();
     }
     
-    void fixTypeForRepresentation(NodeFlags representation);
-    void fixTypeForRepresentation(Node*);
+    void fixTypeForRepresentation(Graph&, NodeFlags representation, Node* = nullptr);
+    void fixTypeForRepresentation(Graph&, Node*);
     
     bool operator==(const AbstractValue& other) const
     {
index a3d7b39..9bcca9e 100644 (file)
@@ -167,7 +167,7 @@ void InPlaceAbstractState::initialize()
             VariableAccessData* variable = node->variableAccessData();
             FlushFormat format = variable->flushFormat();
             target.merge(source);
-            target.fixTypeForRepresentation(resultFor(format));
+            target.fixTypeForRepresentation(m_graph, resultFor(format));
         }
         block->cfaShouldRevisit = true;
     }
index 5a7cebe..6a98534 100644 (file)
@@ -116,17 +116,44 @@ void Node::convertToIdentityOn(Node* child)
     }
     switch (output) {
     case NodeResultDouble:
-        RELEASE_ASSERT(input == NodeResultInt52 || input == NodeResultJS);
         setOpAndDefaultFlags(DoubleRep);
-        return;
+        switch (input) {
+        case NodeResultInt52:
+            child1().setUseKind(Int52RepUse);
+            return;
+        case NodeResultJS:
+            child1().setUseKind(NumberUse);
+            return;
+        default:
+            RELEASE_ASSERT_NOT_REACHED();
+            return;
+        }
     case NodeResultInt52:
-        RELEASE_ASSERT(input == NodeResultDouble || input == NodeResultJS);
         setOpAndDefaultFlags(Int52Rep);
-        return;
+        switch (input) {
+        case NodeResultDouble:
+            child1().setUseKind(DoubleRepMachineIntUse);
+            return;
+        case NodeResultJS:
+            child1().setUseKind(MachineIntUse);
+            return;
+        default:
+            RELEASE_ASSERT_NOT_REACHED();
+            return;
+        }
     case NodeResultJS:
-        RELEASE_ASSERT(input == NodeResultDouble || input == NodeResultInt52);
         setOpAndDefaultFlags(ValueRep);
-        return;
+        switch (input) {
+        case NodeResultDouble:
+            child1().setUseKind(DoubleRepUse);
+            return;
+        case NodeResultInt52:
+            child1().setUseKind(Int52RepUse);
+            return;
+        default:
+            RELEASE_ASSERT_NOT_REACHED();
+            return;
+        }
     default:
         RELEASE_ASSERT_NOT_REACHED();
         return;
diff --git a/Source/JavaScriptCore/tests/stress/cloned-arguments-get-by-val-double-array.js b/Source/JavaScriptCore/tests/stress/cloned-arguments-get-by-val-double-array.js
new file mode 100644 (file)
index 0000000..a026aff
--- /dev/null
@@ -0,0 +1,13 @@
+function foo() {
+    "use strict";
+    return arguments[0] + 1.5;
+}
+
+noInline(foo);
+
+for (var i = 0; i < 10000; ++i) {
+    var result = foo(4.2);
+    if (result != 5.7)
+        throw "Error: bad result: " + result;
+}
+