Generators violate bytecode liveness validation
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 30 Jun 2016 18:13:26 +0000 (18:13 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 30 Jun 2016 18:13:26 +0000 (18:13 +0000)
https://bugs.webkit.org/show_bug.cgi?id=159279

Reviewed by Yusuke Suzuki.
PerformanceTests:

Add Basic to our test harness.

Also made some cosmetic changes to the benchmark harness.

* ES6SampleBench/Basic/basic-tests.yaml: Added.
* ES6SampleBench/Basic/stress-test.js: Added.
(preciseTime):
* ES6SampleBench/driver.js:
(Driver):
(Driver.prototype.start):
(Driver.prototype.reportError):
* ES6SampleBench/glue.js:
* ES6SampleBench/index.html:

Source/JavaScriptCore:

Fix a liveness bug found by Basic. The problem is that resume's intended liveness rule is:
"live-in is just the token argument", but the liveness analysis thought that the rule was
"live-in is live-out minus defs plus live-at-catch". Clearly these two rules are quite
different. The way this sort of worked before is that we would define the defs of resume
as being equal to our prediction of what the live-outs would be. We did this in the hope
that we would subtract all live-outs. But, this misses the live-at-catch part. So, this
change adds another hack to neutralize live-at-catch.

This would make a lot more sense if we wrote a new liveness analysis that was just for
generator conversion. It could reuse BytecodeUseDef but otherwise it would be a new thing.
It would be easy to write crazy rules for save/resume in such an analysis, especially if
that analysis rewrote the bytecode. We could then just have an op_yield that is a no-op.
We would just record the live-outs of op_yield and use that for rewriting the code in terms
of a switch statement.

* bytecode/BytecodeLivenessAnalysis.cpp:
(JSC::stepOverInstruction):
(JSC::BytecodeLivenessAnalysis::dumpResults):
* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::dumpBytecode):

Tools:

Add Basic to our test harness.

* Scripts/run-javascriptcore-tests:
(runJSCStressTests):

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

12 files changed:
PerformanceTests/ChangeLog
PerformanceTests/ES6SampleBench/Basic/basic-tests.yaml [new file with mode: 0644]
PerformanceTests/ES6SampleBench/Basic/stress-test.js [new file with mode: 0644]
PerformanceTests/ES6SampleBench/driver.js
PerformanceTests/ES6SampleBench/glue.js
PerformanceTests/ES6SampleBench/index.html
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.cpp
Source/JavaScriptCore/bytecode/BytecodeUseDef.h
Source/JavaScriptCore/bytecode/CodeBlock.cpp
Tools/ChangeLog
Tools/Scripts/run-javascriptcore-tests

index be2df6e..091c66b 100644 (file)
@@ -1,3 +1,24 @@
+2016-06-29  Filip Pizlo  <fpizlo@apple.com>
+
+        Generators violate bytecode liveness validation
+        https://bugs.webkit.org/show_bug.cgi?id=159279
+
+        Reviewed by Yusuke Suzuki.
+
+        Add Basic to our test harness.
+
+        Also made some cosmetic changes to the benchmark harness.
+
+        * ES6SampleBench/Basic/basic-tests.yaml: Added.
+        * ES6SampleBench/Basic/stress-test.js: Added.
+        (preciseTime):
+        * ES6SampleBench/driver.js:
+        (Driver):
+        (Driver.prototype.start):
+        (Driver.prototype.reportError):
+        * ES6SampleBench/glue.js:
+        * ES6SampleBench/index.html:
+
 2016-06-28  Filip Pizlo  <fpizlo@apple.com>
 
         ES6SampleBench should have a harness
diff --git a/PerformanceTests/ES6SampleBench/Basic/basic-tests.yaml b/PerformanceTests/ES6SampleBench/Basic/basic-tests.yaml
new file mode 100644 (file)
index 0000000..6d55fbe
--- /dev/null
@@ -0,0 +1,28 @@
+# Copyright (C) 2016 Apple Inc. All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions
+# are met:
+#
+# 1.  Redistributions of source code must retain the above copyright
+#     notice, this list of conditions and the following disclaimer. 
+# 2.  Redistributions in binary form must reproduce the above copyright
+#     notice, this list of conditions and the following disclaimer in the
+#     documentation and/or other materials provided with the distribution. 
+#
+# THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
+# EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+# WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+# DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
+# DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+# (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+# LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
+# THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+- path: .
+  tests:
+    - stress-test.js
+  cmd: defaultRunNoisyTest unless parseRunCommands
+
diff --git a/PerformanceTests/ES6SampleBench/Basic/stress-test.js b/PerformanceTests/ES6SampleBench/Basic/stress-test.js
new file mode 100644 (file)
index 0000000..75a5b42
--- /dev/null
@@ -0,0 +1,54 @@
+/*
+ * Copyright (C) 2016 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE INC. OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
+ */
+"use strict";
+
+load("ast.js");
+load("basic.js");
+load("caseless_map.js");
+load("lexer.js");
+load("number.js");
+load("parser.js");
+load("random.js");
+load("state.js");
+load("util.js");
+load("benchmark.js");
+
+let benchmark = new Benchmark();
+let before = preciseTime();
+
+// Run for at least 10 iterations.
+for (let i = 0; i < 10; ++i) {
+    print("Running mandatory iteration #" + (i + 1) + ":");
+    benchmark.runIteration();
+}
+
+// Run until we have been running for two seconds.
+while (preciseTime() < before + 2) {
+    print("Running bonus iteration:");
+    benchmark.runIteration();
+}
+
+print("Success!");
+
index 4edcd2f..2152eca 100644 (file)
 "use strict";
 
 class Driver {
-    constructor(triggerCell, magicCell, summaryCell, key)
+    constructor(triggerCell, triggerLink, magicCell, summaryCell, key)
     {
-        if (!magicCell)
-            throw new Error("Need magic cell");
-        if (!summaryCell)
-            throw new Error("Need summary cell");
-        
         this._benchmarks = new Map();
         this._triggerCell = triggerCell;
+        this._triggerLink = triggerLink;
         this._magicCell = magicCell;
         this._summary = new Stats(summaryCell);
         this._key = key;
+        this._hadErrors = false;
         window[key] = this;
     }
     
@@ -47,7 +44,6 @@ class Driver {
     
     start(numIterations)
     {
-        this._triggerCellSaved = this._triggerCell.innerHTML;
         this._updateIterations();
         
         this._summary.reset();
@@ -70,6 +66,7 @@ class Driver {
     {
         this._benchmarks.get(this._benchmark).reportError(...args);
         this._recomputeSummary();
+        this._hadErrors = true;
         this._iterate();
     }
     
@@ -120,7 +117,9 @@ class Driver {
         this._benchmark = this._iterator ? this._iterator.next().value : null;
         if (!this._benchmark) {
             if (!this._numIterations) {
-                this._triggerCell.innerHTML = this._triggerCellSaved;
+                this._triggerCell.innerHTML =
+                    (this._hadErrors ? "Failures encountered!" : "Success!") +
+                    ` <a href="${this._triggerLink}">Restart Benchmark</a>`;
                 return;
             }
             this._numIterations--;
index aa2264d..a4e0b42 100644 (file)
@@ -26,6 +26,7 @@
 
 const driver = new Driver(
     document.getElementById("trigger"),
+    "driver.start(10)",
     document.getElementById("magic"),
     document.getElementById("Geomean"),
     "sampleBench");
index fe515ac..cf1f7db 100644 (file)
@@ -3,6 +3,12 @@
 <title>ES6 Sample Bench</title>
 <link rel="stylesheet" type="text/css" href="style.css">
 </head>
+<script>
+window.onerror = function(message, url, lineNumber)
+{
+    document.getElementById("trigger").innerHTML = "ERROR: " + url + ":" + lineNumber + ": " + message;
+}
+</script>
 <script src="driver.js"></script>
 <script src="results.js"></script>
 <script src="stats.js"></script>
index 0db30fa..0d177b0 100644 (file)
@@ -1,3 +1,31 @@
+2016-06-29  Filip Pizlo  <fpizlo@apple.com>
+
+        Generators violate bytecode liveness validation
+        https://bugs.webkit.org/show_bug.cgi?id=159279
+
+        Reviewed by Yusuke Suzuki.
+        
+        Fix a liveness bug found by Basic. The problem is that resume's intended liveness rule is:
+        "live-in is just the token argument", but the liveness analysis thought that the rule was
+        "live-in is live-out minus defs plus live-at-catch". Clearly these two rules are quite
+        different. The way this sort of worked before is that we would define the defs of resume
+        as being equal to our prediction of what the live-outs would be. We did this in the hope
+        that we would subtract all live-outs. But, this misses the live-at-catch part. So, this
+        change adds another hack to neutralize live-at-catch.
+        
+        This would make a lot more sense if we wrote a new liveness analysis that was just for
+        generator conversion. It could reuse BytecodeUseDef but otherwise it would be a new thing.
+        It would be easy to write crazy rules for save/resume in such an analysis, especially if
+        that analysis rewrote the bytecode. We could then just have an op_yield that is a no-op.
+        We would just record the live-outs of op_yield and use that for rewriting the code in terms
+        of a switch statement.
+
+        * bytecode/BytecodeLivenessAnalysis.cpp:
+        (JSC::stepOverInstruction):
+        (JSC::BytecodeLivenessAnalysis::dumpResults):
+        * bytecode/CodeBlock.cpp:
+        (JSC::CodeBlock::dumpBytecode):
+
 2016-06-30  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r202659.
index 7228b03..17e82a2 100644 (file)
@@ -132,9 +132,17 @@ static void stepOverInstruction(CodeBlock* codeBlock, BytecodeBasicBlock* block,
     // If we have an exception handler, we want the live-in variables of the 
     // exception handler block to be included in the live-in of this particular bytecode.
     if (HandlerInfo* handler = codeBlock->handlerForBytecodeOffset(bytecodeOffset)) {
-        BytecodeBasicBlock* handlerBlock = findBasicBlockWithLeaderOffset(basicBlocks, handler->target);
-        ASSERT(handlerBlock);
-        handlerBlock->in().forEachSetBit(use);
+        // FIXME: This resume check should not be needed.
+        // https://bugs.webkit.org/show_bug.cgi?id=159281
+        Interpreter* interpreter = codeBlock->vm()->interpreter;
+        Instruction* instructionsBegin = codeBlock->instructions().begin();
+        Instruction* instruction = &instructionsBegin[bytecodeOffset];
+        OpcodeID opcodeID = interpreter->getOpcodeID(instruction->u.opcode);
+        if (opcodeID != op_resume) {
+            BytecodeBasicBlock* handlerBlock = findBasicBlockWithLeaderOffset(basicBlocks, handler->target);
+            ASSERT(handlerBlock);
+            handlerBlock->in().forEachSetBit(use);
+        }
     }
 }
 
@@ -289,6 +297,7 @@ void BytecodeLivenessAnalysis::computeKills(BytecodeKills& result)
 
 void BytecodeLivenessAnalysis::dumpResults()
 {
+    dataLog("\nDumping bytecode liveness for ", *m_codeBlock, ":\n");
     Interpreter* interpreter = m_codeBlock->vm()->interpreter;
     Instruction* instructionsBegin = m_codeBlock->instructions().begin();
     for (unsigned i = 0; i < m_basicBlocks.size(); i++) {
index 64ea57b..981fb1d 100644 (file)
@@ -478,6 +478,8 @@ void computeDefsForBytecodeOffset(CodeBlock* codeBlock, BytecodeBasicBlock* bloc
     }
     case op_resume: {
         RELEASE_ASSERT(block->successors().size() == 1);
+        // FIXME: This is really dirty.
+        // https://bugs.webkit.org/show_bug.cgi?id=159281
         block->successors()[0]->in().forEachSetBit([&](unsigned local) {
             functor(codeBlock, instruction, opcodeID, virtualRegisterForLocal(local).offset());
         });
index bb7287d..27f8345 100644 (file)
@@ -1666,7 +1666,9 @@ void CodeBlock::dumpBytecode(
             int generator = (++it)->u.operand;
             unsigned liveCalleeLocalsIndex = (++it)->u.unsignedValue;
             int offset = (++it)->u.operand;
-            const FastBitVector& liveness = m_rareData->m_liveCalleeLocalsAtYield[liveCalleeLocalsIndex];
+            FastBitVector liveness;
+            if (liveCalleeLocalsIndex < m_rareData->m_liveCalleeLocalsAtYield.size())
+                liveness = m_rareData->m_liveCalleeLocalsAtYield[liveCalleeLocalsIndex];
             printLocationAndOp(out, exec, location, it, "save");
             out.printf("%s, ", registerName(generator).data());
             liveness.dump(out);
@@ -1676,7 +1678,9 @@ void CodeBlock::dumpBytecode(
         case op_resume: {
             int generator = (++it)->u.operand;
             unsigned liveCalleeLocalsIndex = (++it)->u.unsignedValue;
-            const FastBitVector& liveness = m_rareData->m_liveCalleeLocalsAtYield[liveCalleeLocalsIndex];
+            FastBitVector liveness;
+            if (liveCalleeLocalsIndex < m_rareData->m_liveCalleeLocalsAtYield.size())
+                liveness = m_rareData->m_liveCalleeLocalsAtYield[liveCalleeLocalsIndex];
             printLocationAndOp(out, exec, location, it, "resume");
             out.printf("%s, ", registerName(generator).data());
             liveness.dump(out);
index c422ff6..2b83369 100644 (file)
@@ -1,3 +1,15 @@
+2016-06-29  Filip Pizlo  <fpizlo@apple.com>
+
+        Generators violate bytecode liveness validation
+        https://bugs.webkit.org/show_bug.cgi?id=159279
+
+        Reviewed by Yusuke Suzuki.
+        
+        Add Basic to our test harness.
+
+        * Scripts/run-javascriptcore-tests:
+        (runJSCStressTests):
+
 2016-06-30  Per Arne Vollan  <pvollan@apple.com>
 
         [Win][Debug] Assertion fails in TestWTF.
index 9a6e9cc..b7a6091 100755 (executable)
@@ -269,6 +269,7 @@ sub runJSCStressTests
             "PerformanceTests/SunSpider/tests/sunspider-1.0",
             "PerformanceTests/JetStream/cdjs/cdjs-tests.yaml",
             "PerformanceTests/ES6SampleBench/Air/airjs-tests.yaml",
+            "PerformanceTests/ES6SampleBench/Basic/basic-tests.yaml",
             "Source/JavaScriptCore/tests/executableAllocationFuzz.yaml",
             "Source/JavaScriptCore/tests/exceptionFuzz.yaml",
             "PerformanceTests/SunSpider/no-architecture-specific-optimizations.yaml",