Log Message
GetterSetter type confusion during DFG compilation https://bugs.webkit.org/show_bug.cgi?id=199903
Reviewed by Mark Lam.
JSTests:
* stress/cse-propagated-constant-may-not-follow-structure-restrictions.js: Added.
Source/_javascript_Core:
In AI, we are strongly assuming that GetGetter's child constant value should be GetterSetter if it exists.
However, this can be wrong since nobody ensures that. AI assumed so because the control-flow and preceding
CheckStructure ensures that. But this preceding check can be eliminated if the node becomes (at runtime) unreachable.
Let's consider the following graph.
129:<!0:-> PutByOffset(KnownCell:@115, KnownCell:@115, Check:Untyped:@124, MustGen, id5{length}, 0, W:NamedProperties(5), ClobbersExit, bc#154, ExitValid)
130:<!0:-> PutStructure(KnownCell:@115, MustGen, %C8:Object -> %C3:Object, ID:7726, R:JSObject_butterfly, W:JSCell_indexingType,JSCell_structureID,JSCell_typeInfoFlags,JSCell_typeInfoType, ClobbersExit, bc#154, ExitInvalid)
...
158:<!0:-> GetLocal(Check:Untyped:@197, JS|MustGen|UseAsOther, Final, loc7(R<Final>/FlushedCell), R:Stack(-8), bc#187, ExitValid) predicting Final
210:< 1:-> DoubleRep(Check:NotCell:@158, Double|PureInt, BytecodeDouble, Exits, bc#187, ExitValid)
...
162:<!0:-> CheckStructure(Cell:@158, MustGen, [%Ad:Object], R:JSCell_structureID, Exits, bc#192, ExitValid)
163:< 1:-> GetGetterSetterByOffset(KnownCell:@158, KnownCell:@158, JS|UseAsOther, OtherCell, id5{length}, 0, R:NamedProperties(5), Exits, bc#192, ExitValid)
164:< 1:-> GetGetter(KnownCell:@163, JS|UseAsOther, Function, R:GetterSetter_getter, Exits, bc#192, ExitValid)
At @163 and @164, AI proves that @158's AbstractValue is None because @210's edge filters out Cells @158 is a cell. But we do not invalidate graph status as "Invalid" even if edge filters out all possible value.
This is because the result of edge can be None in a valid program. For example, we can put a dependency edge between a consuming node and a producing node, where the producing node is just like a check and it
does not produce a value actually. So, @163 and @164 are not invalidated. This is totally fine in our compiler pipeline right now.
But after that, global CSE phase found that @115 and @158 are same and @129 dominates @158. As a result, we can replace GetGetter child's @163 with @124. Since CheckStructure is already removed (and now, at runtime,
@163 and @164 are never executed), we do not have any structure guarantee on @158 and the result of @163. This means that @163's CSE result can be non-GetterSetter value.
124:< 2:-> JSConstant(JS|UseAsOther, Final, Weak:Object: 0x1199e82a0 with butterfly 0x0 (Structure %B4:Object), StructureID: 49116, bc#0, ExitValid)
...
126:< 2:-> GetGetter(KnownCell:Kill:@124, JS|UseAsOther, Function, R:GetterSetter_getter, Exits, bc#192, ExitValid)
AI filters out @124's non-cell values. But @126 can get non-GetterSetter cell at AI phase. But our AI code is like the following.
JSValue base = forNode(node->child1()).m_value;
if (base) {
GetterSetter* getterSetter = jsCast<GetterSetter*>(base);
...
Then, jsCast casts the above object with GetterSetter accidentally.
In general, DFG AI can get a proven constant value, which could not be shown at runtime. This happens if the processing node is unreachable at runtime while the graph is not invalid yet, because preceding edge
filters already filter out all the possible execution. DFG AI already considered about this possibility, and it attempts to fold a node into a constant only when the constant input matches against the expected one.
But several DFG nodes are not handling this correctly: GetGetter, GetSetter, and SkipScope.
In this patch, we use `jsDynamicCast` to ensure that the constant input matches against the expected (foldable) one, and fold it only when the expectation is met.
We also remove DFG::Node::castConstant and its use. We should not rely on the constant folded value based on graph's control-flow.
* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGNode.h:
(JSC::DFG::Node::castConstant): Deleted.
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileMaterializeCreateActivation):
Modified Paths
- trunk/JSTests/ChangeLog
- trunk/Source/_javascript_Core/ChangeLog
- trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h
- trunk/Source/_javascript_Core/dfg/DFGNode.h
- trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (248148 => 248149)
--- trunk/JSTests/ChangeLog 2019-08-02 05:46:32 UTC (rev 248148)
+++ trunk/JSTests/ChangeLog 2019-08-02 05:58:20 UTC (rev 248149)
@@ -1,3 +1,12 @@
+2019-08-01 Yusuke Suzuki <ysuz...@apple.com>
+
+ GetterSetter type confusion during DFG compilation
+ https://bugs.webkit.org/show_bug.cgi?id=199903
+
+ Reviewed by Mark Lam.
+
+ * stress/cse-propagated-constant-may-not-follow-structure-restrictions.js: Added.
+
2019-08-01 Ross Kirsling <ross.kirsl...@sony.com>
Update Test262 (2019.08.01)
Added: trunk/JSTests/stress/cse-propagated-constant-may-not-follow-structure-restrictions.js (0 => 248149)
--- trunk/JSTests/stress/cse-propagated-constant-may-not-follow-structure-restrictions.js (rev 0)
+++ trunk/JSTests/stress/cse-propagated-constant-may-not-follow-structure-restrictions.js 2019-08-02 05:58:20 UTC (rev 248149)
@@ -0,0 +1,23 @@
+let notAGetterSetter = {whatever: 42};
+
+function v2(v5) {
+ const v10 = Object();
+ if (v5) {
+ const v12 = {set:Array};
+ const v14 = Object.defineProperty(v10,"length",v12);
+ const v15 = (140899729)[140899729];
+ } else {
+ v10.length = notAGetterSetter;
+ }
+ const v18 = new Uint8ClampedArray(49415);
+ v18[1] = v10;
+ const v19 = v10.length;
+ let v20 = 0;
+ while (v20 < 100000) {
+ v20++;
+ }
+}
+const v26 = v2();
+for (let v32 = 0; v32 < 1000; v32++) {
+ const v33 = v2(true);
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (248148 => 248149)
--- trunk/Source/_javascript_Core/ChangeLog 2019-08-02 05:46:32 UTC (rev 248148)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-08-02 05:58:20 UTC (rev 248149)
@@ -1,3 +1,61 @@
+2019-08-01 Yusuke Suzuki <ysuz...@apple.com>
+
+ GetterSetter type confusion during DFG compilation
+ https://bugs.webkit.org/show_bug.cgi?id=199903
+
+ Reviewed by Mark Lam.
+
+ In AI, we are strongly assuming that GetGetter's child constant value should be GetterSetter if it exists.
+ However, this can be wrong since nobody ensures that. AI assumed so because the control-flow and preceding
+ CheckStructure ensures that. But this preceding check can be eliminated if the node becomes (at runtime) unreachable.
+
+ Let's consider the following graph.
+
+ 129:<!0:-> PutByOffset(KnownCell:@115, KnownCell:@115, Check:Untyped:@124, MustGen, id5{length}, 0, W:NamedProperties(5), ClobbersExit, bc#154, ExitValid)
+ 130:<!0:-> PutStructure(KnownCell:@115, MustGen, %C8:Object -> %C3:Object, ID:7726, R:JSObject_butterfly, W:JSCell_indexingType,JSCell_structureID,JSCell_typeInfoFlags,JSCell_typeInfoType, ClobbersExit, bc#154, ExitInvalid)
+ ...
+ 158:<!0:-> GetLocal(Check:Untyped:@197, JS|MustGen|UseAsOther, Final, loc7(R<Final>/FlushedCell), R:Stack(-8), bc#187, ExitValid) predicting Final
+ 210:< 1:-> DoubleRep(Check:NotCell:@158, Double|PureInt, BytecodeDouble, Exits, bc#187, ExitValid)
+ ...
+ 162:<!0:-> CheckStructure(Cell:@158, MustGen, [%Ad:Object], R:JSCell_structureID, Exits, bc#192, ExitValid)
+ 163:< 1:-> GetGetterSetterByOffset(KnownCell:@158, KnownCell:@158, JS|UseAsOther, OtherCell, id5{length}, 0, R:NamedProperties(5), Exits, bc#192, ExitValid)
+ 164:< 1:-> GetGetter(KnownCell:@163, JS|UseAsOther, Function, R:GetterSetter_getter, Exits, bc#192, ExitValid)
+
+ At @163 and @164, AI proves that @158's AbstractValue is None because @210's edge filters out Cells @158 is a cell. But we do not invalidate graph status as "Invalid" even if edge filters out all possible value.
+ This is because the result of edge can be None in a valid program. For example, we can put a dependency edge between a consuming node and a producing node, where the producing node is just like a check and it
+ does not produce a value actually. So, @163 and @164 are not invalidated. This is totally fine in our compiler pipeline right now.
+
+ But after that, global CSE phase found that @115 and @158 are same and @129 dominates @158. As a result, we can replace GetGetter child's @163 with @124. Since CheckStructure is already removed (and now, at runtime,
+ @163 and @164 are never executed), we do not have any structure guarantee on @158 and the result of @163. This means that @163's CSE result can be non-GetterSetter value.
+
+ 124:< 2:-> JSConstant(JS|UseAsOther, Final, Weak:Object: 0x1199e82a0 with butterfly 0x0 (Structure %B4:Object), StructureID: 49116, bc#0, ExitValid)
+ ...
+ 126:< 2:-> GetGetter(KnownCell:Kill:@124, JS|UseAsOther, Function, R:GetterSetter_getter, Exits, bc#192, ExitValid)
+
+ AI filters out @124's non-cell values. But @126 can get non-GetterSetter cell at AI phase. But our AI code is like the following.
+
+
+ JSValue base = forNode(node->child1()).m_value;
+ if (base) {
+ GetterSetter* getterSetter = jsCast<GetterSetter*>(base);
+ ...
+
+ Then, jsCast casts the above object with GetterSetter accidentally.
+
+ In general, DFG AI can get a proven constant value, which could not be shown at runtime. This happens if the processing node is unreachable at runtime while the graph is not invalid yet, because preceding edge
+ filters already filter out all the possible execution. DFG AI already considered about this possibility, and it attempts to fold a node into a constant only when the constant input matches against the expected one.
+ But several DFG nodes are not handling this correctly: GetGetter, GetSetter, and SkipScope.
+
+ In this patch, we use `jsDynamicCast` to ensure that the constant input matches against the expected (foldable) one, and fold it only when the expectation is met.
+ We also remove DFG::Node::castConstant and its use. We should not rely on the constant folded value based on graph's control-flow.
+
+ * dfg/DFGAbstractInterpreterInlines.h:
+ (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+ * dfg/DFGNode.h:
+ (JSC::DFG::Node::castConstant): Deleted.
+ * ftl/FTLLowerDFGToB3.cpp:
+ (JSC::FTL::DFG::LowerDFGToB3::compileMaterializeCreateActivation):
+
2019-08-01 Mark Lam <mark....@apple.com>
Add crash diagnostics for debugging unexpected zapped cells.
Modified: trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h (248148 => 248149)
--- trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h 2019-08-02 05:46:32 UTC (rev 248148)
+++ trunk/Source/_javascript_Core/dfg/DFGAbstractInterpreterInlines.h 2019-08-02 05:58:20 UTC (rev 248149)
@@ -2806,10 +2806,9 @@
break;
case GetGetter: {
- JSValue base = forNode(node->child1()).m_value;
- if (base) {
- GetterSetter* getterSetter = jsCast<GetterSetter*>(base);
- if (!getterSetter->isGetterNull()) {
+ if (JSValue base = forNode(node->child1()).m_value) {
+ GetterSetter* getterSetter = jsDynamicCast<GetterSetter*>(m_vm, base);
+ if (getterSetter && !getterSetter->isGetterNull()) {
setConstant(node, *m_graph.freeze(getterSetter->getterConcurrently()));
break;
}
@@ -2820,10 +2819,9 @@
}
case GetSetter: {
- JSValue base = forNode(node->child1()).m_value;
- if (base) {
- GetterSetter* getterSetter = jsCast<GetterSetter*>(base);
- if (!getterSetter->isSetterNull()) {
+ if (JSValue base = forNode(node->child1()).m_value) {
+ GetterSetter* getterSetter = jsDynamicCast<GetterSetter*>(m_vm, base);
+ if (getterSetter && !getterSetter->isSetterNull()) {
setConstant(node, *m_graph.freeze(getterSetter->setterConcurrently()));
break;
}
@@ -2844,10 +2842,13 @@
break;
case SkipScope: {
- JSValue child = forNode(node->child1()).value();
- if (child) {
- setConstant(node, *m_graph.freeze(JSValue(jsCast<JSScope*>(child.asCell())->next())));
- break;
+ if (JSValue child = forNode(node->child1()).value()) {
+ if (JSScope* scope = jsDynamicCast<JSScope*>(m_vm, child)) {
+ if (JSScope* nextScope = scope->next()) {
+ setConstant(node, *m_graph.freeze(JSValue(nextScope)));
+ break;
+ }
+ }
}
setTypeForNode(node, SpecObjectOther);
break;
Modified: trunk/Source/_javascript_Core/dfg/DFGNode.h (248148 => 248149)
--- trunk/Source/_javascript_Core/dfg/DFGNode.h 2019-08-02 05:46:32 UTC (rev 248148)
+++ trunk/Source/_javascript_Core/dfg/DFGNode.h 2019-08-02 05:58:20 UTC (rev 248149)
@@ -859,14 +859,6 @@
return jsDynamicCast<T>(vm, asCell());
}
- template<typename T>
- T castConstant(VM& vm)
- {
- T result = dynamicCastConstant<T>(vm);
- RELEASE_ASSERT(result);
- return result;
- }
-
bool hasLazyJSValue()
{
return op() == LazyJSConstant;
Modified: trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp (248148 => 248149)
--- trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2019-08-02 05:46:32 UTC (rev 248148)
+++ trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2019-08-02 05:58:20 UTC (rev 248149)
@@ -11346,7 +11346,6 @@
LValue scope = lowCell(m_graph.varArgChild(m_node, 1));
SymbolTable* table = m_node->castOperand<SymbolTable*>();
- ASSERT(table == m_graph.varArgChild(m_node, 0)->castConstant<SymbolTable*>(vm()));
RegisteredStructure structure = m_graph.registerStructure(m_graph.globalObjectFor(m_node->origin.semantic)->activationStructure());
LBasicBlock slowPath = m_out.newBlock();
_______________________________________________ webkit-changes mailing list webkit-changes@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-changes
