InternalField and CheckNeutered DFG nodes are not always safe to execute
authorkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 23 Jan 2020 02:58:51 +0000 (02:58 +0000)
committerkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 23 Jan 2020 02:58:51 +0000 (02:58 +0000)
https://bugs.webkit.org/show_bug.cgi?id=206632

Reviewed by Saam Barati.

JSTests:

* stress/for-of-bad-internal-field-hoist.js: Added.
(foo):

Source/JavaScriptCore:

We currently mark (Get/Set)InternalField/CheckNeutered nodes as safe to execute everywhere. However,
GetInternalField, etc. rely on a proof that the cell passed to it is a subclass of InteralFieldObject.
This combination means we may hoist the nodes past the check guarding them.

Also, remove a bogus assertion that we will have proven the value passed to CheckNeutered is a TypedArray.
It's not valid to require that AI preserve a precise model of all invariants since phases can make changes
that AI doesn't understand.

* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGClobberize.h:
(JSC::DFG::clobberize):
* dfg/DFGSafeToExecute.h:
(JSC::DFG::safeToExecute):
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileCheckNeutered):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileCheckNeutered):

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

JSTests/ChangeLog
JSTests/stress/for-of-bad-internal-field-hoist.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
Source/JavaScriptCore/dfg/DFGClobberize.h
Source/JavaScriptCore/dfg/DFGSafeToExecute.h
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

index 2d85996..1c7ab78 100644 (file)
@@ -1,3 +1,13 @@
+2020-01-22  Keith Miller  <keith_miller@apple.com>
+
+        InternalField and CheckNeutered DFG nodes are not always safe to execute
+        https://bugs.webkit.org/show_bug.cgi?id=206632
+
+        Reviewed by Saam Barati.
+
+        * stress/for-of-bad-internal-field-hoist.js: Added.
+        (foo):
+
 2020-01-22  Yusuke Suzuki  <ysuzuki@apple.com>
 
         [JSC] DateMath should accept more ISO-8601 timezone designators even if they are not included in ECMA262 to produce expected results in the wild code
diff --git a/JSTests/stress/for-of-bad-internal-field-hoist.js b/JSTests/stress/for-of-bad-internal-field-hoist.js
new file mode 100644 (file)
index 0000000..3732beb
--- /dev/null
@@ -0,0 +1,13 @@
+//@ requireOptions("--maximumFunctionForCallInlineCandidateBytecodeCost=500")
+
+function foo() {
+  let x = ''
+  for (let j = 0; j < 10; j++) {
+    for (const y of x) {}
+    x = [0]
+  }
+}
+
+for (let i=0; i<100000; i++) {
+  foo();
+}
index 2ea6302..73116c5 100644 (file)
@@ -1,3 +1,29 @@
+2020-01-22  Keith Miller  <keith_miller@apple.com>
+
+        InternalField and CheckNeutered DFG nodes are not always safe to execute
+        https://bugs.webkit.org/show_bug.cgi?id=206632
+
+        Reviewed by Saam Barati.
+
+        We currently mark (Get/Set)InternalField/CheckNeutered nodes as safe to execute everywhere. However,
+        GetInternalField, etc. rely on a proof that the cell passed to it is a subclass of InteralFieldObject.
+        This combination means we may hoist the nodes past the check guarding them.
+
+        Also, remove a bogus assertion that we will have proven the value passed to CheckNeutered is a TypedArray.
+        It's not valid to require that AI preserve a precise model of all invariants since phases can make changes
+        that AI doesn't understand.
+
+        * dfg/DFGAbstractInterpreterInlines.h:
+        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+        * dfg/DFGClobberize.h:
+        (JSC::DFG::clobberize):
+        * dfg/DFGSafeToExecute.h:
+        (JSC::DFG::safeToExecute):
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileCheckNeutered):
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileCheckNeutered):
+
 2020-01-22  Saam Barati  <sbarati@apple.com>
 
         Add an option for logging total phase times
index 1378bab..e95800a 100644 (file)
@@ -3374,11 +3374,6 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
         break;
     }
 
-    case CheckNeutered: {
-        DFG_ASSERT(m_graph, node, speculationChecked(forNode(node->child1()).m_type, SpecTypedArrayView));
-        break;
-    }
-
     case CheckArrayOrEmpty:
     case CheckArray: {
         AbstractValue& value = forNode(node->child1());
@@ -4100,6 +4095,7 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
     case CountExecution:
     case CheckTierUpInLoop:
     case CheckTierUpAtReturn:
+    case CheckNeutered:
     case SuperSamplerBegin:
     case SuperSamplerEnd:
     case CheckTierUpAndOSREnter:
index c758de6..3813f85 100644 (file)
@@ -1104,8 +1104,6 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
         return;
 
     case CheckNeutered:
-        read(JSCell_typeInfoType);
-        read(JSCell_structureID);
         read(MiscFields);
         return; 
         
index c4c2a2b..9d53350 100644 (file)
 
 namespace JSC { namespace DFG {
 
+// This phase is used to determine if a node can safely run at a new location.
+// It is important to note that returning false does not mean it's definitely 
+// wrong to run the node at the new location. In other words, returning false 
+// does not imply moving the node would be invalid only that this phase could
+// not prove it is valid. Thus, it is always ok to return false.
+
 template<typename AbstractStateType>
 class SafeToExecuteEdge {
 public:
@@ -280,7 +286,6 @@ bool safeToExecute(AbstractStateType& state, Graph& graph, Node* node, bool igno
     case CheckSubClass:
     case CheckArray:
     case CheckArrayOrEmpty:
-    case CheckNeutered:
     case Arrayify:
     case ArrayifyToStructure:
     case GetScope:
@@ -292,8 +297,6 @@ bool safeToExecute(AbstractStateType& state, Graph& graph, Node* node, bool igno
     case GetGlobalVar:
     case GetGlobalLexicalVariable:
     case PutGlobalVariable:
-    case GetInternalField:
-    case PutInternalField:
     case CheckCell:
     case CheckBadCell:
     case CheckNotEmpty:
@@ -552,6 +555,7 @@ bool safeToExecute(AbstractStateType& state, Graph& graph, Node* node, bool igno
     case ArrayPush:
         return node->arrayMode().alreadyChecked(graph, node, state.forNode(graph.varArgChild(node, 1)));
 
+    case CheckNeutered:
     case GetTypedArrayByteOffset:
         return !(state.forNode(node->child1()).m_type & ~(SpecTypedArrayView));
             
@@ -620,6 +624,10 @@ bool safeToExecute(AbstractStateType& state, Graph& graph, Node* node, bool igno
         return true;
     }
 
+    case GetInternalField:
+    case PutInternalField:
+        return false;
+
     case DataViewSet:
         return false;
 
index 6302986..7a07b8a 100644 (file)
@@ -1811,9 +1811,6 @@ void SpeculativeJIT::compileCheckNeutered(Node* node)
     SpeculateCellOperand base(this, node->child1());
     GPRReg baseReg = base.gpr();
 
-    // We only emit this node after we have checked this is a typed array so that better be true now.
-    DFG_ASSERT(m_graph, node, speculationChecked(m_state.forNode(node->child1()).m_type, SpecTypedArrayView));
-
     speculationCheck(
         BadIndexingType, JSValueSource::unboxedCell(baseReg), node->child1(), 
         m_jit.branchTestPtr(MacroAssembler::Zero, MacroAssembler::Address(baseReg, JSArrayBufferView::offsetOfVector())));
index 27befc8..0fe88d6 100644 (file)
@@ -4105,9 +4105,6 @@ private:
     {
         Edge edge = m_node->child1();
         LValue cell = lowCell(edge);
-        
-        // We only emit this node after we have checked this is a typed array so that better be true now.
-        DFG_ASSERT(m_graph, m_node, speculationChecked(abstractValue(edge).m_type, SpecTypedArrayView));
 
         speculate(
             BadIndexingType, jsValueValue(cell), edge.node(),