<!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>[195238] 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/195238">195238</a></dd>
<dt>Author</dt> <dd>sbarati@apple.com</dd>
<dt>Date</dt> <dd>2016-01-18 14:15:30 -0800 (Mon, 18 Jan 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>FTL doesn't do proper spilling for exception handling when GetById/Snippets go to slow path
https://bugs.webkit.org/show_bug.cgi?id=153186
Reviewed by Michael Saboff.
Michael was investigating a bug he found while doing the new JSC calling
convention work and it turns out to be a latent bug in FTL try/catch machinery.
After I looked at the code again, I realized that what I had previously
written is wrong in a subtle way. The FTL callOperation machinery will remove
its result register from the set of registers it needs to spill. This is not
correct when we have try/catch. We may want to do value recovery on
the value that the result register is prior to the call after the call
throws an exception. The case that we were solving before was when the
resultRegister == baseRegister in a GetById, or left/rightRegister == resultRegister in a Snippet.
This code is correct in wanting to spill in that case, even though it might spill
when we don't need it to (i.e the result is not needed for value recovery). Once I
investigated this bug further, I realized that the previous rule is just a
partial subset of the rule that says we should spill anytime the result is
a register we might do value recovery on. This patch implements the rule that
says we always want to spill the result when we will do value recovery on it
if an exception is thrown.
* ftl/FTLCompile.cpp:
(JSC::FTL::mmAllocateDataSection):
* tests/stress/ftl-try-catch-getter-throw-interesting-value-recovery.js: Added.
(assert):
(random):
(identity):
(let.o2.get f):
(let.o3.get f):
(foo):
(i.else):</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreftlFTLCompilecpp">trunk/Source/JavaScriptCore/ftl/FTLCompile.cpp</a></li>
</ul>
<h3>Added Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoretestsstressftltrycatchgetterthrowinterestingvaluerecoveryjs">trunk/Source/JavaScriptCore/tests/stress/ftl-try-catch-getter-throw-interesting-value-recovery.js</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (195237 => 195238)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2016-01-18 21:27:53 UTC (rev 195237)
+++ trunk/Source/JavaScriptCore/ChangeLog 2016-01-18 22:15:30 UTC (rev 195238)
</span><span class="lines">@@ -1,3 +1,38 @@
</span><ins>+2016-01-18 Saam barati <sbarati@apple.com>
+
+ FTL doesn't do proper spilling for exception handling when GetById/Snippets go to slow path
+ https://bugs.webkit.org/show_bug.cgi?id=153186
+
+ Reviewed by Michael Saboff.
+
+ Michael was investigating a bug he found while doing the new JSC calling
+ convention work and it turns out to be a latent bug in FTL try/catch machinery.
+ After I looked at the code again, I realized that what I had previously
+ written is wrong in a subtle way. The FTL callOperation machinery will remove
+ its result register from the set of registers it needs to spill. This is not
+ correct when we have try/catch. We may want to do value recovery on
+ the value that the result register is prior to the call after the call
+ throws an exception. The case that we were solving before was when the
+ resultRegister == baseRegister in a GetById, or left/rightRegister == resultRegister in a Snippet.
+ This code is correct in wanting to spill in that case, even though it might spill
+ when we don't need it to (i.e the result is not needed for value recovery). Once I
+ investigated this bug further, I realized that the previous rule is just a
+ partial subset of the rule that says we should spill anytime the result is
+ a register we might do value recovery on. This patch implements the rule that
+ says we always want to spill the result when we will do value recovery on it
+ if an exception is thrown.
+
+ * ftl/FTLCompile.cpp:
+ (JSC::FTL::mmAllocateDataSection):
+ * tests/stress/ftl-try-catch-getter-throw-interesting-value-recovery.js: Added.
+ (assert):
+ (random):
+ (identity):
+ (let.o2.get f):
+ (let.o3.get f):
+ (foo):
+ (i.else):
+
</ins><span class="cx"> 2016-01-18 Konstantin Tokarev <annulen@yandex.ru>
</span><span class="cx">
</span><span class="cx"> [MIPS] LLInt: fix calculation of Global Offset Table
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreftlFTLCompilecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ftl/FTLCompile.cpp (195237 => 195238)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ftl/FTLCompile.cpp 2016-01-18 21:27:53 UTC (rev 195237)
+++ trunk/Source/JavaScriptCore/ftl/FTLCompile.cpp 2016-01-18 22:15:30 UTC (rev 195238)
</span><span class="lines">@@ -555,16 +555,10 @@
</span><span class="cx"> // be preserved to the stack before the call, that way the OSR exit
</span><span class="cx"> // exception handler can recover them into the proper registers.
</span><span class="cx"> exit.gatherRegistersToSpillForCallIfException(stackmaps, record);
</span><del>- } else if (exitDescriptorImpl.m_exceptionType == ExceptionType::GetById) {
</del><ins>+ } else if (exitDescriptorImpl.m_exceptionType == ExceptionType::GetById || exitDescriptorImpl.m_exceptionType == ExceptionType::BinaryOpGenerator) {
</ins><span class="cx"> GPRReg result = record.locations[0].directGPR();
</span><del>- GPRReg base = record.locations[1].directGPR();
- if (base == result)
- callOperationExit->registersToPreserveForCallThatMightThrow.set(base);
- } else if (exitDescriptorImpl.m_exceptionType == ExceptionType::BinaryOpGenerator) {
- GPRReg result = record.locations[0].directGPR();
- GPRReg left = record.locations[1].directGPR();
- GPRReg right = record.locations[2].directGPR();
- if (result == left || result == right)
</del><ins>+ RegisterSet registersNeededForOSRExit = record.usedRegisterSet();
+ if (registersNeededForOSRExit.get(result))
</ins><span class="cx"> callOperationExit->registersToPreserveForCallThatMightThrow.set(result);
</span><span class="cx"> }
</span><span class="cx"> }
</span><span class="lines">@@ -664,19 +658,24 @@
</span><span class="cx">
</span><span class="cx"> bool addedUniqueExceptionJump = addNewExceptionJumpIfNecessary(iter->value[i].index);
</span><span class="cx"> MacroAssembler::Label begin = slowPathJIT.label();
</span><del>- if (result == base) {
- // This situation has a really interesting story. We may have a GetById inside
- // a try block where LLVM assigns the result and the base to the same register.
- // The inline cache may miss and we may end up at this slow path callOperation.
- // Then, suppose the base and the result are both the same register, so the return
- // value of the C call gets stored into the original base register. If the operationGetByIdOptimize
- // throws, it will return "undefined" and we will be stuck with "undefined" in the base
- // register that we would like to do value recovery on. We combat this situation from ever
- // taking place by ensuring we spill the original base value and then recover it from
- // the spill slot as the first step in OSR exit.
- if (OSRExit* exit = exceptionHandlerManager.callOperationOSRExit(iter->value[i].index))
- exit->spillRegistersToSpillSlot(slowPathJIT, osrExitFromGenericUnwindStackSpillSlot);
- }
</del><ins>+ // We take care to always spill the result whenever we need to do value recovery
+ // on it in the OSR exit. This is because the callOperation(.) machinery doesn't
+ // ever spill the result value. It actually takes care to never spill the result
+ // because it overwrites it with the result of the call. But, with exceptions and
+ // OSR exits, we may need the result value prior to the call during OSR exit.
+ // We take care to mark it for spillage now.
+ //
+ // This also handles another really interesting register preservation story:
+ // We may have a GetById/Snippet inside a try block where LLVM assigns the result
+ // and the base to the same register. The inline cache may miss and we may end up
+ // at this slow path callOperation. Then, because the base and the result are both
+ // the same register, the return value of the C call gets stored into the original
+ // base register. If the operationGetByIdOptimize throws, it will return "undefined"
+ // and we will be stuck with "undefined" in the base register that we would like to
+ // do value recovery on. We combat this situation from ever taking place by ensuring
+ // we spill the original base value (i.e the result register).
+ if (OSRExit* exit = exceptionHandlerManager.callOperationOSRExit(iter->value[i].index))
+ exit->spillRegistersToSpillSlot(slowPathJIT, osrExitFromGenericUnwindStackSpillSlot);
</ins><span class="cx"> MacroAssembler::Call call = callOperation(
</span><span class="cx"> state, usedRegisters, slowPathJIT, codeOrigin, addedUniqueExceptionJump ? &exceptionJumpsToLink.last().first : &exceptionTarget,
</span><span class="cx"> operationGetByIdOptimize, result, CCallHelpers::TrustedImmPtr(gen.stubInfo()),
</span><span class="lines">@@ -718,6 +717,9 @@
</span><span class="cx">
</span><span class="cx"> MacroAssembler::Label begin = slowPathJIT.label();
</span><span class="cx">
</span><ins>+ if (OSRExit* exit = exceptionHandlerManager.callOperationOSRExit(iter->value[i].index))
+ exit->spillRegistersToSpillSlot(slowPathJIT, osrExitFromGenericUnwindStackSpillSlot);
+
</ins><span class="cx"> MacroAssembler::Call call = callOperation(
</span><span class="cx"> state, usedRegisters, slowPathJIT, codeOrigin, addedUniqueExceptionJump ? &exceptionJumpsToLink.last().first : &exceptionTarget,
</span><span class="cx"> gen.slowPathFunction(), InvalidGPRReg,
</span><span class="lines">@@ -790,12 +792,10 @@
</span><span class="cx">
</span><span class="cx"> binaryOp.m_slowPathStarts.append(slowPathJIT.label());
</span><span class="cx"> bool addedUniqueExceptionJump = addNewExceptionJumpIfNecessary(iter->value[i].index);
</span><del>- if (result == left || result == right) {
- // This situation has a really interesting register preservation story.
- // See comment above for GetByIds.
- if (OSRExit* exit = exceptionHandlerManager.callOperationOSRExit(iter->value[i].index))
- exit->spillRegistersToSpillSlot(slowPathJIT, osrExitFromGenericUnwindStackSpillSlot);
- }
</del><ins>+ // This situation has a really interesting register preservation story.
+ // See comment above for GetByIds.
+ if (OSRExit* exit = exceptionHandlerManager.callOperationOSRExit(iter->value[i].index))
+ exit->spillRegistersToSpillSlot(slowPathJIT, osrExitFromGenericUnwindStackSpillSlot);
</ins><span class="cx">
</span><span class="cx"> callOperation(state, usedRegisters, slowPathJIT, codeOrigin, addedUniqueExceptionJump ? &exceptionJumpsToLink.last().first : &exceptionTarget,
</span><span class="cx"> binaryOp.slowPathFunction(), result, left, right).call();
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoretestsstressftltrycatchgetterthrowinterestingvaluerecoveryjs"></a>
<div class="addfile"><h4>Added: trunk/Source/JavaScriptCore/tests/stress/ftl-try-catch-getter-throw-interesting-value-recovery.js (0 => 195238)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/tests/stress/ftl-try-catch-getter-throw-interesting-value-recovery.js (rev 0)
+++ trunk/Source/JavaScriptCore/tests/stress/ftl-try-catch-getter-throw-interesting-value-recovery.js 2016-01-18 22:15:30 UTC (rev 195238)
</span><span class="lines">@@ -0,0 +1,65 @@
</span><ins>+function assert(b) {
+ if (!b)
+ throw new Error("bad value")
+}
+noInline(assert);
+
+function random() {
+ return "blah";
+}
+noInline(random);
+
+function identity(x) {
+ return x;
+}
+noInline(identity);
+
+let o1 = {
+ g: 20,
+ y: 40,
+ f: "get f"
+};
+
+let o2 = {
+ g: "g",
+ y: "y",
+ get f() {
+ return "get f";
+ }
+}
+
+let o4 = {};
+
+let o3 = {
+ get f() {
+ throw new Error("blah");
+ }
+}
+
+function foo(o, a) {
+ let x = o.g;
+ let y = o.y;
+ let oo = identity(o);
+ let j = random();
+ try {
+ j = o.f;
+ } catch(e) {
+ assert(j === "blah");
+ assert(oo === o3);
+ return x + y + 1;
+ }
+ return x + y;
+}
+
+noInline(foo);
+for (let i = 0; i < 100000; i++) {
+ if (i % 3 == 0) {
+ assert(foo(o1) === 60);
+ } else if (i % 3 === 1) {
+ assert(foo(o2) === "gy");
+ } else {
+ foo(o4);
+ }
+}
+
+foo(o3);
</ins></span></pre>
</div>
</div>
</body>
</html>