mergeOSREntryValue is wrong when the incoming value does not match up with the flush...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 15 Apr 2019 20:44:33 +0000 (20:44 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 15 Apr 2019 20:44:33 +0000 (20:44 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196918

Reviewed by Yusuke Suzuki.

r244238 lead to some debug failures because we were calling checkConsistency()
before doing fixTypeForRepresentation when merging in must handle values in
CFA. This patch fixes that.

However, as I was reading over mergeOSREntryValue, I realized it was wrong. It
was possible it could merge in a value/type outside of the variable's flushed type.
Once the flush format types are locked in, we can't introduce a type out of
that range. This probably never lead to any crashes as our profiling injection
and speculation decision code is solid. However, what we were doing is clearly
wrong, and something a fuzzer could have found if we fuzzed the must handle
values inside prediction injection. We should do that fuzzing:
https://bugs.webkit.org/show_bug.cgi?id=196924

* dfg/DFGAbstractValue.cpp:
(JSC::DFG::AbstractValue::mergeOSREntryValue):
* dfg/DFGAbstractValue.h:
* dfg/DFGCFAPhase.cpp:
(JSC::DFG::CFAPhase::injectOSR):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGAbstractValue.cpp
Source/JavaScriptCore/dfg/DFGAbstractValue.h
Source/JavaScriptCore/dfg/DFGCFAPhase.cpp

index 77400bd..efa2b5b 100644 (file)
@@ -1,3 +1,29 @@
+2019-04-15  Saam barati  <sbarati@apple.com>
+
+        mergeOSREntryValue is wrong when the incoming value does not match up with the flush format
+        https://bugs.webkit.org/show_bug.cgi?id=196918
+
+        Reviewed by Yusuke Suzuki.
+
+        r244238 lead to some debug failures because we were calling checkConsistency()
+        before doing fixTypeForRepresentation when merging in must handle values in
+        CFA. This patch fixes that.
+        
+        However, as I was reading over mergeOSREntryValue, I realized it was wrong. It
+        was possible it could merge in a value/type outside of the variable's flushed type.
+        Once the flush format types are locked in, we can't introduce a type out of
+        that range. This probably never lead to any crashes as our profiling injection
+        and speculation decision code is solid. However, what we were doing is clearly
+        wrong, and something a fuzzer could have found if we fuzzed the must handle
+        values inside prediction injection. We should do that fuzzing:
+        https://bugs.webkit.org/show_bug.cgi?id=196924
+
+        * dfg/DFGAbstractValue.cpp:
+        (JSC::DFG::AbstractValue::mergeOSREntryValue):
+        * dfg/DFGAbstractValue.h:
+        * dfg/DFGCFAPhase.cpp:
+        (JSC::DFG::CFAPhase::injectOSR):
+
 2019-04-15  Robin Morisset  <rmorisset@apple.com>
 
         Several structures and enums in the Yarr interpreter can be shrunk
index 2daaaa8..a9c137f 100644 (file)
@@ -180,8 +180,19 @@ void AbstractValue::fixTypeForRepresentation(Graph& graph, Node* node)
     fixTypeForRepresentation(graph, node->result(), node);
 }
 
-bool AbstractValue::mergeOSREntryValue(Graph& graph, JSValue value)
+bool AbstractValue::mergeOSREntryValue(Graph& graph, JSValue value, VariableAccessData* variable, Node* node)
 {
+    FlushFormat flushFormat = variable->flushFormat();
+
+    {
+        if (flushFormat == FlushedDouble && value.isNumber())
+            value = jsDoubleNumber(value.asNumber());
+        SpeculatedType incomingType = resultFor(flushFormat) == NodeResultInt52 ? int52AwareSpeculationFromValue(value) : speculationFromValue(value);
+        SpeculatedType requiredType = typeFilterFor(flushFormat);
+        if (incomingType & ~requiredType)
+            return false;
+    }
+
     AbstractValue oldMe = *this;
     
     if (isClear()) {
@@ -207,8 +218,11 @@ bool AbstractValue::mergeOSREntryValue(Graph& graph, JSValue value)
             m_value = JSValue();
     }
     
-    checkConsistency();
     assertIsRegistered(graph);
+
+    fixTypeForRepresentation(graph, resultFor(flushFormat), node);
+
+    checkConsistency();
     
     return oldMe != *this;
 }
index 70544d9..3ee7289 100644 (file)
@@ -48,6 +48,7 @@ namespace DFG {
 
 class Graph;
 struct Node;
+class VariableAccessData;
 
 struct AbstractValue {
     AbstractValue()
@@ -303,7 +304,7 @@ struct AbstractValue {
         return result;
     }
     
-    bool mergeOSREntryValue(Graph&, JSValue);
+    bool mergeOSREntryValue(Graph&, JSValue, VariableAccessData*, Node*);
     
     void merge(SpeculatedType type)
     {
index bd0449d..8e9bb4a 100644 (file)
@@ -188,9 +188,7 @@ private:
                 dataLog("   Widening ", VirtualRegister(operand), " with ", value.value(), "\n");
             
             AbstractValue& target = block->valuesAtHead.operand(operand);
-            changed |= target.mergeOSREntryValue(m_graph, value.value());
-            target.fixTypeForRepresentation(
-                m_graph, resultFor(node->variableAccessData()->flushFormat()), node);
+            changed |= target.mergeOSREntryValue(m_graph, value.value(), node->variableAccessData(), node);
         }
         
         if (changed || !block->cfaHasVisited) {