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 743bc050cb0806daac609818de19b7b5bc5323c3..5c67abd7d40e0554ed46c3fba9613a9a1dde8eb7 100644 (file)
@@ -1,3 +1,20 @@
+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
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 b61edb467a3b88ca254bfffcf109a5948359a988..70fab6023de6ba61cd2c95845bc4809f8127f852 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 b7e02cf9d4694c35206707b72f3c64025939f353..69cec67414058720929edc083bec1268dbd41e94 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))