AI folding of IsObjectOrNull is broken for non-object types that may be null
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 11 Jul 2015 03:01:20 +0000 (03:01 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 11 Jul 2015 03:01:20 +0000 (03:01 +0000)
https://bugs.webkit.org/show_bug.cgi?id=146867

Reviewed by Ryosuke Niwa.

* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects): Fix the bug and add some text describing what is going on.
* tests/stress/misc-is-object-or-null.js: Added. Test for the bug.
(foo):
* tests/stress/other-is-object-or-null.js: Added. Test for a bug I almost introduced.
(foo):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
Source/JavaScriptCore/tests/stress/misc-is-object-or-null.js [new file with mode: 0644]
Source/JavaScriptCore/tests/stress/other-is-object-or-null.js [new file with mode: 0644]

index cc3736f..d42e57c 100644 (file)
@@ -1,5 +1,19 @@
 2015-07-10  Filip Pizlo  <fpizlo@apple.com>
 
+        AI folding of IsObjectOrNull is broken for non-object types that may be null
+        https://bugs.webkit.org/show_bug.cgi?id=146867
+
+        Reviewed by Ryosuke Niwa.
+
+        * dfg/DFGAbstractInterpreterInlines.h:
+        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects): Fix the bug and add some text describing what is going on.
+        * tests/stress/misc-is-object-or-null.js: Added. Test for the bug.
+        (foo):
+        * tests/stress/other-is-object-or-null.js: Added. Test for a bug I almost introduced.
+        (foo):
+
+2015-07-10  Filip Pizlo  <fpizlo@apple.com>
+
         It should be easy to measure total compile times.
         https://bugs.webkit.org/show_bug.cgi?id=146857
 
index d5af927..21ee0e4 100644 (file)
@@ -961,6 +961,10 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
                 break;
         }
         
+        // FIXME: This code should really use AbstractValue::isType() and
+        // AbstractValue::couldBeType().
+        // https://bugs.webkit.org/show_bug.cgi?id=146870
+        
         bool constantWasSet = false;
         switch (node->op()) {
         case IsUndefined:
@@ -1034,13 +1038,28 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
             // FIXME: Use the masquerades-as-undefined watchpoint thingy.
             // https://bugs.webkit.org/show_bug.cgi?id=144456
             
+            // These expressions are complicated to parse. A helpful way to parse this is that
+            // "!(T & ~S)" means "T is a subset of S". Conversely, "!(T & S)" means "T is a
+            // disjoint set from S". Things like "T - S" means that, provided that S is a
+            // subset of T, it's the "set of all things in T but not in S". Things like "T | S"
+            // mean the "union of T and S".
+            
+            // Is the child's type an object that isn't an other-object (i.e. object that could
+            // have masquaredes-as-undefined traps) and isn't a function?  Then: we should fold
+            // this to true.
             if (!(child.m_type & ~(SpecObject - SpecObjectOther - SpecFunction))) {
                 setConstant(node, jsBoolean(true));
                 constantWasSet = true;
                 break;
             }
             
-            if (!(child.m_type & (SpecObject - SpecFunction))) {
+            // Is the child's type definitely not either of: an object that isn't a function,
+            // or either undefined or null?  Then: we should fold this to false.  This means
+            // for example that if it's any non-function object, including those that have
+            // masquerades-as-undefined traps, then we don't fold. It also means we won't fold
+            // if it's undefined-or-null, since the type bits don't distinguish between
+            // undefined (which should fold to false) and null (which should fold to true).
+            if (!(child.m_type & ((SpecObject - SpecFunction) | SpecOther))) {
                 setConstant(node, jsBoolean(false));
                 constantWasSet = true;
                 break;
diff --git a/Source/JavaScriptCore/tests/stress/misc-is-object-or-null.js b/Source/JavaScriptCore/tests/stress/misc-is-object-or-null.js
new file mode 100644 (file)
index 0000000..e8968e5
--- /dev/null
@@ -0,0 +1,13 @@
+function foo(p) {
+    var x = p ? null : false;
+    return (typeof x) == "object";
+}
+
+noInline(foo);
+
+for (var i = 0; i < 10000; ++i) {
+    var p = !!(i & 1);
+    var result = foo(p);
+    if (result !== p)
+        throw "Error: bad result for p = " + p + ": " + result;
+}
diff --git a/Source/JavaScriptCore/tests/stress/other-is-object-or-null.js b/Source/JavaScriptCore/tests/stress/other-is-object-or-null.js
new file mode 100644 (file)
index 0000000..81c0b0a
--- /dev/null
@@ -0,0 +1,13 @@
+function foo(p) {
+    var x = p ? null : void 0;
+    return (typeof x) == "object";
+}
+
+noInline(foo);
+
+for (var i = 0; i < 10000; ++i) {
+    var p = !!(i & 1);
+    var result = foo(p);
+    if (result !== p)
+        throw "Error: bad result for p = " + p + ": " + result;
+}