Attachment 339195 Details for Bug 183172 – Patch

[patch] Patch

bug-183172-20180501155644.patch (text/plain), 7.55 KB, created by Robin Morisset on 2018-05-01 06:56:45 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Robin Morisset

Created: 2018-05-01 06:56:45 PDT

Size: 7.55 KB

patch

obsolete

>Subversion Revision: 231001
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index c95a4dadd730333809af2fb38a9cb3351e226e57..945a3c9f6bed479ecf51d5f2294fdf754a4cb476 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,21 @@
>+2018-05-01  Robin Morisset  <rmorisset@apple.com>
>+
>+        emitCodeToGetArgumentsArrayLength should not crash on PhantomNewArrayWithSpread
>+        https://bugs.webkit.org/show_bug.cgi?id=183172
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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.
>+
>+        * dfg/DFGArgumentsEliminationPhase.cpp:
>+        * dfg/DFGArgumentsUtilities.cpp:
>+        (JSC::DFG::emitCodeToGetArgumentsArrayLength):
>+
> 2018-04-25  Michael Catanzaro  <mcatanzaro@igalia.com>
> 
>         [GTK] Miscellaneous build cleanups
>diff --git a/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp b/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp
>index d1ccfec6c52799024bab375191e9be11985efae8..3f2131487a995f34f49fe610175db9c9b6b56f1b 100644
>--- a/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp
>@@ -128,7 +128,7 @@ private:
>                 case NewArrayWithSpread: {
>                     if (m_graph.isWatchingHavingABadTimeWatchpoint(node)) {
>                         BitVector* bitVector = node->bitVector();
>-                        // We only allow for Spreads to be of rest nodes for now.
>+                        // We only allow for Spreads to be of CreateRest or NewArrayBuffer nodes for now.
>                         bool isOK = true;
>                         for (unsigned i = 0; i < node->numChildren(); i++) {
>                             if (bitVector->get(i)) {
>@@ -320,8 +320,11 @@ private:
>                     break;
> 
>                 case GetArrayLength:
>-                    // FIXME: It would not be hard to support NewArrayWithSpread here if it is only over Spread(CreateRest) nodes.
>                     escape(node->child2(), node);
>+                    // Computing the length of a NewArrayWithSpread can require some additions.
>+                    // These additions can overflow if the array is sufficiently enormous, and in that case we will need to exit.
>+                    if ((node->child1()->op() == NewArrayWithSpread) && !node->origin.exitOK)
>+                        escape(node->child1(), node);
>                     break;
> 
>                 case NewArrayWithSpread: {
>diff --git a/Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp b/Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp
>index f4333e424d89f66b926f273928f202293dd1c87a..d12998918c233d49700495544f78eec1f51664c2 100644
>--- a/Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp
>@@ -65,11 +65,42 @@ Node* emitCodeToGetArgumentsArrayLength(
>     DFG_ASSERT(
>         graph, arguments,
>         arguments->op() == CreateDirectArguments || arguments->op() == CreateScopedArguments
>-        || arguments->op() == CreateClonedArguments || arguments->op() == CreateRest || arguments->op() == NewArrayBuffer
>+        || arguments->op() == CreateClonedArguments || arguments->op() == CreateRest
>+        || arguments->op() == NewArrayBuffer
>         || arguments->op() == PhantomDirectArguments || arguments->op() == PhantomClonedArguments
>-        || arguments->op() == PhantomCreateRest || arguments->op() == PhantomNewArrayBuffer,
>+        || arguments->op() == PhantomCreateRest || arguments->op() == PhantomNewArrayBuffer
>+        || arguments->op() == PhantomNewArrayWithSpread,
>         arguments->op());
> 
>+    if (arguments->op() == PhantomNewArrayWithSpread) {
>+        unsigned numberOfNonSpreadArguments = 0;
>+        BitVector* bitVector = arguments->bitVector();
>+        Node* currentSum = nullptr;
>+        for (unsigned i = 0; i < arguments->numChildren(); i++) {
>+            if (bitVector->get(i)) {
>+                Node* child = graph.varArgChild(arguments, i).node();
>+                DFG_ASSERT(graph, child, child->op() == PhantomSpread, child->op());
>+                DFG_ASSERT(graph, child->child1().node(),
>+                    child->child1()->op() == PhantomCreateRest || child->child1()->op() == PhantomNewArrayBuffer,
>+                    child->child1()->op());
>+                Node* lengthOfChild = emitCodeToGetArgumentsArrayLength(insertionSet, child->child1().node(), nodeIndex, origin);
>+                if (currentSum)
>+                    currentSum = insertionSet.insertNode(nodeIndex, SpecInt32Only, ArithAdd, origin, OpInfo(Arith::CheckOverflow), Edge(currentSum, Int32Use), Edge(lengthOfChild, Int32Use));
>+                else
>+                    currentSum = lengthOfChild;
>+            } else
>+                numberOfNonSpreadArguments++;
>+        }
>+        if (currentSum) {
>+            if (!numberOfNonSpreadArguments)
>+                return currentSum;
>+            return insertionSet.insertNode(
>+                nodeIndex, SpecInt32Only, ArithAdd, origin, OpInfo(Arith::CheckOverflow), Edge(currentSum, Int32Use),
>+                insertionSet.insertConstantForUse(nodeIndex, origin, jsNumber(numberOfNonSpreadArguments), Int32Use));
>+        }
>+        return insertionSet.insertConstant(nodeIndex, origin, jsNumber(numberOfNonSpreadArguments));
>+    }
>+
>     if (arguments->op() == NewArrayBuffer || arguments->op() == PhantomNewArrayBuffer) {
>         return insertionSet.insertConstant(
>             nodeIndex, origin, jsNumber(arguments->castOperand<JSFixedArray*>()->length()));
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index 6629c69ad2e12746f713312ea8e89d3f6a76bda8..da74bdc2f3478b0987ee2193f67b127fdd5a84e4 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,15 @@
>+2018-05-01  Robin Morisset  <rmorisset@apple.com>
>+
>+        emitCodeToGetArgumentsArrayLength should not crash on PhantomNewArrayWithSpread
>+        https://bugs.webkit.org/show_bug.cgi?id=183172
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/length-of-new-array-with-spread.js: Added.
>+        (foo):
>+        (bar):
>+        (baz):
>+
> 2018-04-24  Keith Miller  <keith_miller@apple.com>
> 
>         fromCharCode is missing some exception checks
>diff --git a/JSTests/stress/length-of-new-array-with-spread.js b/JSTests/stress/length-of-new-array-with-spread.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..215ee5559a44ccd88bac8952843f61fd729a3ce1
>--- /dev/null
>+++ b/JSTests/stress/length-of-new-array-with-spread.js
>@@ -0,0 +1,31 @@
>+function foo()
>+{
>+    var a = [1, 2];
>+    var l = [...a, 42, ...a].length;
>+    if (l != 5)
>+        throw "Wrong length in foo: " + l;
>+}
>+noInline(foo);
>+
>+function bar(...b)
>+{
>+    var l = [...b, 43, ...b].length;
>+    if (l != 7)
>+        throw "Wrong length in bar: " + l
>+}
>+noInline(bar);
>+
>+function baz(arg0, ...c)
>+{
>+    var x = [...c, ...c];
>+    var l = [...x, ...x, ...x].length;
>+    if (l != 24)
>+        throw "Wrong length in baz: " + l
>+}
>+noInline(baz);
>+
>+for (var i = 0; i < 10000; ++i) {
>+    foo();
>+    bar(1, 2, 3);
>+    baz(0, 1, 2, 3, 4);
>+}

Actions: View | Formatted Diff | Diff

Attachments on bug 183172: 338903 | 338907 | 339195 | 339198