Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (142514 => 142515)
--- trunk/Source/_javascript_Core/ChangeLog 2013-02-11 22:22:48 UTC (rev 142514)
+++ trunk/Source/_javascript_Core/ChangeLog 2013-02-11 22:23:08 UTC (rev 142515)
@@ -1,5 +1,38 @@
2013-02-10 Filip Pizlo <fpi...@apple.com>
+ DFG CompareEq(a, null) and CompareStrictEq(a, const) are unsound with respect to constant folding
+ https://bugs.webkit.org/show_bug.cgi?id=109387
+
+ Reviewed by Oliver Hunt and Mark Hahnenberg.
+
+ Lock in the decision to use a non-speculative constant comparison as early as possible
+ and don't let the CFA change it by folding constants. This might be a performance
+ penalty on some really weird code (FWIW, I haven't seen this on benchmarks), but on
+ the other hand it completely side-steps the unsoundness that the bug speaks of.
+
+ Rolling back in after adding 32-bit path.
+
+ * dfg/DFGAbstractState.cpp:
+ (JSC::DFG::AbstractState::execute):
+ * dfg/DFGByteCodeParser.cpp:
+ (JSC::DFG::ByteCodeParser::isConstantForCompareStrictEq):
+ (ByteCodeParser):
+ (JSC::DFG::ByteCodeParser::parseBlock):
+ * dfg/DFGCSEPhase.cpp:
+ (JSC::DFG::CSEPhase::performNodeCSE):
+ * dfg/DFGNodeType.h:
+ (DFG):
+ * dfg/DFGPredictionPropagationPhase.cpp:
+ (JSC::DFG::PredictionPropagationPhase::propagate):
+ * dfg/DFGSpeculativeJIT.cpp:
+ (JSC::DFG::SpeculativeJIT::compileStrictEq):
+ * dfg/DFGSpeculativeJIT32_64.cpp:
+ (JSC::DFG::SpeculativeJIT::compile):
+ * dfg/DFGSpeculativeJIT64.cpp:
+ (JSC::DFG::SpeculativeJIT::compile):
+
+2013-02-10 Filip Pizlo <fpi...@apple.com>
+
DFG TypeOf implementation should have its backend code aligned to what the CFA does
https://bugs.webkit.org/show_bug.cgi?id=109385
Modified: trunk/Source/_javascript_Core/dfg/DFGAbstractState.cpp (142514 => 142515)
--- trunk/Source/_javascript_Core/dfg/DFGAbstractState.cpp 2013-02-11 22:22:48 UTC (rev 142514)
+++ trunk/Source/_javascript_Core/dfg/DFGAbstractState.cpp 2013-02-11 22:23:08 UTC (rev 142515)
@@ -778,7 +778,8 @@
case CompareLessEq:
case CompareGreater:
case CompareGreaterEq:
- case CompareEq: {
+ case CompareEq:
+ case CompareEqConstant: {
bool constantWasSet = false;
JSValue leftConst = forNode(node->child1()).value();
@@ -809,7 +810,7 @@
}
}
- if (!constantWasSet && node->op() == CompareEq) {
+ if (!constantWasSet && (node->op() == CompareEqConstant || node->op() == CompareEq)) {
SpeculatedType leftType = forNode(node->child1()).m_type;
SpeculatedType rightType = forNode(node->child2()).m_type;
if ((isInt32Speculation(leftType) && isOtherSpeculation(rightType))
@@ -825,6 +826,12 @@
forNode(node).set(SpecBoolean);
+ if (node->op() == CompareEqConstant) {
+ // We can exit if we haven't fired the MasqueradesAsUndefind watchpoint yet.
+ node->setCanExit(m_codeBlock->globalObjectFor(node->codeOrigin)->masqueradesAsUndefinedWatchpoint()->isStillValid());
+ break;
+ }
+
Node* left = node->child1().node();
Node* right = node->child2().node();
SpeculatedType filter;
@@ -836,13 +843,6 @@
filter = SpecNumber;
checker = isNumberSpeculation;
} else if (node->op() == CompareEq) {
- if ((m_graph.isConstant(left) && m_graph.valueOfJSConstant(left).isNull())
- || (m_graph.isConstant(right) && m_graph.valueOfJSConstant(right).isNull())) {
- // We can exit if we haven't fired the MasqueradesAsUndefind watchpoint yet.
- node->setCanExit(m_codeBlock->globalObjectFor(node->codeOrigin)->masqueradesAsUndefinedWatchpoint()->isStillValid());
- break;
- }
-
if (left->shouldSpeculateString() || right->shouldSpeculateString()) {
node->setCanExit(false);
break;
@@ -882,7 +882,8 @@
break;
}
- case CompareStrictEq: {
+ case CompareStrictEq:
+ case CompareStrictEqConstant: {
Node* leftNode = node->child1().node();
Node* rightNode = node->child2().node();
JSValue left = forNode(leftNode).value();
@@ -894,20 +895,10 @@
break;
}
forNode(node).set(SpecBoolean);
- if (m_graph.isJSConstant(leftNode)) {
- JSValue value = m_graph.valueOfJSConstant(leftNode);
- if (!value.isNumber() && !value.isString()) {
- node->setCanExit(false);
- break;
- }
+ if (node->op() == CompareStrictEqConstant) {
+ node->setCanExit(false);
+ break;
}
- if (m_graph.isJSConstant(rightNode)) {
- JSValue value = m_graph.valueOfJSConstant(rightNode);
- if (!value.isNumber() && !value.isString()) {
- node->setCanExit(false);
- break;
- }
- }
if (Node::shouldSpeculateInteger(leftNode, rightNode)) {
speculateInt32Binary(node);
break;
Modified: trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp (142514 => 142515)
--- trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2013-02-11 22:22:48 UTC (rev 142514)
+++ trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp 2013-02-11 22:23:08 UTC (rev 142515)
@@ -684,6 +684,14 @@
return node->isStronglyProvedConstantIn(inlineCallFrame());
}
+ bool isConstantForCompareStrictEq(Node* node)
+ {
+ if (!node->isConstant())
+ return false;
+ JSValue value = valueOfJSConstant(node);
+ return value.isBoolean() || value.isUndefinedOrNull();
+ }
+
// These methods create a node and add it to the graph. If nodes of this type are
// 'mustGenerate' then the node will implicitly be ref'ed to ensure generation.
Node* addToGraph(NodeType op, Node* child1 = 0, Node* child2 = 0, Node* child3 = 0)
@@ -2418,7 +2426,7 @@
case op_eq_null: {
Node* value = get(currentInstruction[2].u.operand);
- set(currentInstruction[1].u.operand, addToGraph(CompareEq, value, constantNull()));
+ set(currentInstruction[1].u.operand, addToGraph(CompareEqConstant, value, constantNull()));
NEXT_OPCODE(op_eq_null);
}
@@ -2434,7 +2442,12 @@
NEXT_OPCODE(op_stricteq);
}
}
- set(currentInstruction[1].u.operand, addToGraph(CompareStrictEq, op1, op2));
+ if (isConstantForCompareStrictEq(op1))
+ set(currentInstruction[1].u.operand, addToGraph(CompareStrictEqConstant, op2, op1));
+ else if (isConstantForCompareStrictEq(op2))
+ set(currentInstruction[1].u.operand, addToGraph(CompareStrictEqConstant, op1, op2));
+ else
+ set(currentInstruction[1].u.operand, addToGraph(CompareStrictEq, op1, op2));
NEXT_OPCODE(op_stricteq);
}
@@ -2456,7 +2469,7 @@
case op_neq_null: {
Node* value = get(currentInstruction[2].u.operand);
- set(currentInstruction[1].u.operand, addToGraph(LogicalNot, addToGraph(CompareEq, value, constantNull())));
+ set(currentInstruction[1].u.operand, addToGraph(LogicalNot, addToGraph(CompareEqConstant, value, constantNull())));
NEXT_OPCODE(op_neq_null);
}
@@ -2472,7 +2485,14 @@
NEXT_OPCODE(op_stricteq);
}
}
- set(currentInstruction[1].u.operand, addToGraph(LogicalNot, addToGraph(CompareStrictEq, op1, op2)));
+ Node* invertedResult;
+ if (isConstantForCompareStrictEq(op1))
+ invertedResult = addToGraph(CompareStrictEqConstant, op2, op1);
+ else if (isConstantForCompareStrictEq(op2))
+ invertedResult = addToGraph(CompareStrictEqConstant, op1, op2);
+ else
+ invertedResult = addToGraph(CompareStrictEq, op1, op2);
+ set(currentInstruction[1].u.operand, addToGraph(LogicalNot, invertedResult));
NEXT_OPCODE(op_nstricteq);
}
@@ -2726,7 +2746,7 @@
case op_jeq_null: {
unsigned relativeOffset = currentInstruction[2].u.operand;
Node* value = get(currentInstruction[1].u.operand);
- Node* condition = addToGraph(CompareEq, value, constantNull());
+ Node* condition = addToGraph(CompareEqConstant, value, constantNull());
addToGraph(Branch, OpInfo(m_currentIndex + relativeOffset), OpInfo(m_currentIndex + OPCODE_LENGTH(op_jeq_null)), condition);
LAST_OPCODE(op_jeq_null);
}
@@ -2734,7 +2754,7 @@
case op_jneq_null: {
unsigned relativeOffset = currentInstruction[2].u.operand;
Node* value = get(currentInstruction[1].u.operand);
- Node* condition = addToGraph(CompareEq, value, constantNull());
+ Node* condition = addToGraph(CompareEqConstant, value, constantNull());
addToGraph(Branch, OpInfo(m_currentIndex + OPCODE_LENGTH(op_jneq_null)), OpInfo(m_currentIndex + relativeOffset), condition);
LAST_OPCODE(op_jneq_null);
}
Modified: trunk/Source/_javascript_Core/dfg/DFGCSEPhase.cpp (142514 => 142515)
--- trunk/Source/_javascript_Core/dfg/DFGCSEPhase.cpp 2013-02-11 22:22:48 UTC (rev 142514)
+++ trunk/Source/_javascript_Core/dfg/DFGCSEPhase.cpp 2013-02-11 22:23:08 UTC (rev 142515)
@@ -1111,6 +1111,7 @@
case GetScopeRegisters:
case GetScope:
case TypeOf:
+ case CompareEqConstant:
setReplacement(pureCSE(node));
break;
Modified: trunk/Source/_javascript_Core/dfg/DFGNodeType.h (142514 => 142515)
--- trunk/Source/_javascript_Core/dfg/DFGNodeType.h 2013-02-11 22:22:48 UTC (rev 142514)
+++ trunk/Source/_javascript_Core/dfg/DFGNodeType.h 2013-02-11 22:23:08 UTC (rev 142515)
@@ -186,7 +186,9 @@
macro(CompareGreater, NodeResultBoolean | NodeMustGenerate | NodeMightClobber) \
macro(CompareGreaterEq, NodeResultBoolean | NodeMustGenerate | NodeMightClobber) \
macro(CompareEq, NodeResultBoolean | NodeMustGenerate | NodeMightClobber) \
+ macro(CompareEqConstant, NodeResultBoolean | NodeMustGenerate) \
macro(CompareStrictEq, NodeResultBoolean) \
+ macro(CompareStrictEqConstant, NodeResultBoolean) \
\
/* Calls. */\
macro(Call, NodeResultJS | NodeMustGenerate | NodeHasVarArgs | NodeClobbersWorld) \
Modified: trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp (142514 => 142515)
--- trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp 2013-02-11 22:22:48 UTC (rev 142514)
+++ trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp 2013-02-11 22:23:08 UTC (rev 142515)
@@ -508,7 +508,9 @@
case CompareGreater:
case CompareGreaterEq:
case CompareEq:
+ case CompareEqConstant:
case CompareStrictEq:
+ case CompareStrictEqConstant:
case InstanceOf:
case IsUndefined:
case IsBoolean:
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp (142514 => 142515)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2013-02-11 22:22:48 UTC (rev 142514)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT.cpp 2013-02-11 22:23:08 UTC (rev 142515)
@@ -3460,23 +3460,6 @@
bool SpeculativeJIT::compileStrictEq(Node* node)
{
- // 1) If either operand is a constant and that constant is not a double, integer,
- // or string, then do a JSValue comparison.
-
- if (isJSConstant(node->child1().node())) {
- JSValue value = valueOfJSConstant(node->child1().node());
- if (!value.isNumber() && !value.isString())
- return compileStrictEqForConstant(node, node->child2(), value);
- }
-
- if (isJSConstant(node->child2().node())) {
- JSValue value = valueOfJSConstant(node->child2().node());
- if (!value.isNumber() && !value.isString())
- return compileStrictEqForConstant(node, node->child1(), value);
- }
-
- // 2) If the operands are predicted integer, do an integer comparison.
-
if (Node::shouldSpeculateInteger(node->child1().node(), node->child2().node())) {
unsigned branchIndexInBlock = detectPeepHoleBranch();
if (branchIndexInBlock != UINT_MAX) {
@@ -3492,8 +3475,6 @@
return false;
}
- // 3) If the operands are predicted double, do a double comparison.
-
if (Node::shouldSpeculateNumber(node->child1().node(), node->child2().node())) {
unsigned branchIndexInBlock = detectPeepHoleBranch();
if (branchIndexInBlock != UINT_MAX) {
@@ -3526,8 +3507,6 @@
return false;
}
- // 5) Fall back to non-speculative strict equality.
-
return nonSpeculativeStrictEq(node);
}
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp (142514 => 142515)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp 2013-02-11 22:22:48 UTC (rev 142514)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp 2013-02-11 22:23:08 UTC (rev 142515)
@@ -2549,22 +2549,23 @@
if (compare(node, JITCompiler::GreaterThanOrEqual, JITCompiler::DoubleGreaterThanOrEqual, operationCompareGreaterEq))
return;
break;
+
+ case CompareEqConstant:
+ ASSERT(isNullConstant(node->child2().node()));
+ if (nonSpeculativeCompareNull(node, node->child1()))
+ return;
+ break;
case CompareEq:
- if (isNullConstant(node->child1().node())) {
- if (nonSpeculativeCompareNull(node, node->child2()))
- return;
- break;
- }
- if (isNullConstant(node->child2().node())) {
- if (nonSpeculativeCompareNull(node, node->child1()))
- return;
- break;
- }
if (compare(node, JITCompiler::Equal, JITCompiler::DoubleEqual, operationCompareEq))
return;
break;
+ case CompareStrictEqConstant:
+ if (compileStrictEqForConstant(node, node->child1(), valueOfJSConstant(node->child2().node())))
+ return;
+ break;
+
case CompareStrictEq:
if (compileStrictEq(node))
return;
Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp (142514 => 142515)
--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp 2013-02-11 22:22:48 UTC (rev 142514)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp 2013-02-11 22:23:08 UTC (rev 142515)
@@ -2513,22 +2513,23 @@
if (compare(node, JITCompiler::GreaterThanOrEqual, JITCompiler::DoubleGreaterThanOrEqual, operationCompareGreaterEq))
return;
break;
+
+ case CompareEqConstant:
+ ASSERT(isNullConstant(node->child2().node()));
+ if (nonSpeculativeCompareNull(node, node->child1()))
+ return;
+ break;
case CompareEq:
- if (isNullConstant(node->child1().node())) {
- if (nonSpeculativeCompareNull(node, node->child2()))
- return;
- break;
- }
- if (isNullConstant(node->child2().node())) {
- if (nonSpeculativeCompareNull(node, node->child1()))
- return;
- break;
- }
if (compare(node, JITCompiler::Equal, JITCompiler::DoubleEqual, operationCompareEq))
return;
break;
+ case CompareStrictEqConstant:
+ if (compileStrictEqForConstant(node, node->child1(), valueOfJSConstant(node->child2().node())))
+ return;
+ break;
+
case CompareStrictEq:
if (compileStrictEq(node))
return;
