Log Message
Python implementation reports "MemoryError" instead of doing things https://bugs.webkit.org/show_bug.cgi?id=106690
Source/_javascript_Core:
Reviewed by Oliver Hunt.
The bug was that the CFA was assuming that a variable is dead at the end of a basic block and hence doesn't need to
be merged to the next block if the last mention of the variable was dead. This is almost correct, except that it
doesn't work if the last mention is a GetLocal - the GetLocal itself may be dead, but that doesn't mean that the
variable is dead - it may still be live. The appropriate thing to do is to look at the GetLocal's Phi. If the
variable is used in the next block then the next block will have a reference to the last mention in our block unless
that last mention is a GetLocal, in which case it will link to the Phi. Doing it this way captures everything that
the CFA wants: if the last use is a live GetLocal then the CFA needs to consider the GetLocal itself for possible
refinements to the proof of the value in the variable, but if the GetLocal is dead, then this must mean that the
variable is not mentioned in the block but may still be "passed through" it, which is what the Phi will tell us.
Note that it is not possible for the GetLocal to refer to anything other than a Phi, and it is also not possible
for the last mention of a variable to be a dead GetLocal while there are other mentions that aren't dead - if
there had been SetLocals or GetLocals prior to the dead one then the dead one wouldn't have been emitted by the
parser.
This also fixes a similar bug in the handling of captured variables. If a variable is captured, then it doesn't
matter if the last mention is dead, or not. Either way, we already know that a captured variable will be live in
the next block, so we must merge it no matter what.
Finally, this change makes the output of Operands dumping a bit more verbose: it now prints the variable name next
to each variable's dump. I've often found the lack of this information confusing particularly for operand dumps
that involve a lot of variables.
* bytecode/Operands.h:
(JSC::dumpOperands):
* dfg/DFGAbstractState.cpp:
(JSC::DFG::AbstractState::mergeStateAtTail):
LayoutTests:
Reviewed by Oliver Hunt.
* fast/js/dfg-cfa-merge-with-dead-use-at-tail-expected.txt: Added.
* fast/js/dfg-cfa-merge-with-dead-use-at-tail.html: Added.
* fast/js/jsc-test-list:
* fast/js/script-tests/dfg-cfa-merge-with-dead-use-at-tail.js: Added.
(foo):
Modified Paths
- trunk/LayoutTests/ChangeLog
- trunk/LayoutTests/fast/js/jsc-test-list
- trunk/Source/_javascript_Core/ChangeLog
- trunk/Source/_javascript_Core/bytecode/Operands.h
- trunk/Source/_javascript_Core/dfg/DFGAbstractState.cpp
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (139686 => 139687)
--- trunk/LayoutTests/ChangeLog 2013-01-15 00:51:30 UTC (rev 139686)
+++ trunk/LayoutTests/ChangeLog 2013-01-15 00:53:39 UTC (rev 139687)
@@ -1,3 +1,16 @@
+2013-01-11 Filip Pizlo <fpi...@apple.com>
+
+ Python implementation reports "MemoryError" instead of doing things
+ https://bugs.webkit.org/show_bug.cgi?id=106690
+
+ Reviewed by Oliver Hunt.
+
+ * fast/js/dfg-cfa-merge-with-dead-use-at-tail-expected.txt: Added.
+ * fast/js/dfg-cfa-merge-with-dead-use-at-tail.html: Added.
+ * fast/js/jsc-test-list:
+ * fast/js/script-tests/dfg-cfa-merge-with-dead-use-at-tail.js: Added.
+ (foo):
+
2013-01-14 Tien-Ren Chen <trc...@chromium.org>
Correct FrameView::scrollableAreaBoundingBox() calculation in the presence of transforms
Added: trunk/LayoutTests/fast/js/dfg-cfa-merge-with-dead-use-at-tail-expected.txt (0 => 139687)
--- trunk/LayoutTests/fast/js/dfg-cfa-merge-with-dead-use-at-tail-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/js/dfg-cfa-merge-with-dead-use-at-tail-expected.txt 2013-01-15 00:53:39 UTC (rev 139687)
@@ -0,0 +1,209 @@
+Tests that a dead use of a variable at the tail of a basic block doesn't confuse the CFA into believing that the variable being used is dead as well.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS foo(false, true, 5) is 42
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/js/dfg-cfa-merge-with-dead-use-at-tail.html (0 => 139687)
--- trunk/LayoutTests/fast/js/dfg-cfa-merge-with-dead-use-at-tail.html (rev 0)
+++ trunk/LayoutTests/fast/js/dfg-cfa-merge-with-dead-use-at-tail.html 2013-01-15 00:53:39 UTC (rev 139687)
@@ -0,0 +1,10 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src=""
+</head>
+<body>
+<script src=""
+<script src=""
+</body>
+</html>
Modified: trunk/LayoutTests/fast/js/jsc-test-list (139686 => 139687)
--- trunk/LayoutTests/fast/js/jsc-test-list 2013-01-15 00:51:30 UTC (rev 139686)
+++ trunk/LayoutTests/fast/js/jsc-test-list 2013-01-15 00:53:39 UTC (rev 139687)
@@ -90,6 +90,7 @@
fast/js/dfg-array-push-slow-put
fast/js/dfg-bool-to-int32-reuse
fast/js/dfg-branch-not-fail
+fast/js/dfg-cfa-merge-with-dead-use-at-tail
fast/js/dfg-cfg-simplify-phantom-get-local-on-same-block-set-local
fast/js/dfg-cfg-simplify-redundant-dead-get-local
fast/js/dfg-check-two-structures
Added: trunk/LayoutTests/fast/js/script-tests/dfg-cfa-merge-with-dead-use-at-tail.js (0 => 139687)
--- trunk/LayoutTests/fast/js/script-tests/dfg-cfa-merge-with-dead-use-at-tail.js (rev 0)
+++ trunk/LayoutTests/fast/js/script-tests/dfg-cfa-merge-with-dead-use-at-tail.js 2013-01-15 00:53:39 UTC (rev 139687)
@@ -0,0 +1,22 @@
+description(
+"Tests that a dead use of a variable at the tail of a basic block doesn't confuse the CFA into believing that the variable being used is dead as well."
+);
+
+function foo(p, q, v) {
+ var x, y;
+ if (p)
+ x = 0;
+ else {
+ if (q)
+ x = v;
+ else
+ x = 0;
+ y = x;
+ }
+ if (x)
+ return 42;
+ return 0;
+}
+
+for (var i = 0; i < 200; ++i)
+ shouldBe("foo(false, true, 5)", "42");
Modified: trunk/Source/_javascript_Core/ChangeLog (139686 => 139687)
--- trunk/Source/_javascript_Core/ChangeLog 2013-01-15 00:51:30 UTC (rev 139686)
+++ trunk/Source/_javascript_Core/ChangeLog 2013-01-15 00:53:39 UTC (rev 139687)
@@ -1,3 +1,37 @@
+2013-01-11 Filip Pizlo <fpi...@apple.com>
+
+ Python implementation reports "MemoryError" instead of doing things
+ https://bugs.webkit.org/show_bug.cgi?id=106690
+
+ Reviewed by Oliver Hunt.
+
+ The bug was that the CFA was assuming that a variable is dead at the end of a basic block and hence doesn't need to
+ be merged to the next block if the last mention of the variable was dead. This is almost correct, except that it
+ doesn't work if the last mention is a GetLocal - the GetLocal itself may be dead, but that doesn't mean that the
+ variable is dead - it may still be live. The appropriate thing to do is to look at the GetLocal's Phi. If the
+ variable is used in the next block then the next block will have a reference to the last mention in our block unless
+ that last mention is a GetLocal, in which case it will link to the Phi. Doing it this way captures everything that
+ the CFA wants: if the last use is a live GetLocal then the CFA needs to consider the GetLocal itself for possible
+ refinements to the proof of the value in the variable, but if the GetLocal is dead, then this must mean that the
+ variable is not mentioned in the block but may still be "passed through" it, which is what the Phi will tell us.
+ Note that it is not possible for the GetLocal to refer to anything other than a Phi, and it is also not possible
+ for the last mention of a variable to be a dead GetLocal while there are other mentions that aren't dead - if
+ there had been SetLocals or GetLocals prior to the dead one then the dead one wouldn't have been emitted by the
+ parser.
+
+ This also fixes a similar bug in the handling of captured variables. If a variable is captured, then it doesn't
+ matter if the last mention is dead, or not. Either way, we already know that a captured variable will be live in
+ the next block, so we must merge it no matter what.
+
+ Finally, this change makes the output of Operands dumping a bit more verbose: it now prints the variable name next
+ to each variable's dump. I've often found the lack of this information confusing particularly for operand dumps
+ that involve a lot of variables.
+
+ * bytecode/Operands.h:
+ (JSC::dumpOperands):
+ * dfg/DFGAbstractState.cpp:
+ (JSC::DFG::AbstractState::mergeStateAtTail):
+
2013-01-14 Roger Fong <roger_f...@apple.com>
Unreviewed. Fix vcproj file. Missing file tag after http://trac.webkit.org/changeset/139541.
Modified: trunk/Source/_javascript_Core/bytecode/Operands.h (139686 => 139687)
--- trunk/Source/_javascript_Core/bytecode/Operands.h 2013-01-15 00:51:30 UTC (rev 139686)
+++ trunk/Source/_javascript_Core/bytecode/Operands.h 2013-01-15 00:53:39 UTC (rev 139687)
@@ -192,15 +192,17 @@
template<typename T, typename Traits>
void dumpOperands(const Operands<T, Traits>& operands, PrintStream& out)
{
- for (size_t argument = 0; argument < operands.numberOfArguments(); ++argument) {
+ for (size_t argument = operands.numberOfArguments(); argument--;) {
if (argument)
out.printf(" ");
+ out.print("arg", argument, ":");
Traits::dump(operands.argument(argument), out);
}
out.printf(" : ");
for (size_t local = 0; local < operands.numberOfLocals(); ++local) {
if (local)
out.printf(" ");
+ out.print("r", local, ":");
Traits::dump(operands.local(local), out);
}
}
Modified: trunk/Source/_javascript_Core/dfg/DFGAbstractState.cpp (139686 => 139687)
--- trunk/Source/_javascript_Core/dfg/DFGAbstractState.cpp 2013-01-15 00:51:30 UTC (rev 139686)
+++ trunk/Source/_javascript_Core/dfg/DFGAbstractState.cpp 2013-01-15 00:53:39 UTC (rev 139687)
@@ -1860,16 +1860,14 @@
return false;
AbstractValue source;
+
+ Node& tailNode = m_graph[nodeIndex];
+ if (tailNode.variableAccessData()->isCaptured()) {
+ // If it's captured then we know that whatever value was stored into the variable last is the
+ // one we care about. This is true even if the variable at tail is dead, which might happen if
+ // the last thing we did to the variable was a GetLocal and then ended up now using the
+ // GetLocal's result.
- Node& node = m_graph[nodeIndex];
- if (!node.refCount())
- return false;
-
-#if DFG_ENABLE(DEBUG_PROPAGATION_VERBOSE)
- dataLogF(" It's live, node @%u.\n", nodeIndex);
-#endif
-
- if (node.variableAccessData()->isCaptured()) {
source = inVariable;
#if DFG_ENABLE(DEBUG_PROPAGATION_VERBOSE)
dataLogF(" Transfering ");
@@ -1877,6 +1875,25 @@
dataLogF(" from last access due to captured variable.\n");
#endif
} else {
+ if (!tailNode.shouldGenerate()) {
+ // If the node at tail is a GetLocal that is dead, then skip it to get to the Phi.
+ // The Phi may be live.
+ if (tailNode.op() != GetLocal)
+ return false;
+
+ nodeIndex = tailNode.child1().index();
+ ASSERT(m_graph[nodeIndex].op() == Phi);
+ if (!m_graph[nodeIndex].shouldGenerate())
+ return false;
+ }
+
+ Node& node = m_graph[nodeIndex];
+ ASSERT(node.shouldGenerate());
+
+#if DFG_ENABLE(DEBUG_PROPAGATION_VERBOSE)
+ dataLogF(" It's live, node @%u.\n", nodeIndex);
+#endif
+
switch (node.op()) {
case Phi:
case SetArgument:
_______________________________________________ webkit-changes mailing list webkit-changes@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-changes
