Kill ArithNegate's ArithProfile assert inside BytecodeParser
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 19 Jan 2018 22:30:45 +0000 (22:30 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 19 Jan 2018 22:30:45 +0000 (22:30 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181877
<rdar://problem/36630552>

Reviewed by Mark Lam.

JSTests:

* stress/arith-profile-for-negate-can-see-non-number-due-to-dfg-osr-exit-profiling.js: Added.
(runNearStackLimit):
(f1):
(f2):
(f3):
(i.catch):
(i.try.runNearStackLimit):
(catch):

Source/JavaScriptCore:

Before this patch, we used to assert that op_negate's result ArithProfile
only produces number. It's logically true that negate only produces a number.
However, the DFG may incorrectly pick this ArithProfile when doing OSR exit
profiling. So we'll end up profiling something that's likely the input to
negate. This patch removes the assert. We cede to the fact that Graph::methodOfGettingAValueProfileFor
is entirely heuristic based, potentially leading to profiling results being imprecise.

* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::makeSafe):

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

JSTests/ChangeLog
JSTests/stress/arith-profile-for-negate-can-see-non-number-due-to-dfg-osr-exit-profiling.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

index 743bc05..5c67abd 100644 (file)
@@ -1,5 +1,22 @@
 2018-01-19  Saam Barati  <sbarati@apple.com>
 
+        Kill ArithNegate's ArithProfile assert inside BytecodeParser
+        https://bugs.webkit.org/show_bug.cgi?id=181877
+        <rdar://problem/36630552>
+
+        Reviewed by Mark Lam.
+
+        * stress/arith-profile-for-negate-can-see-non-number-due-to-dfg-osr-exit-profiling.js: Added.
+        (runNearStackLimit):
+        (f1):
+        (f2):
+        (f3):
+        (i.catch):
+        (i.try.runNearStackLimit):
+        (catch):
+
+2018-01-19  Saam Barati  <sbarati@apple.com>
+
         Spread's effects are modeled incorrectly both in AI and in Clobberize
         https://bugs.webkit.org/show_bug.cgi?id=181867
         <rdar://problem/36290415>
diff --git a/JSTests/stress/arith-profile-for-negate-can-see-non-number-due-to-dfg-osr-exit-profiling.js b/JSTests/stress/arith-profile-for-negate-can-see-non-number-due-to-dfg-osr-exit-profiling.js
new file mode 100644 (file)
index 0000000..5d01839
--- /dev/null
@@ -0,0 +1,32 @@
+function runNearStackLimit(f) {
+    try {
+        return t();
+    } catch (e) {
+        return f();
+    }
+}
+let flag = false;
+function f1() {
+    return flag ? {} : 10;
+}
+noInline(f1);
+
+function f2() {
+}
+
+function f3(arg) {
+    let r = -(arg ? f1() : f2());
+}
+
+for (let i = 0; i < 100000; ++i) {
+    try {
+        f3(!!(i % 2));
+    } catch (e) {}
+} 
+
+flag = true;
+for (let i = 0; i < 100000; ++i) try {
+    runNearStackLimit(() => {
+        return f3(!!(i % 2));
+    });
+} catch (e) {}
index b61edb4..70fab60 100644 (file)
@@ -1,3 +1,21 @@
+2018-01-19  Saam Barati  <sbarati@apple.com>
+
+        Kill ArithNegate's ArithProfile assert inside BytecodeParser
+        https://bugs.webkit.org/show_bug.cgi?id=181877
+        <rdar://problem/36630552>
+
+        Reviewed by Mark Lam.
+
+        Before this patch, we used to assert that op_negate's result ArithProfile
+        only produces number. It's logically true that negate only produces a number.
+        However, the DFG may incorrectly pick this ArithProfile when doing OSR exit
+        profiling. So we'll end up profiling something that's likely the input to
+        negate. This patch removes the assert. We cede to the fact that Graph::methodOfGettingAValueProfileFor
+        is entirely heuristic based, potentially leading to profiling results being imprecise.
+
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::makeSafe):
+
 2018-01-19  David Kilzer  <ddkilzer@apple.com>
 
         oss-fuzz jsc build is broken: StringImpl.h:27:10: fatal error: 'unicode/ustring.h' file not found
index b7e02cf..69cec67 100644 (file)
@@ -940,8 +940,10 @@ private:
                     break;
                 }
                 case ArithNegate: {
-                    ASSERT_WITH_MESSAGE(!arithProfile->didObserveNonNumber(), "op_negate starts with a toNumber() on the argument, it should only produce numbers.");
-
+                    // We'd like to assert here that the arith profile for the result of negate never
+                    // sees a non-number, but we can't. It's true that negate never produces a non-number.
+                    // But sometimes we'll end up grabbing the wrong ArithProfile during OSR exit, and
+                    // profiling the wrong value, leading the ArithProfile to think it observed a non-number result.
                     if (arithProfile->lhsObservedType().sawNumber() || arithProfile->didObserveDouble())
                         node->mergeFlags(NodeMayHaveDoubleResult);
                     if (arithProfile->didObserveNegZeroDouble() || m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, NegativeZero))