AbstractValue can represent more than int52
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 20 Apr 2019 00:37:43 +0000 (00:37 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 20 Apr 2019 00:37:43 +0000 (00:37 +0000)
https://bugs.webkit.org/show_bug.cgi?id=197118
<rdar://problem/49969960>

Reviewed by Michael Saboff.

JSTests:

* stress/abstract-value-can-include-int52.js: Added.
(foo):
(index.index.8.index.60.index.65.index.1234.index.1234.parseInt.string_appeared_here.String.fromCharCode):

Source/JavaScriptCore:

Let's analyze this control flow diamond:

#0
branch #1, #2

#1:
PutStack(JSValue, loc42)
Jump #3

#2:
PutStack(Int52, loc42)
Jump #3

#3:
...

Our abstract value for loc42 at the head of #3 will contain an abstract
value that us the union of Int52 with other things. Obviously in the
above program, a GetStack for loc42 would be inavlid, since it might
be loading either JSValue or Int52. However, the abstract interpreter
just tracks what the value could be, and it could be Int52 or JSValue.

When I did the Int52 refactoring, I expected such things to never happen,
but it turns out it does. We should just allow for this instead of asserting
against it since it's valid IR to do the above.

* bytecode/SpeculatedType.cpp:
(JSC::dumpSpeculation):
* dfg/DFGAbstractValue.cpp:
(JSC::DFG::AbstractValue::checkConsistency const):
* dfg/DFGAbstractValue.h:
(JSC::DFG::AbstractValue::validateTypeAcceptingBoxedInt52 const):

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

JSTests/ChangeLog
JSTests/stress/abstract-value-can-include-int52.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/SpeculatedType.cpp
Source/JavaScriptCore/dfg/DFGAbstractValue.cpp
Source/JavaScriptCore/dfg/DFGAbstractValue.h

index 5b16741..c3da161 100644 (file)
@@ -1,3 +1,15 @@
+2019-04-19  Saam Barati  <sbarati@apple.com>
+
+        AbstractValue can represent more than int52
+        https://bugs.webkit.org/show_bug.cgi?id=197118
+        <rdar://problem/49969960>
+
+        Reviewed by Michael Saboff.
+
+        * stress/abstract-value-can-include-int52.js: Added.
+        (foo):
+        (index.index.8.index.60.index.65.index.1234.index.1234.parseInt.string_appeared_here.String.fromCharCode):
+
 2019-04-18  Yusuke Suzuki  <ysuzuki@apple.com>
 
         [WTF] StringBuilder should set correct m_is8Bit flag when merging
diff --git a/JSTests/stress/abstract-value-can-include-int52.js b/JSTests/stress/abstract-value-can-include-int52.js
new file mode 100644 (file)
index 0000000..9451c82
--- /dev/null
@@ -0,0 +1,27 @@
+//@ runDefault("--useRandomizingFuzzerAgent=1", "--jitPolicyScale=0", "--useConcurrentJIT=0", "--useConcurrentGC=0")
+
+function foo(n) {
+    while (n) {
+        n >>>= 1;
+    }
+    return ''[0];
+}
+var indexP;
+var indexO = 0;
+for (var index = 0; index <= 100; index++) {
+    if (index < 8 || index > 60 && index <= 65 || index > 1234 && index < 1234) {
+        let x = foo(index);
+        if (parseInt('1Z' + String.fromCharCode(index), 36) !== 71) {
+            if (indexO === 0) {
+                indexO = 0;
+            } else {
+                if (index - indexP) {
+                    var hexP = foo(indexP);
+                    index - index
+                    index = index;
+                }
+            }
+            indexP = index;
+        }
+    }
+}
index b986148..3edb1d2 100644 (file)
@@ -1,3 +1,44 @@
+2019-04-19  Saam Barati  <sbarati@apple.com>
+
+        AbstractValue can represent more than int52
+        https://bugs.webkit.org/show_bug.cgi?id=197118
+        <rdar://problem/49969960>
+
+        Reviewed by Michael Saboff.
+
+        Let's analyze this control flow diamond:
+        
+        #0
+        branch #1, #2
+        
+        #1:
+        PutStack(JSValue, loc42)
+        Jump #3
+        
+        #2:
+        PutStack(Int52, loc42)
+        Jump #3
+        
+        #3:
+        ...
+        
+        Our abstract value for loc42 at the head of #3 will contain an abstract
+        value that us the union of Int52 with other things. Obviously in the
+        above program, a GetStack for loc42 would be inavlid, since it might
+        be loading either JSValue or Int52. However, the abstract interpreter
+        just tracks what the value could be, and it could be Int52 or JSValue.
+        
+        When I did the Int52 refactoring, I expected such things to never happen,
+        but it turns out it does. We should just allow for this instead of asserting
+        against it since it's valid IR to do the above.
+
+        * bytecode/SpeculatedType.cpp:
+        (JSC::dumpSpeculation):
+        * dfg/DFGAbstractValue.cpp:
+        (JSC::DFG::AbstractValue::checkConsistency const):
+        * dfg/DFGAbstractValue.h:
+        (JSC::DFG::AbstractValue::validateTypeAcceptingBoxedInt52 const):
+
 2019-04-19  Tadeu Zagallo  <tzagallo@apple.com>
 
         Add option to dump JIT memory
index 57e2cd0..24e54ef 100644 (file)
@@ -248,13 +248,7 @@ void dumpSpeculation(PrintStream& outStream, SpeculatedType value)
         else
             isTop = false;
     }
-    
-    if (value & SpecInt32AsInt52)
-        strOut.print("Int32AsInt52");
 
-    if (value & SpecNonInt32AsInt52)
-        strOut.print("NonInt32AsInt52");
-        
     if ((value & SpecBytecodeDouble) == SpecBytecodeDouble)
         strOut.print("BytecodeDouble");
     else {
@@ -287,13 +281,31 @@ void dumpSpeculation(PrintStream& outStream, SpeculatedType value)
     else
         isTop = false;
     
-    if (isTop)
+    if (value & SpecEmpty)
+        strOut.print("Empty");
+    else
+        isTop = false;
+
+    if (value & SpecInt52Any) {
+        if ((value & SpecInt52Any) == SpecInt52Any)
+            strOut.print("Int52Any");
+        else if (value & SpecInt32AsInt52)
+            strOut.print("Int32AsInt52");
+        else if (value & SpecNonInt32AsInt52)
+            strOut.print("NonInt32AsInt52");
+    } else
+        isTop = false;
+    
+    if (value == SpecBytecodeTop)
+        out.print("BytecodeTop");
+    else if (value == SpecHeapTop)
+        out.print("HeapTop");
+    else if (value == SpecFullTop)
+        out.print("FullTop");
+    else if (isTop)
         out.print("Top");
     else
         out.print(strStream.toCString());
-    
-    if (value & SpecEmpty)
-        out.print("Empty");
 }
 
 // We don't expose this because we don't want anyone relying on the fact that this method currently
index a9c137f..f9dc4f3 100644 (file)
@@ -452,11 +452,6 @@ void AbstractValue::checkConsistency() const
     if (isClear())
         RELEASE_ASSERT(!m_value);
     
-    if (m_type & SpecInt52Any) {
-        if (m_type != SpecFullTop)
-            RELEASE_ASSERT(isAnyInt52Speculation(m_type));
-    }
-
     if (!!m_value)
         RELEASE_ASSERT(validateTypeAcceptingBoxedInt52(m_value));
 
index 3ee7289..2ef3112 100644 (file)
@@ -526,11 +526,8 @@ private:
             return true;
         
         if (m_type & SpecInt52Any) {
-            ASSERT(!(m_type & ~SpecInt52Any));
-
-            if (mergeSpeculations(m_type, int52AwareSpeculationFromValue(value)) != m_type)
-                return false;
-            return true;
+            if (mergeSpeculations(m_type, int52AwareSpeculationFromValue(value)) == m_type)
+                return true;
         }
 
         if (mergeSpeculations(m_type, speculationFromValue(value)) != m_type)