<!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>[204010] 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/204010">204010</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2016-08-01 22:38:58 -0700 (Mon, 01 Aug 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>REGRESSION (<a href="http://trac.webkit.org/projects/webkit/changeset/203990">r203990</a>): JSC Debug test stress/arity-check-ftl-throw.js failing
https://bugs.webkit.org/show_bug.cgi?id=160438
Reviewed by Mark Lam.
In <a href="http://trac.webkit.org/projects/webkit/changeset/203990">r203990</a> I fixed a bug where CommonSlowPaths.h/arityCheckFor() was basically failing at
catching stack overflow due to large parameter count. It would only catch regular old stack
overflow, like if the frame pointer was already past the limit.
This had a secondary problem: unfortunately all of our tests for what happens when you overflow
the stack due to large parameter count were not going down that path at all, so we haven't had
test coverage for this in ages. There were bugs in all tiers of the engine when handling this
case.
We need to be able to roll back the topCallFrame on paths that are meant to throw an exception
from the caller. Otherwise, we'd crash in StackVisitor because it would see a busted stack
frame. Rolling back like this "just works" except when the caller is the VM entry frame. I had
some choices here. I could have forced anyone who is rolling back to always skip VM entry
frames. They can't do it in a way that changes the value of VM::topVMEntryFrame, which is what
a stack frame roll back normally does, since exception unwinding needs to see the current value
of topVMEntryFrame. So, we have a choice to either try to magically avoid all of the paths that
look at topCallFrame, or give topCallFrame a state that unambiguously signals that we are
sitting right on top of a VM entry frame without having succeeded at making a JS call. The only
place that really needs to know is StackVisitor, which wants to start scanning at topCallFrame.
To signal this, I could have either made topCallFrame point to the real top JS call frame
without also rolling back topVMEntryFrame, or I could make topCallFrame == topVMEntryFrame. The
latter felt somehow cleaner. I filed a bug (https://bugs.webkit.org/show_bug.cgi?id=160441) for
converting topCallFrame to a void*, which would give us a chance to harden the rest of the
engine against this case.
* interpreter/StackVisitor.cpp:
(JSC::StackVisitor::StackVisitor):
We may do ShadowChicken processing, which invokes StackVisitor, when we have topCallFrame
pointing at topVMEntryFrame. This teaches StackVisitor how to handle this case. I believe that
StackVisitor is the only place that needs to be taught about this at this time, because it's
one of the few things that access topCallFrame along this special path.
* jit/JITOperations.cpp: Roll back the top call frame.
* runtime/CommonSlowPaths.cpp:
(JSC::SLOW_PATH_DECL): Roll back the top call frame.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreinterpreterStackVisitorcpp">trunk/Source/JavaScriptCore/interpreter/StackVisitor.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCorejitJITOperationscpp">trunk/Source/JavaScriptCore/jit/JITOperations.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreruntimeCommonSlowPathscpp">trunk/Source/JavaScriptCore/runtime/CommonSlowPaths.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCoreruntimeVMh">trunk/Source/JavaScriptCore/runtime/VM.h</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (204009 => 204010)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2016-08-02 05:02:27 UTC (rev 204009)
+++ trunk/Source/JavaScriptCore/ChangeLog 2016-08-02 05:38:58 UTC (rev 204010)
</span><span class="lines">@@ -1,3 +1,46 @@
</span><ins>+2016-08-01 Filip Pizlo <fpizlo@apple.com>
+
+ REGRESSION (r203990): JSC Debug test stress/arity-check-ftl-throw.js failing
+ https://bugs.webkit.org/show_bug.cgi?id=160438
+
+ Reviewed by Mark Lam.
+
+ In r203990 I fixed a bug where CommonSlowPaths.h/arityCheckFor() was basically failing at
+ catching stack overflow due to large parameter count. It would only catch regular old stack
+ overflow, like if the frame pointer was already past the limit.
+
+ This had a secondary problem: unfortunately all of our tests for what happens when you overflow
+ the stack due to large parameter count were not going down that path at all, so we haven't had
+ test coverage for this in ages. There were bugs in all tiers of the engine when handling this
+ case.
+
+ We need to be able to roll back the topCallFrame on paths that are meant to throw an exception
+ from the caller. Otherwise, we'd crash in StackVisitor because it would see a busted stack
+ frame. Rolling back like this "just works" except when the caller is the VM entry frame. I had
+ some choices here. I could have forced anyone who is rolling back to always skip VM entry
+ frames. They can't do it in a way that changes the value of VM::topVMEntryFrame, which is what
+ a stack frame roll back normally does, since exception unwinding needs to see the current value
+ of topVMEntryFrame. So, we have a choice to either try to magically avoid all of the paths that
+ look at topCallFrame, or give topCallFrame a state that unambiguously signals that we are
+ sitting right on top of a VM entry frame without having succeeded at making a JS call. The only
+ place that really needs to know is StackVisitor, which wants to start scanning at topCallFrame.
+ To signal this, I could have either made topCallFrame point to the real top JS call frame
+ without also rolling back topVMEntryFrame, or I could make topCallFrame == topVMEntryFrame. The
+ latter felt somehow cleaner. I filed a bug (https://bugs.webkit.org/show_bug.cgi?id=160441) for
+ converting topCallFrame to a void*, which would give us a chance to harden the rest of the
+ engine against this case.
+
+ * interpreter/StackVisitor.cpp:
+ (JSC::StackVisitor::StackVisitor):
+ We may do ShadowChicken processing, which invokes StackVisitor, when we have topCallFrame
+ pointing at topVMEntryFrame. This teaches StackVisitor how to handle this case. I believe that
+ StackVisitor is the only place that needs to be taught about this at this time, because it's
+ one of the few things that access topCallFrame along this special path.
+
+ * jit/JITOperations.cpp: Roll back the top call frame.
+ * runtime/CommonSlowPaths.cpp:
+ (JSC::SLOW_PATH_DECL): Roll back the top call frame.
+
</ins><span class="cx"> 2016-08-01 Benjamin Poulain <bpoulain@apple.com>
</span><span class="cx">
</span><span class="cx"> [JSC][ARM64] Fix branchTest32/64 taking an immediate as mask
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreinterpreterStackVisitorcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/interpreter/StackVisitor.cpp (204009 => 204010)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/interpreter/StackVisitor.cpp 2016-08-02 05:02:27 UTC (rev 204009)
+++ trunk/Source/JavaScriptCore/interpreter/StackVisitor.cpp 2016-08-02 05:38:58 UTC (rev 204010)
</span><span class="lines">@@ -43,6 +43,11 @@
</span><span class="cx"> if (startFrame) {
</span><span class="cx"> m_frame.m_VMEntryFrame = startFrame->vm().topVMEntryFrame;
</span><span class="cx"> topFrame = startFrame->vm().topCallFrame;
</span><ins>+
+ if (topFrame && static_cast<void*>(m_frame.m_VMEntryFrame) == static_cast<void*>(topFrame)) {
+ topFrame = vmEntryRecord(m_frame.m_VMEntryFrame)->m_prevTopCallFrame;
+ m_frame.m_VMEntryFrame = vmEntryRecord(m_frame.m_VMEntryFrame)->m_prevTopVMEntryFrame;
+ }
</ins><span class="cx"> } else {
</span><span class="cx"> m_frame.m_VMEntryFrame = 0;
</span><span class="cx"> topFrame = 0;
</span></span></pre></div>
<a id="trunkSourceJavaScriptCorejitJITOperationscpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/jit/JITOperations.cpp (204009 => 204010)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/jit/JITOperations.cpp 2016-08-02 05:02:27 UTC (rev 204009)
+++ trunk/Source/JavaScriptCore/jit/JITOperations.cpp 2016-08-02 05:38:58 UTC (rev 204010)
</span><span class="lines">@@ -2182,7 +2182,7 @@
</span><span class="cx">
</span><span class="cx"> void JIT_OPERATION lookupExceptionHandlerFromCallerFrame(VM* vm, ExecState* exec)
</span><span class="cx"> {
</span><del>- NativeCallFrameTracer tracer(vm, exec);
</del><ins>+ vm->topCallFrame = exec->callerFrame();
</ins><span class="cx"> genericUnwind(vm, exec, UnwindFromCallerFrame);
</span><span class="cx"> ASSERT(vm->targetMachinePCForThrow);
</span><span class="cx"> }
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreruntimeCommonSlowPathscpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/runtime/CommonSlowPaths.cpp (204009 => 204010)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/runtime/CommonSlowPaths.cpp 2016-08-02 05:02:27 UTC (rev 204009)
+++ trunk/Source/JavaScriptCore/runtime/CommonSlowPaths.cpp 2016-08-02 05:38:58 UTC (rev 204010)
</span><span class="lines">@@ -182,7 +182,8 @@
</span><span class="cx"> int slotsToAdd = CommonSlowPaths::arityCheckFor(exec, vm, CodeForCall);
</span><span class="cx"> if (slotsToAdd < 0) {
</span><span class="cx"> exec = exec->callerFrame();
</span><del>- ErrorHandlingScope errorScope(exec->vm());
</del><ins>+ vm.topCallFrame = exec;
+ ErrorHandlingScope errorScope(vm);
</ins><span class="cx"> CommonSlowPaths::interpreterThrowInCaller(exec, createStackOverflowError(exec));
</span><span class="cx"> RETURN_TWO(bitwise_cast<void*>(static_cast<uintptr_t>(1)), exec);
</span><span class="cx"> }
</span><span class="lines">@@ -195,7 +196,8 @@
</span><span class="cx"> int slotsToAdd = CommonSlowPaths::arityCheckFor(exec, vm, CodeForConstruct);
</span><span class="cx"> if (slotsToAdd < 0) {
</span><span class="cx"> exec = exec->callerFrame();
</span><del>- ErrorHandlingScope errorScope(exec->vm());
</del><ins>+ vm.topCallFrame = exec;
+ ErrorHandlingScope errorScope(vm);
</ins><span class="cx"> CommonSlowPaths::interpreterThrowInCaller(exec, createStackOverflowError(exec));
</span><span class="cx"> RETURN_TWO(bitwise_cast<void*>(static_cast<uintptr_t>(1)), exec);
</span><span class="cx"> }
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreruntimeVMh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/runtime/VM.h (204009 => 204010)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/runtime/VM.h 2016-08-02 05:02:27 UTC (rev 204009)
+++ trunk/Source/JavaScriptCore/runtime/VM.h 2016-08-02 05:38:58 UTC (rev 204010)
</span><span class="lines">@@ -270,7 +270,11 @@
</span><span class="cx"> VMType vmType;
</span><span class="cx"> ClientData* clientData;
</span><span class="cx"> VMEntryFrame* topVMEntryFrame;
</span><del>- ExecState* topCallFrame;
</del><ins>+ // NOTE: When throwing an exception while rolling back the call frame, this may be equal to
+ // topVMEntryFrame.
+ // FIXME: This should be a void*, because it might not point to a CallFrame.
+ // https://bugs.webkit.org/show_bug.cgi?id=160441
+ ExecState* topCallFrame;
</ins><span class="cx"> Strong<Structure> structureStructure;
</span><span class="cx"> Strong<Structure> structureRareDataStructure;
</span><span class="cx"> Strong<Structure> terminatedExecutionErrorStructure;
</span></span></pre>
</div>
</div>
</body>
</html>