Allocation elimination of rest parameter doesn't take into account indexed properties...
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 2 Nov 2016 23:15:33 +0000 (23:15 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 2 Nov 2016 23:15:33 +0000 (23:15 +0000)
https://bugs.webkit.org/show_bug.cgi?id=164301

Reviewed by Geoffrey Garen.

JSTests:

* stress/rest-parameter-allocation-elimination-watchpoints-2.js: Added.
(assert):
(foo):
* stress/rest-parameter-allocation-elimination-watchpoints-3.js: Added.
(assert):
(foo):
* stress/rest-parameter-allocation-elimination-watchpoints-4.js: Added.
(assert):
(foo):
* stress/rest-parameter-allocation-elimination-watchpoints-5.js: Added.
(assert):
(foo):
* stress/rest-parameter-allocation-elimination-watchpoints-6.js: Added.
(assert):
(foo):
* stress/rest-parameter-allocation-elimination-watchpoints.js: Added.
(assert):
(foo):

Source/JavaScriptCore:

We weren't taking into account indexed properties on the __proto__
of the rest parameter. This made the code for doing out of bound
accesses incorrect since it just assumed it's safe for the result of
an out of bound access to be undefined. This broke the semantics
of JS code when there was an indexed property on the Array.prototype
or Object.prototype.

This patch makes sure we set up the proper watchpoints for making
sure out of bound accesses are safe to return undefined.

* dfg/DFGArgumentsEliminationPhase.cpp:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@208311 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/rest-parameter-allocation-elimination-watchpoints-2.js [new file with mode: 0644]
JSTests/stress/rest-parameter-allocation-elimination-watchpoints-3.js [new file with mode: 0644]
JSTests/stress/rest-parameter-allocation-elimination-watchpoints-4.js [new file with mode: 0644]
JSTests/stress/rest-parameter-allocation-elimination-watchpoints-5.js [new file with mode: 0644]
JSTests/stress/rest-parameter-allocation-elimination-watchpoints-6.js [new file with mode: 0644]
JSTests/stress/rest-parameter-allocation-elimination-watchpoints.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp

index 0f5dc71..5e0ceeb 100644 (file)
@@ -1,3 +1,29 @@
+2016-11-02  Saam Barati  <sbarati@apple.com>
+
+        Allocation elimination of rest parameter doesn't take into account indexed properties on Array.prototype/Object.prototype
+        https://bugs.webkit.org/show_bug.cgi?id=164301
+
+        Reviewed by Geoffrey Garen.
+
+        * stress/rest-parameter-allocation-elimination-watchpoints-2.js: Added.
+        (assert):
+        (foo):
+        * stress/rest-parameter-allocation-elimination-watchpoints-3.js: Added.
+        (assert):
+        (foo):
+        * stress/rest-parameter-allocation-elimination-watchpoints-4.js: Added.
+        (assert):
+        (foo):
+        * stress/rest-parameter-allocation-elimination-watchpoints-5.js: Added.
+        (assert):
+        (foo):
+        * stress/rest-parameter-allocation-elimination-watchpoints-6.js: Added.
+        (assert):
+        (foo):
+        * stress/rest-parameter-allocation-elimination-watchpoints.js: Added.
+        (assert):
+        (foo):
+
 2016-11-01  Saam Barati  <sbarati@apple.com>
 
         We should be able to eliminate rest parameter allocations
diff --git a/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-2.js b/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-2.js
new file mode 100644 (file)
index 0000000..c14cac8
--- /dev/null
@@ -0,0 +1,19 @@
+function assert(b) {
+    if (!b)
+        throw new Error;
+}
+noInline(assert);
+
+function foo(...args) {
+    return args[0];
+}
+noInline(foo);
+
+for (let i = 0; i < 10000; i++) {
+    // Warm it up on in bound accesses.
+    assert(foo(i) === i);
+}
+
+Array.prototype[0] = 50;
+for (let i = 0; i < 10000; i++)
+    assert(foo() === 50);
diff --git a/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-3.js b/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-3.js
new file mode 100644 (file)
index 0000000..d258687
--- /dev/null
@@ -0,0 +1,22 @@
+function assert(b) {
+    if (!b)
+        throw new Error;
+}
+noInline(assert);
+
+function foo(...args) {
+    return args[0];
+}
+noInline(foo);
+
+for (let i = 0; i < 10000; i++) {
+    // Warm it up on both in bound and out of bound accesses.
+    if (i % 2)
+        assert(foo(i) === i);
+    else
+        assert(foo() === undefined);
+}
+
+Object.prototype[0] = 50;
+for (let i = 0; i < 10000; i++)
+    assert(foo() === 50);
diff --git a/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-4.js b/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-4.js
new file mode 100644 (file)
index 0000000..4226770
--- /dev/null
@@ -0,0 +1,20 @@
+
+function assert(b) {
+    if (!b)
+        throw new Error;
+}
+noInline(assert);
+
+function foo(...args) {
+    return args[0];
+}
+noInline(foo);
+
+for (let i = 0; i < 10000; i++) {
+    // Warm it up on in bound accesses.
+    assert(foo(i) === i);
+}
+
+Object.prototype[0] = 50;
+for (let i = 0; i < 10000; i++)
+    assert(foo() === 50);
diff --git a/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-5.js b/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-5.js
new file mode 100644 (file)
index 0000000..5449872
--- /dev/null
@@ -0,0 +1,25 @@
+function assert(b) {
+    if (!b)
+        throw new Error;
+}
+noInline(assert);
+
+function foo(...args) {
+    return args[0];
+}
+noInline(foo);
+
+for (let i = 0; i < 10000; i++) {
+    // Warm it up on both in bound and out of bound accesses.
+    if (i % 2)
+        assert(foo(i) === i);
+    else
+        assert(foo() === undefined);
+}
+
+let newProto = [50];
+newProto.__proto__ = null;
+
+Array.prototype.__proto__ = newProto;
+for (let i = 0; i < 10000; i++)
+    assert(foo() === 50);
diff --git a/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-6.js b/JSTests/stress/rest-parameter-allocation-elimination-watchpoints-6.js
new file mode 100644 (file)
index 0000000..1de398a
--- /dev/null
@@ -0,0 +1,24 @@
+function assert(b) {
+    if (!b)
+        throw new Error;
+}
+noInline(assert);
+
+function foo(...args) {
+    return args[0];
+}
+noInline(foo);
+
+for (let i = 0; i < 10000; i++) {
+    // Warm it up on both in bound and out of bound accesses.
+    if (i % 2)
+        assert(foo(i) === i);
+    else
+        assert(foo() === undefined);
+}
+
+let newProto = [50];
+newProto.__proto__ = null;
+Object.prototype.__proto__ = newProto;
+for (let i = 0; i < 10000; i++)
+    assert(foo() === 50);
diff --git a/JSTests/stress/rest-parameter-allocation-elimination-watchpoints.js b/JSTests/stress/rest-parameter-allocation-elimination-watchpoints.js
new file mode 100644 (file)
index 0000000..8a70e42
--- /dev/null
@@ -0,0 +1,22 @@
+function assert(b) {
+    if (!b)
+        throw new Error;
+}
+noInline(assert);
+
+function foo(...args) {
+    return args[0];
+}
+noInline(foo);
+
+for (let i = 0; i < 10000; i++) {
+    // Warm it up on both in bound and out of bound accesses.
+    if (i % 2)
+        assert(foo(i) === i);
+    else
+        assert(foo() === undefined);
+}
+
+Array.prototype[0] = 50;
+for (let i = 0; i < 10000; i++)
+    assert(foo() === 50);
index 70aeeaf..90ca956 100644 (file)
@@ -1,3 +1,22 @@
+2016-11-02  Saam Barati  <sbarati@apple.com>
+
+        Allocation elimination of rest parameter doesn't take into account indexed properties on Array.prototype/Object.prototype
+        https://bugs.webkit.org/show_bug.cgi?id=164301
+
+        Reviewed by Geoffrey Garen.
+
+        We weren't taking into account indexed properties on the __proto__
+        of the rest parameter. This made the code for doing out of bound
+        accesses incorrect since it just assumed it's safe for the result of
+        an out of bound access to be undefined. This broke the semantics
+        of JS code when there was an indexed property on the Array.prototype
+        or Object.prototype.
+
+        This patch makes sure we set up the proper watchpoints for making
+        sure out of bound accesses are safe to return undefined.
+
+        * dfg/DFGArgumentsEliminationPhase.cpp:
+
 2016-11-02  Geoffrey Garen  <ggaren@apple.com>
 
         One file per class for CodeBlock.h/.cpp
index 97879bb..7a0c6a5 100644 (file)
@@ -28,6 +28,7 @@
 
 #if ENABLE(DFG_JIT)
 
+#include "ArrayPrototype.h"
 #include "BytecodeLivenessAnalysisInlines.h"
 #include "ClonedArguments.h"
 #include "DFGArgumentsUtilities.h"
@@ -154,13 +155,25 @@ private:
                 if (mode.isInBounds())
                     break;
                 
-                // If we're out-of-bounds then we proceed only if the object prototype is
-                // sane (i.e. doesn't have indexed properties).
+                // If we're out-of-bounds then we proceed only if the prototype chain
+                // for the allocation is sane (i.e. doesn't have indexed properties).
                 JSGlobalObject* globalObject = m_graph.globalObjectFor(edge->origin.semantic);
-                if (globalObject->objectPrototypeIsSane()) {
-                    m_graph.watchpoints().addLazily(globalObject->objectPrototype()->structure()->transitionWatchpointSet());
-                    if (globalObject->objectPrototypeIsSane())
+                InlineWatchpointSet& objectPrototypeTransition = globalObject->objectPrototype()->structure()->transitionWatchpointSet();
+                if (edge->op() == CreateRest) {
+                    InlineWatchpointSet& arrayPrototypeTransition = globalObject->arrayPrototype()->structure()->transitionWatchpointSet();
+                    if (arrayPrototypeTransition.isStillValid() 
+                        && objectPrototypeTransition.isStillValid() 
+                        && globalObject->arrayPrototypeChainIsSane()) {
+                        m_graph.watchpoints().addLazily(arrayPrototypeTransition);
+                        m_graph.watchpoints().addLazily(objectPrototypeTransition);
                         break;
+                    }
+                } else {
+                    if (objectPrototypeTransition.isStillValid() 
+                        && globalObject->objectPrototypeIsSane()) {
+                        m_graph.watchpoints().addLazily(objectPrototypeTransition);
+                        break;
+                    }
                 }
                 escape(edge, source);
                 break;