DFG models InstanceOf incorrectly
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 16 May 2018 21:02:49 +0000 (21:02 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 16 May 2018 21:02:49 +0000 (21:02 +0000)
https://bugs.webkit.org/show_bug.cgi?id=185694

Reviewed by Keith Miller.
JSTests:

* stress/instanceof-proxy-check-structure.js: Added.
(Foo):
(Bar):
(doBadThings):
(getPrototypeOf):
(foo):
(i.new.Bar):
(new.Bar):
* stress/instanceof-proxy-loop.js: Added.
(Foo):
(Bar):
(doBadThings):
(getPrototypeOf):
(foo):
* stress/instanceof-proxy.js: Added.
(Foo):
(Bar):
(doBadThings):
(getPrototypeOf):
(foo):

Source/JavaScriptCore:

Proxies mean that InstanceOf can have effects. Exceptions mean that it's illegal to DCE it or
hoist it.

* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGClobberize.h:
(JSC::DFG::clobberize):
* dfg/DFGHeapLocation.cpp:
(WTF::printInternal):
* dfg/DFGHeapLocation.h:
* dfg/DFGNodeType.h:

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

JSTests/ChangeLog
JSTests/stress/instanceof-proxy-check-structure.js [new file with mode: 0644]
JSTests/stress/instanceof-proxy-loop.js [new file with mode: 0644]
JSTests/stress/instanceof-proxy.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
Source/JavaScriptCore/dfg/DFGClobberize.h
Source/JavaScriptCore/dfg/DFGHeapLocation.cpp
Source/JavaScriptCore/dfg/DFGHeapLocation.h
Source/JavaScriptCore/dfg/DFGNodeType.h

index 2a8de37..b04b705 100644 (file)
@@ -1,3 +1,31 @@
+2018-05-16  Filip Pizlo  <fpizlo@apple.com>
+
+        DFG models InstanceOf incorrectly
+        https://bugs.webkit.org/show_bug.cgi?id=185694
+
+        Reviewed by Keith Miller.
+
+        * stress/instanceof-proxy-check-structure.js: Added.
+        (Foo):
+        (Bar):
+        (doBadThings):
+        (getPrototypeOf):
+        (foo):
+        (i.new.Bar):
+        (new.Bar):
+        * stress/instanceof-proxy-loop.js: Added.
+        (Foo):
+        (Bar):
+        (doBadThings):
+        (getPrototypeOf):
+        (foo):
+        * stress/instanceof-proxy.js: Added.
+        (Foo):
+        (Bar):
+        (doBadThings):
+        (getPrototypeOf):
+        (foo):
+
 2018-05-16  Caio Lima  <ticaiolima@gmail.com>
 
         [ESNext][BigInt] Implement support for "/" operation
diff --git a/JSTests/stress/instanceof-proxy-check-structure.js b/JSTests/stress/instanceof-proxy-check-structure.js
new file mode 100644 (file)
index 0000000..c4b88f9
--- /dev/null
@@ -0,0 +1,59 @@
+class Foo { }
+
+function Bar() { }
+
+var numberOfGetPrototypeOfCalls = 0;
+
+var doBadThings = function() { };
+
+Bar.prototype = new Proxy(
+    {},
+    {
+        getPrototypeOf()
+        {
+            numberOfGetPrototypeOfCalls++;
+            doBadThings();
+            return Foo.prototype;
+        }
+    });
+
+// Break some watchpoints.
+var o = {f:42};
+o.g = 43;
+
+function foo(o, p, q)
+{
+    var result = o.f;
+    var _ = p instanceof Foo;
+    q.f = 11;
+    return result + o.f;
+}
+
+noInline(foo);
+
+for (var i = 0; i < 10000; ++i) {
+    var result = foo({f:42}, new Bar(), {f:0});
+    if (result != 84)
+        throw "Error: bad result in loop: " + result;
+}
+
+if (numberOfGetPrototypeOfCalls != 10000)
+    throw "Error: did not call getPrototypeOf() the right number of times";
+
+var globalO = {f:42};
+var didCallGetter = false;
+doBadThings = function() {
+    delete globalO.f;
+    globalO.__defineGetter__("f", function() {
+        didCallGetter = true;
+        return 43;
+    });
+};
+
+var result = foo(globalO, new Bar(), {f:0});
+if (result != 85)
+    throw "Error: bad result at end: " + result;
+if (!didCallGetter)
+    throw "Error: did not call getter";
+if (numberOfGetPrototypeOfCalls != 10001)
+    throw "Error: did not call getPrototypeOf() the right number of times at end";
diff --git a/JSTests/stress/instanceof-proxy-loop.js b/JSTests/stress/instanceof-proxy-loop.js
new file mode 100644 (file)
index 0000000..1bff5ed
--- /dev/null
@@ -0,0 +1,59 @@
+class Foo { }
+
+function Bar() { }
+
+var numberOfGetPrototypeOfCalls = 0;
+
+var doBadThings = function() { };
+
+Bar.prototype = new Proxy(
+    {},
+    {
+        getPrototypeOf()
+        {
+            numberOfGetPrototypeOfCalls++;
+            doBadThings();
+            return Foo.prototype;
+        }
+    });
+
+// Break some watchpoints.
+var o = {f:42};
+o.g = 43;
+
+function foo(o, p)
+{
+    var result = o.f;
+    for (var i = 0; i < 5; ++i)
+        var _ = p instanceof Foo;
+    return result + o.f;
+}
+
+noInline(foo);
+
+for (var i = 0; i < 10000; ++i) {
+    var result = foo({f:42}, new Bar());
+    if (result != 84)
+        throw "Error: bad result in loop: " + result;
+}
+
+if (numberOfGetPrototypeOfCalls != 10000 * 5)
+    throw "Error: did not call getPrototypeOf() the right number of times";
+
+var globalO = {f:42};
+var didCallGetter = false;
+doBadThings = function() {
+    delete globalO.f;
+    globalO.__defineGetter__("f", function() {
+        didCallGetter = true;
+        return 43;
+    });
+};
+
+var result = foo(globalO, new Bar());
+if (result != 85)
+    throw "Error: bad result at end: " + result;
+if (!didCallGetter)
+    throw "Error: did not call getter";
+if (numberOfGetPrototypeOfCalls != 10001 * 5)
+    throw "Error: did not call getPrototypeOf() the right number of times at end";
diff --git a/JSTests/stress/instanceof-proxy.js b/JSTests/stress/instanceof-proxy.js
new file mode 100644 (file)
index 0000000..c1ad114
--- /dev/null
@@ -0,0 +1,47 @@
+class Foo { }
+
+function Bar() { }
+
+var numberOfGetPrototypeOfCalls = 0;
+
+var doBadThings = function() { };
+
+Bar.prototype = new Proxy(
+    {},
+    {
+        getPrototypeOf()
+        {
+            numberOfGetPrototypeOfCalls++;
+            doBadThings();
+            return Foo.prototype;
+        }
+    });
+
+var o = {f:42};
+
+function foo(o, p)
+{
+    var result = o.f;
+    var _ = p instanceof Foo;
+    return result + o.f;
+}
+
+noInline(foo);
+
+for (var i = 0; i < 10000; ++i) {
+    var result = foo({f:42}, new Bar());
+    if (result != 84)
+        throw "Error: bad result in loop: " + result;
+}
+
+var globalO = {f:42};
+var didCallGetter = false;
+doBadThings = function() {
+    globalO.f = 43;
+};
+
+var result = foo(globalO, new Bar());
+if (result != 85)
+    throw "Error: bad result at end: " + result;
+if (numberOfGetPrototypeOfCalls != 10001)
+    throw "Error: did not call getPrototypeOf() the right number of times at end";
index 6ae0b3e..f30732e 100644 (file)
@@ -1,3 +1,22 @@
+2018-05-16  Filip Pizlo  <fpizlo@apple.com>
+
+        DFG models InstanceOf incorrectly
+        https://bugs.webkit.org/show_bug.cgi?id=185694
+
+        Reviewed by Keith Miller.
+        
+        Proxies mean that InstanceOf can have effects. Exceptions mean that it's illegal to DCE it or
+        hoist it.
+
+        * dfg/DFGAbstractInterpreterInlines.h:
+        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+        * dfg/DFGClobberize.h:
+        (JSC::DFG::clobberize):
+        * dfg/DFGHeapLocation.cpp:
+        (WTF::printInternal):
+        * dfg/DFGHeapLocation.h:
+        * dfg/DFGNodeType.h:
+
 2018-05-16  Andy VanWagoner  <andy@vanwagoner.family>
 
         Add support for Intl NumberFormat formatToParts
index 5c1c5f2..87fd8ac 100644 (file)
@@ -3363,7 +3363,7 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
         break;
             
     case InstanceOf:
-        // Sadly, we don't propagate the fact that we've done InstanceOf
+        clobberWorld();
         setNonCellTypeForNode(node, SpecBoolean);
         break;
 
index 0ba017a..ad62364 100644 (file)
@@ -636,6 +636,7 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
     case ToNumber:
     case NumberToStringWithRadix:
     case CreateThis:
+    case InstanceOf:
         read(World);
         write(Heap);
         return;
@@ -1042,11 +1043,6 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
         def(HeapLocation(OverridesHasInstanceLoc, JSCell_typeInfoFlags, node->child1()), LazyNode(node));
         return;
 
-    case InstanceOf:
-        read(JSCell_structureID);
-        def(HeapLocation(InstanceOfLoc, JSCell_structureID, node->child1(), node->child2()), LazyNode(node));
-        return;
-
     case PutStructure:
         read(JSObject_butterfly);
         write(JSCell_structureID);
index be0dfad..016ad0e 100644 (file)
@@ -144,10 +144,6 @@ void printInternal(PrintStream& out, LocationKind kind)
         out.print("IndexedPropertyStorageLoc");
         return;
         
-    case InstanceOfLoc:
-        out.print("InstanceOfLoc");
-        return;
-        
     case NamedPropertyLoc:
         out.print("NamedPropertyLoc");
         return;
index 8973804..674251e 100644 (file)
@@ -52,7 +52,6 @@ enum LocationKind {
     IndexedPropertyInt52Loc,
     IndexedPropertyJSLoc,
     IndexedPropertyStorageLoc,
-    InstanceOfLoc,
     InvalidationPointLoc,
     IsFunctionLoc,
     IsObjectOrNullLoc,
index 48e0d99..feec25e 100644 (file)
@@ -344,7 +344,7 @@ namespace JSC { namespace DFG {
     \
     /* Nodes for misc operations. */\
     macro(OverridesHasInstance, NodeMustGenerate | NodeResultBoolean) \
-    macro(InstanceOf, NodeResultBoolean) \
+    macro(InstanceOf, NodeMustGenerate | NodeResultBoolean) \
     macro(InstanceOfCustom, NodeMustGenerate | NodeResultBoolean) \
     \
     macro(IsCellWithType, NodeResultBoolean) \