DFG should know that CreateThis can be effectful
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 26 Mar 2018 21:01:16 +0000 (21:01 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 26 Mar 2018 21:01:16 +0000 (21:01 +0000)
https://bugs.webkit.org/show_bug.cgi?id=184013

Reviewed by Saam Barati.

JSTests:

* stress/create-this-property-change.js: Added.
(Foo):
(RealBar):
(get if):
* stress/create-this-structure-change-without-cse.js: Added.
(Foo):
(RealBar):
(get if):
* stress/create-this-structure-change.js: Added.
(Foo):
(RealBar):
(get if):

Source/JavaScriptCore:

As shown in the tests added in JSTests, CreateThis can be effectful if the constructor this
is a proxy.

* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGClobberize.h:
(JSC::DFG::clobberize):

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

JSTests/ChangeLog
JSTests/stress/create-this-property-change.js [new file with mode: 0644]
JSTests/stress/create-this-structure-change-without-cse.js [new file with mode: 0644]
JSTests/stress/create-this-structure-change.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
Source/JavaScriptCore/dfg/DFGClobberize.h

index 62482a2..fbac095 100644 (file)
@@ -1,3 +1,23 @@
+2018-03-26  Filip Pizlo  <fpizlo@apple.com>
+
+        DFG should know that CreateThis can be effectful
+        https://bugs.webkit.org/show_bug.cgi?id=184013
+
+        Reviewed by Saam Barati.
+
+        * stress/create-this-property-change.js: Added.
+        (Foo):
+        (RealBar):
+        (get if):
+        * stress/create-this-structure-change-without-cse.js: Added.
+        (Foo):
+        (RealBar):
+        (get if):
+        * stress/create-this-structure-change.js: Added.
+        (Foo):
+        (RealBar):
+        (get if):
+
 2018-03-22  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [DFG] Introduces fused compare and jump
diff --git a/JSTests/stress/create-this-property-change.js b/JSTests/stress/create-this-property-change.js
new file mode 100644 (file)
index 0000000..6a0e8d3
--- /dev/null
@@ -0,0 +1,49 @@
+var globalO;
+
+function Foo()
+{
+    this.g = 42;
+}
+
+class RealBar extends Foo {
+    constructor()
+    {
+        var o = globalO;
+        var result = o.f;
+        super();
+        result += o.f;
+        this.result = result;
+    }
+}
+
+var doIntercept = false;
+var didExecuteFGetter = false;
+var Bar = new Proxy(RealBar, {
+    get: function(target, property, receiver) {
+        if (property == "prototype" && doIntercept) {
+            globalO.f = 666;
+            didExecuteFGetter = true;
+        }
+        return Reflect.get(target, property, receiver);
+    }
+});
+
+noInline(RealBar);
+
+for (var i = 0; i < 10000; ++i) {
+    (function() {
+        globalO = {f:43};
+        var result = new Bar().result;
+        if (result != 86)
+            throw "bad result in loop: " + result;
+    })();
+}
+
+doIntercept = true;
+globalO = {f:43};
+var result = new Bar().result;
+if (result != 709)
+    throw "bad result at end: " + result;
+if (!didExecuteFGetter)
+    throw "did not execute f getter";
+
diff --git a/JSTests/stress/create-this-structure-change-without-cse.js b/JSTests/stress/create-this-structure-change-without-cse.js
new file mode 100644 (file)
index 0000000..08642b5
--- /dev/null
@@ -0,0 +1,51 @@
+var globalO;
+
+function Foo()
+{
+    this.f = 42;
+}
+
+class RealBar extends Foo {
+    constructor()
+    {
+        var o = globalO;
+        var result = o.f;
+        super();
+        result += o.f;
+        this.result = result;
+    }
+}
+
+var doIntercept = false;
+var didExecuteFGetter = false;
+var Bar = new Proxy(RealBar, {
+    get: function(target, property, receiver) {
+        if (property == "prototype" && doIntercept) {
+            globalO.__defineGetter__("f", function() {
+                didExecuteFGetter = true;
+                return 666;
+            });
+        }
+        return Reflect.get(target, property, receiver);
+    }
+});
+
+noInline(RealBar);
+
+for (var i = 0; i < 10000; ++i) {
+    (function() {
+        globalO = {f:43};
+        var result = new Bar().result;
+        if (result != 86)
+            throw "bad result in loop: " + result;
+    })();
+}
+
+doIntercept = true;
+globalO = {f:43};
+var result = new Bar().result;
+if (result != 709)
+    throw "bad result at end: " + result;
+if (!didExecuteFGetter)
+    throw "did not execute f getter";
+
diff --git a/JSTests/stress/create-this-structure-change.js b/JSTests/stress/create-this-structure-change.js
new file mode 100644 (file)
index 0000000..68a9816
--- /dev/null
@@ -0,0 +1,51 @@
+var globalO;
+
+function Foo()
+{
+    this.g = 42;
+}
+
+class RealBar extends Foo {
+    constructor()
+    {
+        var o = globalO;
+        var result = o.f;
+        super();
+        result += o.f;
+        this.result = result;
+    }
+}
+
+var doIntercept = false;
+var didExecuteFGetter = false;
+var Bar = new Proxy(RealBar, {
+    get: function(target, property, receiver) {
+        if (property == "prototype" && doIntercept) {
+            globalO.__defineGetter__("f", function() {
+                didExecuteFGetter = true;
+                return 666;
+            });
+        }
+        return Reflect.get(target, property, receiver);
+    }
+});
+
+noInline(RealBar);
+
+for (var i = 0; i < 10000; ++i) {
+    (function() {
+        globalO = {f:43};
+        var result = new Bar().result;
+        if (result != 86)
+            throw "bad result in loop: " + result;
+    })();
+}
+
+doIntercept = true;
+globalO = {f:43};
+var result = new Bar().result;
+if (result != 709)
+    throw "bad result at end: " + result;
+if (!didExecuteFGetter)
+    throw "did not execute f getter";
+
index 22cee86..5d9ff26 100644 (file)
@@ -1,3 +1,18 @@
+2018-03-26  Filip Pizlo  <fpizlo@apple.com>
+
+        DFG should know that CreateThis can be effectful
+        https://bugs.webkit.org/show_bug.cgi?id=184013
+
+        Reviewed by Saam Barati.
+
+        As shown in the tests added in JSTests, CreateThis can be effectful if the constructor this
+        is a proxy.
+
+        * dfg/DFGAbstractInterpreterInlines.h:
+        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+        * dfg/DFGClobberize.h:
+        (JSC::DFG::clobberize):
+
 2018-03-25  Saam Barati  <sbarati@apple.com>
 
         Fix typo in JSC option name
index e7f1585..fc1a7c5 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -2274,6 +2274,7 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
                 }
             }
         }
+        clobberWorld(node->origin.semantic, clobberLimit);
         forNode(node).setType(m_graph, SpecFinalObject);
         break;
     }
index 6c54eb9..23e9f61 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -272,28 +272,12 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
         write(MathDotRandomState);
         return;
 
-    case HasGenericProperty:
-    case HasStructureProperty:
-    case GetPropertyEnumerator: {
-        read(World);
-        write(Heap);
-        return;
-    }
-
     case GetEnumerableLength: {
         read(Heap);
         write(SideState);
         return;
     }
 
-    case GetDirectPname: {
-        // This reads and writes heap because it can end up calling a generic getByVal 
-        // if the Structure changed, which could in turn end up calling a getter.
-        read(World);
-        write(Heap);
-        return;
-    }
-
     case ToIndexString:
     case GetEnumeratorStructurePname:
     case GetEnumeratorGenericPname: {
@@ -537,18 +521,12 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
         write(HeapObjectCount);
         return;
 
-    case ToObject:
-        read(World);
-        write(Heap);
-        return;
-
     case CallObjectConstructor:
         read(HeapObjectCount);
         write(HeapObjectCount);
         return;
 
     case ToThis:
-    case CreateThis:
         read(MiscFields);
         read(HeapObjectCount);
         write(HeapObjectCount);
@@ -645,6 +623,15 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
     case PutDynamicVar:
     case ResolveScopeForHoistingFuncDeclInEval:
     case ResolveScope:
+    case ToObject:
+    case HasGenericProperty:
+    case HasStructureProperty:
+    case GetPropertyEnumerator:
+    case GetDirectPname:
+    case InstanceOfCustom:
+    case ToNumber:
+    case NumberToStringWithRadix:
+    case CreateThis:
         read(World);
         write(Heap);
         return;
@@ -1031,11 +1018,6 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
         def(HeapLocation(InstanceOfLoc, JSCell_structureID, node->child1(), node->child2()), LazyNode(node));
         return;
 
-    case InstanceOfCustom:
-        read(World);
-        write(Heap);
-        return;
-
     case PutStructure:
         read(JSObject_butterfly);
         write(JSCell_structureID);
@@ -1571,12 +1553,6 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
         def(PureValue(node));
         return;
 
-    case ToNumber: {
-        read(World);
-        write(Heap);
-        return;
-    }
-        
     case ToString:
     case CallStringConstructor:
         switch (node->child1().useKind()) {
@@ -1719,12 +1695,6 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
         def(PureValue(node));
         return;
 
-    case NumberToStringWithRadix:
-        // If the radix is invalid, NumberToStringWithRadix can throw an error.
-        read(World);
-        write(Heap);
-        return;
-
     case NumberToStringWithValidRadixConstant:
         def(PureValue(node, node->validRadixConstant()));
         return;