Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (184780 => 184781)
--- trunk/Source/_javascript_Core/ChangeLog 2015-05-22 18:43:42 UTC (rev 184780)
+++ trunk/Source/_javascript_Core/ChangeLog 2015-05-22 18:48:03 UTC (rev 184781)
@@ -1,3 +1,17 @@
+2015-05-21 Filip Pizlo <[email protected]>
+
+ Arguments elimination phase mishandles arity check failure in its reduction of LoadVarargs to GetStack/PutStacks
+ https://bugs.webkit.org/show_bug.cgi?id=145298
+
+ Reviewed by Geoffrey Garen.
+
+ * dfg/DFGArgumentsEliminationPhase.cpp: Fix the bug. I restructured the loop to make it more obvious that we're initializing everything that we're supposed to initialize.
+ * dfg/DFGNode.h: Add a comment to clarify something I was confused about while writing this code.
+ * dfg/DFGPutStackSinkingPhase.cpp: Hacking on PutStacks made me think deep thoughts, and I added some FIXMEs.
+ * tests/stress/fold-load-varargs-arity-check-fail-barely.js: Added. This test crashes or fails before this patch.
+ * tests/stress/fold-load-varargs-arity-check-fail.js: Added. This is even more sure to crash or fail.
+ * tests/stress/simplify-varargs-mandatory-minimum-smaller-than-limit.js: Added. Not sure if we had coverage for this case before.
+
2015-05-22 Basile Clement <[email protected]>
Allow DFGClobberize to return non-node constants that must be later created
Modified: trunk/Source/_javascript_Core/dfg/DFGArgumentsEliminationPhase.cpp (184780 => 184781)
--- trunk/Source/_javascript_Core/dfg/DFGArgumentsEliminationPhase.cpp 2015-05-22 18:43:42 UTC (rev 184780)
+++ trunk/Source/_javascript_Core/dfg/DFGArgumentsEliminationPhase.cpp 2015-05-22 18:48:03 UTC (rev 184781)
@@ -496,24 +496,52 @@
OpInfo(m_graph.m_stackAccessData.add(varargsData->count, FlushedInt32)),
Edge(argumentCount, Int32Use));
- for (unsigned i = 1 + varargsData->offset; i < inlineCallFrame->arguments.size(); ++i) {
- StackAccessData* data = ""
- virtualRegisterForArgument(i) + inlineCallFrame->stackOffset,
- FlushedJSValue);
+ DFG_ASSERT(m_graph, node, varargsData->limit - 1 >= varargsData->mandatoryMinimum);
+ // Define our limit to not include "this", since that's a bit easier to reason about.
+ unsigned limit = varargsData->limit - 1;
+ Node* undefined = nullptr;
+ for (unsigned storeIndex = 0; storeIndex < limit; ++storeIndex) {
+ // First determine if we have an element we can load, and load it if
+ // possible.
- Node* value = insertionSet.insertNode(
- nodeIndex, SpecNone, GetStack, node->origin, OpInfo(data));
+ unsigned loadIndex = storeIndex + varargsData->offset;
- VirtualRegister reg = varargsData->start + i - 1 - varargsData->offset;
+ Node* value;
+ if (loadIndex + 1 < inlineCallFrame->arguments.size()) {
+ VirtualRegister reg =
+ virtualRegisterForArgument(loadIndex + 1) +
+ inlineCallFrame->stackOffset;
+ StackAccessData* data = ""
+ reg, FlushedJSValue);
+
+ value = insertionSet.insertNode(
+ nodeIndex, SpecNone, GetStack, node->origin, OpInfo(data));
+ } else {
+ // Check if this an element that we must initialize.
+ if (storeIndex >= varargsData->mandatoryMinimum) {
+ // It's not. We're done.
+ break;
+ }
+
+ if (!undefined) {
+ undefined = insertionSet.insertConstant(
+ nodeIndex, node->origin, jsUndefined());
+ }
+ value = undefined;
+ }
+ // Now that we have a value, store it.
+
+ VirtualRegister reg = varargsData->start + storeIndex;
+ StackAccessData* data =
+ m_graph.m_stackAccessData.add(reg, FlushedJSValue);
+
insertionSet.insertNode(
nodeIndex, SpecNone, MovHint, node->origin, OpInfo(reg.offset()),
Edge(value));
-
- data = "" FlushedJSValue);
-
insertionSet.insertNode(
- nodeIndex, SpecNone, PutStack, node->origin, OpInfo(data), Edge(value));
+ nodeIndex, SpecNone, PutStack, node->origin, OpInfo(data),
+ Edge(value));
}
node->remove();
Modified: trunk/Source/_javascript_Core/dfg/DFGNode.h (184780 => 184781)
--- trunk/Source/_javascript_Core/dfg/DFGNode.h 2015-05-22 18:43:42 UTC (rev 184780)
+++ trunk/Source/_javascript_Core/dfg/DFGNode.h 2015-05-22 18:48:03 UTC (rev 184781)
@@ -190,7 +190,7 @@
};
struct LoadVarargsData {
- VirtualRegister start; // Local for the first element.
+ VirtualRegister start; // Local for the first element. This is the first actual argument, not this.
VirtualRegister count; // Local for the count.
VirtualRegister machineStart;
VirtualRegister machineCount;
Modified: trunk/Source/_javascript_Core/dfg/DFGPutStackSinkingPhase.cpp (184780 => 184781)
--- trunk/Source/_javascript_Core/dfg/DFGPutStackSinkingPhase.cpp 2015-05-22 18:43:42 UTC (rev 184780)
+++ trunk/Source/_javascript_Core/dfg/DFGPutStackSinkingPhase.cpp 2015-05-22 18:48:03 UTC (rev 184781)
@@ -64,6 +64,13 @@
// after SSA conversion, we have almost a guarantee that the Phi graph we produce here would
// be trivially redundant to the one we already have.
+ // FIXME: This phase doesn't adequately use KillStacks. KillStack can be viewed as a def.
+ // This is mostly inconsequential; it would be a bug to have a local live at a KillStack.
+ // More important is that KillStack should swallow any deferral. After a KillStack, the
+ // local should behave like a TOP deferral because it would be invalid for anyone to trust
+ // the stack. It's not clear to me if this is important or not.
+ // https://bugs.webkit.org/show_bug.cgi?id=145296
+
if (verbose) {
dataLog("Graph before PutStack sinking:\n");
m_graph.dump();
@@ -107,6 +114,10 @@
live.operand(operand) = true;
};
+ // FIXME: This might mishandle LoadVarargs and ForwardVarargs. It might make us
+ // think that the locals being written are stack-live here. They aren't. This
+ // should be harmless since we overwrite them anyway, but still, it's sloppy.
+ // https://bugs.webkit.org/show_bug.cgi?id=145295
preciseLocalClobberize(
m_graph, node, escapeHandler, escapeHandler,
[&] (VirtualRegister operand, LazyNode source) {
@@ -487,7 +498,7 @@
insertionSet.execute(block);
}
- // Finally eliminate the sunken PutStacks by turning them into Phantoms. This keeps whatever
+ // Finally eliminate the sunken PutStacks by turning them into Checks. This keeps whatever
// type check they were doing. Also prepend KillStacks to them to ensure that we know that
// the relevant value was *not* stored to the stack.
for (BasicBlock* block : m_graph.blocksInNaturalOrder()) {
Added: trunk/Source/_javascript_Core/tests/stress/fold-load-varargs-arity-check-fail-barely.js (0 => 184781)
--- trunk/Source/_javascript_Core/tests/stress/fold-load-varargs-arity-check-fail-barely.js (rev 0)
+++ trunk/Source/_javascript_Core/tests/stress/fold-load-varargs-arity-check-fail-barely.js 2015-05-22 18:48:03 UTC (rev 184781)
@@ -0,0 +1,28 @@
+function foo(a, b) {
+ return [a, b];
+}
+
+function bar() {
+ return foo.apply(this, arguments);
+}
+
+function baz() {
+ return bar(42);
+}
+
+noInline(baz);
+
+for (var i = 0; i < 10000; ++i) {
+ var result = baz();
+ if (!(result instanceof Array))
+ throw "Error: result is not an array.";
+ if (result.length != 2)
+ throw "Error: result doesn't have length 4.";
+ if (result[0] != 42)
+ throw "Error: first element is not 42: " + result[0];
+ for (var j = 1; j < 2; ++j) {
+ if (result[j] !== void 0)
+ throw "Error: element " + j + " is not undefined: " + result[j];
+ }
+}
+
Added: trunk/Source/_javascript_Core/tests/stress/fold-load-varargs-arity-check-fail.js (0 => 184781)
--- trunk/Source/_javascript_Core/tests/stress/fold-load-varargs-arity-check-fail.js (rev 0)
+++ trunk/Source/_javascript_Core/tests/stress/fold-load-varargs-arity-check-fail.js 2015-05-22 18:48:03 UTC (rev 184781)
@@ -0,0 +1,29 @@
+function foo(a, b, c, d) {
+ return [a, b, c, d];
+}
+
+function bar() {
+ return foo.apply(this, arguments);
+}
+
+function baz() {
+ return bar(42);
+}
+
+noInline(baz);
+
+for (var i = 0; i < 10000; ++i) {
+ var result = baz();
+ if (!(result instanceof Array))
+ throw "Error: result is not an array.";
+ if (result.length != 4)
+ throw "Error: result doesn't have length 4.";
+ if (result[0] != 42)
+ throw "Error: first element is not 42: " + result[0];
+ for (var j = 1; j < 4; ++j) {
+ if (result[j] !== void 0)
+ throw "Error: element " + j + " is not undefined: " + result[j];
+ }
+}
+
+
Added: trunk/Source/_javascript_Core/tests/stress/simplify-varargs-mandatory-minimum-smaller-than-limit.js (0 => 184781)
--- trunk/Source/_javascript_Core/tests/stress/simplify-varargs-mandatory-minimum-smaller-than-limit.js (rev 0)
+++ trunk/Source/_javascript_Core/tests/stress/simplify-varargs-mandatory-minimum-smaller-than-limit.js 2015-05-22 18:48:03 UTC (rev 184781)
@@ -0,0 +1,15 @@
+function foo() { return 42; }
+
+function bar() { return foo.apply(this, arguments); }
+
+function baz() { return bar(1, 2, 3); }
+
+noInline(baz);
+
+for (var i = 0; i < 10000; ++i) {
+ var result = baz();
+ if (result != 42)
+ throw "Error: bad result: " + result;
+}
+
+
