SVN: rev228977 Build with: perl Tools/Scripts/build-jsc --jsc-only --debug Executing the following test case leads to this assertion: --- DFG ASSERTION FAILED: arguments->op() == CreateDirectArguments || arguments->op() == CreateScopedArguments || arguments->op() == CreateClonedArguments || arguments->op() == CreateRest || arguments->op() = = NewArrayBuffer || arguments->op() == PhantomDirectArguments || arguments->op() == PhantomClonedArguments || arguments->op() == PhantomCreateRest || arguments->op() == PhantomNewArrayBuffer ../../Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp(71) : JSC::DFG::Node* JSC::DFG::emitCodeToGetArgumentsArrayLength(JSC::DFG::InsertionSet&, JSC::DFG::Node*, unsigned int, JSC::DFG::NodeOrigin) --- Test case: --- function f() { var a = [1]; for (var i = 0; i < 1000000; ++i) { [...a].length; } } f(); ---
I was able to reproduce this. I am looking into it.
I found the problem and discussed it with Saam. DFGArgumentsEliminationPhase.cpp currently believes that allocations of NewArrayWithSpread can be deleted if they are only used by GetArrayLength, but when it then calls emitCodeToGetArgumentsArrayLength, the latter has no idea what to do with GetArrayLength. I fix the problem by teaching emitCodeToGetArgumentsArrayLength how to deal with GetArrayLength. Because this requires emitting an Add that can overflow and thus exit, we also tell DFGArgumentsEliminationPhase to give up on eliminating a NewArrayWithSpread when it is used by a GetArrayLength that is not allowed to exit.
Created attachment 338903 [details] Patch
Comment on attachment 338903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338903&action=review > Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:326 > + if ((node->child1()->op() == NewArrayWithSpread || node->child1()->op() == PhantomNewArrayWithSpread) PhantomNewArrayWithSpread is not possible at this point in the transformation > Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:76 > + unsigned countNonSpreadArguments = 0; style nit: I would name this "numberOfNonSpreadArguments" > JSTests/stress/length-of-new-array-with-spread.js:9 > +function f() { > + var a = [1, 2]; > + for (var i = 0; i < 10000; ++i) { > + var l = [...a, 42, ...a].length; > + if (l != 5) > + throw "Wrong length: " + l; > + } > +} > +f(); Let's add some more tests: where a may be a rest parameter, etc.
Created attachment 338907 [details] Patch
(In reply to Saam Barati from comment #4) > Comment on attachment 338903 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=338903&action=review > > > Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:326 > > + if ((node->child1()->op() == NewArrayWithSpread || node->child1()->op() == PhantomNewArrayWithSpread) > > PhantomNewArrayWithSpread is not possible at this point in the transformation Right, I removed it from the test. > > Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:76 > > + unsigned countNonSpreadArguments = 0; > > style nit: I would name this "numberOfNonSpreadArguments" Done. > > JSTests/stress/length-of-new-array-with-spread.js:9 > > +function f() { > > + var a = [1, 2]; > > + for (var i = 0; i < 10000; ++i) { > > + var l = [...a, 42, ...a].length; > > + if (l != 5) > > + throw "Wrong length: " + l; > > + } > > +} > > +f(); > > Let's add some more tests: > where a may be a rest parameter, etc. I've added two more tests, tell me if more are needed.
Comment on attachment 338907 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338907&action=review > Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:75 > + if (arguments->op() == NewArrayWithSpread || arguments->op() == PhantomNewArrayWithSpread) { I don't think we want to actually support NewArrayWithSpread here, just PhantomNewArrayWithSpread, reasons below > Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:88 > + DFG_ASSERT(graph, child, child->op() == Spread || child->op() == PhantomSpread, child->op()); > + DFG_ASSERT(graph, child->child1().node(), > + child->child1()->op() == CreateRest > + || child->child1()->op() == NewArrayBuffer > + || child->child1()->op() == PhantomCreateRest > + || child->child1()->op() == PhantomNewArrayBuffer, > + child->child1()->op()); These assertions only holds if it's PhantomNewArrayWithSpread. And if it's PhantomNewArrayWithSpread, then its children must also be Phantoms. > Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:104 > + return insertionSet.insertConstant(nodeIndex, origin, jsNumber(numberOfNonSpreadArguments)); It would be really weird if this code were reached...
Created attachment 339195 [details] Patch
(In reply to Saam Barati from comment #7) > Comment on attachment 338907 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=338907&action=review > > > Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:75 > > + if (arguments->op() == NewArrayWithSpread || arguments->op() == PhantomNewArrayWithSpread) { > > I don't think we want to actually support NewArrayWithSpread here, just > PhantomNewArrayWithSpread, reasons below Fixed. > > > Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:88 > > + DFG_ASSERT(graph, child, child->op() == Spread || child->op() == PhantomSpread, child->op()); > > + DFG_ASSERT(graph, child->child1().node(), > > + child->child1()->op() == CreateRest > > + || child->child1()->op() == NewArrayBuffer > > + || child->child1()->op() == PhantomCreateRest > > + || child->child1()->op() == PhantomNewArrayBuffer, > > + child->child1()->op()); > > These assertions only holds if it's PhantomNewArrayWithSpread. And if it's > PhantomNewArrayWithSpread, then its children must also be Phantoms. Fixed. > > > Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp:104 > > + return insertionSet.insertConstant(nodeIndex, origin, jsNumber(numberOfNonSpreadArguments)); > > It would be really weird if this code were reached... I agree it does not seem likely with the code as it is currently, but I like it in case the Spread nodes were eliminated (for example through some kind of constant folding) happening earlier.
Comment on attachment 339195 [details] Patch Attachment 339195 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7524394 New failing tests: http/tests/security/canvas-remote-read-remote-video-localhost.html
Created attachment 339198 [details] Archive of layout-test-results from ews200 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 339195 [details] Patch The windows test failure seem unrelated.
Comment on attachment 339195 [details] Patch Clearing flags on attachment: 339195 Committed r231229: <https://trac.webkit.org/changeset/231229>
All reviewed patches have been landed. Closing bug.
<rdar://problem/39899574>
*** Bug 183770 has been marked as a duplicate of this bug. ***