<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[182857] trunk/Source/JavaScriptCore</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/182857">182857</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2015-04-15 13:32:20 -0700 (Wed, 15 Apr 2015)</dd>
</dl>
<h3>Log Message</h3>
<pre>Harden DFGForAllKills
https://bugs.webkit.org/show_bug.cgi?id=143792
Reviewed by Geoffrey Garen.
Unfortunately, we don't have a good way to test this yet - but it will be needed to prevent
bugs in https://bugs.webkit.org/show_bug.cgi?id=143734.
Previously ForAllKills used the bytecode kill analysis. That seemed like a good idea because
that analysis is cheaper than the full liveness analysis. Unfortunately, it's probably wrong:
- It looks for kill sites at forExit origin boundaries. But, something might have been killed
by an operation that was logically in between the forExit origins at the boundary, but was
removed from the DFG for whatever reason. The DFG is allowed to have bytecode instruction
gaps.
- It overlooked the fact that a MovHint that addresses a local that is always live kills that
local. For example, storing to an argument means that the prior value of the argument is
killed.
This fixes the analysis by making it handle MovHints directly, and making it define kills in
the most conservative way possible: it asks if you were live before but dead after. If we
have the compile time budget to afford this more direct approach, then it's definitel a good
idea since it's so fool-proof.
* dfg/DFGArgumentsEliminationPhase.cpp:
* dfg/DFGForAllKills.h:
(JSC::DFG::forAllKilledOperands):
(JSC::DFG::forAllKilledNodesAtNodeIndex):
(JSC::DFG::forAllDirectlyKilledOperands): Deleted.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGArgumentsEliminationPhasecpp">trunk/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGForAllKillsh">trunk/Source/JavaScriptCore/dfg/DFGForAllKills.h</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (182856 => 182857)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2015-04-15 19:54:21 UTC (rev 182856)
+++ trunk/Source/JavaScriptCore/ChangeLog 2015-04-15 20:32:20 UTC (rev 182857)
</span><span class="lines">@@ -1,3 +1,36 @@
</span><ins>+2015-04-15 Filip Pizlo <fpizlo@apple.com>
+
+ Harden DFGForAllKills
+ https://bugs.webkit.org/show_bug.cgi?id=143792
+
+ Reviewed by Geoffrey Garen.
+
+ Unfortunately, we don't have a good way to test this yet - but it will be needed to prevent
+ bugs in https://bugs.webkit.org/show_bug.cgi?id=143734.
+
+ Previously ForAllKills used the bytecode kill analysis. That seemed like a good idea because
+ that analysis is cheaper than the full liveness analysis. Unfortunately, it's probably wrong:
+
+ - It looks for kill sites at forExit origin boundaries. But, something might have been killed
+ by an operation that was logically in between the forExit origins at the boundary, but was
+ removed from the DFG for whatever reason. The DFG is allowed to have bytecode instruction
+ gaps.
+
+ - It overlooked the fact that a MovHint that addresses a local that is always live kills that
+ local. For example, storing to an argument means that the prior value of the argument is
+ killed.
+
+ This fixes the analysis by making it handle MovHints directly, and making it define kills in
+ the most conservative way possible: it asks if you were live before but dead after. If we
+ have the compile time budget to afford this more direct approach, then it's definitel a good
+ idea since it's so fool-proof.
+
+ * dfg/DFGArgumentsEliminationPhase.cpp:
+ * dfg/DFGForAllKills.h:
+ (JSC::DFG::forAllKilledOperands):
+ (JSC::DFG::forAllKilledNodesAtNodeIndex):
+ (JSC::DFG::forAllDirectlyKilledOperands): Deleted.
+
</ins><span class="cx"> 2015-04-15 Joseph Pecoraro <pecoraro@apple.com>
</span><span class="cx">
</span><span class="cx"> Provide SPI to allow changing whether JSContexts are remote debuggable by default
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGArgumentsEliminationPhasecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp (182856 => 182857)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp 2015-04-15 19:54:21 UTC (rev 182856)
+++ trunk/Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp 2015-04-15 20:32:20 UTC (rev 182857)
</span><span class="lines">@@ -293,6 +293,7 @@
</span><span class="cx"> return;
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+ // This loop considers all nodes up to the nodeIndex, excluding the nodeIndex.
</ins><span class="cx"> while (nodeIndex--) {
</span><span class="cx"> Node* node = block->at(nodeIndex);
</span><span class="cx"> if (node == candidate)
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGForAllKillsh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGForAllKills.h (182856 => 182857)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGForAllKills.h 2015-04-15 19:54:21 UTC (rev 182856)
+++ trunk/Source/JavaScriptCore/dfg/DFGForAllKills.h 2015-04-15 20:32:20 UTC (rev 182857)
</span><span class="lines">@@ -72,18 +72,9 @@
</span><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> template<typename Functor>
</span><del>-void forAllDirectlyKilledOperands(Graph& graph, CodeOrigin origin, const Functor& functor)
</del><ins>+void forAllKilledOperands(Graph& graph, CodeOrigin before, Node* nodeAfter, const Functor& functor)
</ins><span class="cx"> {
</span><del>- graph.killsFor(origin.inlineCallFrame).forEachOperandKilledAt(origin.bytecodeIndex, functor);
-}
-
-template<typename Functor>
-void forAllKilledOperands(Graph& graph, CodeOrigin before, CodeOrigin after, const Functor& functor)
-{
- // The philosophy here is that if we're on the boundary between exiting to origin A and exiting
- // to origin B, then we note the kills for A. Kills for A are the bytecode kills plus the things
- // that were live at whatever callsites are popped between A and B. It's legal to conservatively
- // just consider everything live at A.
</del><ins>+ CodeOrigin after = nodeAfter->origin.forExit;
</ins><span class="cx">
</span><span class="cx"> if (!before) {
</span><span class="cx"> if (!after)
</span><span class="lines">@@ -99,43 +90,30 @@
</span><span class="cx"> return;
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+ // If we MovHint something that is live at the time, then we kill the old value.
+ VirtualRegister alreadyNoted;
+ if (nodeAfter->containsMovHint()) {
+ VirtualRegister reg = nodeAfter->unlinkedLocal();
+ if (graph.isLiveInBytecode(reg, after)) {
+ functor(reg);
+ alreadyNoted = reg;
+ }
+ }
+
</ins><span class="cx"> if (before == after)
</span><span class="cx"> return;
</span><span class="cx">
</span><span class="cx"> // before could be unset even if after is, but the opposite cannot happen.
</span><span class="cx"> ASSERT(!!after);
</span><span class="cx">
</span><del>- if (before.inlineCallFrame != after.inlineCallFrame) {
- // Is before deeper than after?
- bool beforeIsDeeper;
- if (!after.inlineCallFrame)
- beforeIsDeeper = true;
- else {
- beforeIsDeeper = false;
- for (InlineCallFrame* current = before.inlineCallFrame; current; current = current->caller.inlineCallFrame) {
- if (current == after.inlineCallFrame) {
- beforeIsDeeper = true;
- break;
- }
- }
- }
- if (beforeIsDeeper) {
- ASSERT(before.inlineCallFrame);
- for (CodeOrigin current = before; current.inlineCallFrame != after.inlineCallFrame; current = current.inlineCallFrame->caller) {
- ASSERT(current.inlineCallFrame);
- CodeBlock* codeBlock = graph.baselineCodeBlockFor(current.inlineCallFrame);
- FullBytecodeLiveness& liveness = graph.livenessFor(codeBlock);
- for (unsigned i = codeBlock->m_numCalleeRegisters; i--;) {
- VirtualRegister reg = virtualRegisterForLocal(i);
- if (liveness.operandIsLive(reg.offset(), current.bytecodeIndex))
- functor(reg + current.inlineCallFrame->stackOffset);
- }
- forAllDirectlyKilledOperands(graph, current.inlineCallFrame->caller, functor);
- }
- }
</del><ins>+ // Detect kills the super conservative way: it is killed if it was live before and dead after.
+ for (unsigned i = graph.block(0)->variablesAtHead.numberOfLocals(); i--;) {
+ VirtualRegister reg = virtualRegisterForLocal(i);
+ if (reg == alreadyNoted)
+ continue;
+ if (graph.isLiveInBytecode(reg, before) && !graph.isLiveInBytecode(reg, after))
+ functor(reg);
</ins><span class="cx"> }
</span><del>-
- forAllDirectlyKilledOperands(graph, before, functor);
</del><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> // Tells you all of the nodes that would no longer be live across the node at this nodeIndex.
</span><span class="lines">@@ -167,7 +145,7 @@
</span><span class="cx"> before = block->at(nodeIndex - 1)->origin.forExit;
</span><span class="cx">
</span><span class="cx"> forAllKilledOperands(
</span><del>- graph, before, node->origin.forExit,
</del><ins>+ graph, before, node,
</ins><span class="cx"> [&] (VirtualRegister reg) {
</span><span class="cx"> availabilityMap.closeStartingWithLocal(
</span><span class="cx"> reg,
</span></span></pre>
</div>
</div>
</body>
</html>