Attachment 338903 Details for Bug 183172 – Patch

[patch] Patch

bug-183172-20180426211442.patch (text/plain), 7.42 KB, created by Robin Morisset on 2018-04-26 12:14:44 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Robin Morisset

Created: 2018-04-26 12:14:44 PDT

Size: 7.42 KB

patch

obsolete

>Subversion Revision: 231001
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index c95a4dadd730333809af2fb38a9cb3351e226e57..d2e4697de1ab1cc10942c5ca0bf38f1b346fc45f 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,21 @@
>+2018-04-26  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..c24dd33e373caf7930e25c564569e0100dc89c99 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,12 @@ 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->child1()->op() == PhantomNewArrayWithSpread)
>+                        && !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..28085b03bfce7c5609b69b54d7f59d4146b614d4 100644
>--- a/Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGArgumentsUtilities.cpp
>@@ -65,11 +65,45 @@ 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() == NewArrayWithSpread || arguments->op() == PhantomNewArrayWithSpread) {
>+        unsigned countNonSpreadArguments = 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() == 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());
>+                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
>+                countNonSpreadArguments++;
>+        }
>+        if (currentSum) {
>+            if (!countNonSpreadArguments)
>+                return currentSum;
>+            return insertionSet.insertNode(
>+                nodeIndex, SpecInt32Only, ArithAdd, origin, OpInfo(Arith::CheckOverflow), Edge(currentSum, Int32Use),
>+                insertionSet.insertConstantForUse(nodeIndex, origin, jsNumber(countNonSpreadArguments), Int32Use));
>+        }
>+        return insertionSet.insertConstant(nodeIndex, origin, jsNumber(countNonSpreadArguments));
>+    }
>+
>     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..78c761770c0965c224c7bf8b4fdadd508941782c 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,13 @@
>+2018-04-26  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.
>+        (f):
>+
> 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..b76e26331a89edfa61e195a00f7c357c1538c81f
>--- /dev/null
>+++ b/JSTests/stress/length-of-new-array-with-spread.js
>@@ -0,0 +1,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();

Actions: View | Formatted Diff | Diff

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