<!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>[199166] 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/199166">199166</a></dd>
<dt>Author</dt> <dd>fpizlo@apple.com</dd>
<dt>Date</dt> <dd>2016-04-07 11:38:15 -0700 (Thu, 07 Apr 2016)</dd>
</dl>
<h3>Log Message</h3>
<pre>Rationalize the makeSpaceForCCall stuff
https://bugs.webkit.org/show_bug.cgi?id=156352
Reviewed by Mark Lam.
I want to add more code to PolymorphicAccess that makes C calls, so that I can finally fix
https://bugs.webkit.org/show_bug.cgi?id=130914 (allow transition caches to handle indexing
headers).
When trying to understand what it takes to make a C call, I came across code that was making
room on the stack for spilled arguments. This logic was guarded with some complicated
condition. At first, I tried to just refactor the code so that the same ugly condition
wouldn't have to be copy-pasted everywhere that we made C calls. But then I started thinking
about the condition, and realized that it was probably wrong: if the outer PolymorphicAccess
harness decides to reuse a register for the scratchGPR then the top of the stack will store
the old value of scratchGPR, but the condition wouldn't necessarily trigger. So if the call
then overwrote something on the stack, we'd have a bad time.
Making room on the stack for a call is a cheap operation. It's orders of magnitude cheaper
than the rest of the call. Therefore, I think that it's best to just unconditionally make
room on the stack.
This patch makes us do just that. I also made the relevant helpers not inline, because I
think that we have too many inline methods in our assemblers. Now it's much easier to make
C calls from PolymorphicAccess because you just call the AssemblyHelper methods for making
space. There are no special conditions or anything like that.
* bytecode/PolymorphicAccess.cpp:
(JSC::AccessCase::generate):
* jit/AssemblyHelpers.cpp:
(JSC::AssemblyHelpers::emitLoadStructure):
(JSC::AssemblyHelpers::makeSpaceOnStackForCCall):
(JSC::AssemblyHelpers::reclaimSpaceOnStackForCCall):
(JSC::emitRandomThunkImpl):
* jit/AssemblyHelpers.h:
(JSC::AssemblyHelpers::makeSpaceOnStackForCCall): Deleted.
(JSC::AssemblyHelpers::reclaimSpaceOnStackForCCall): Deleted.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCorebytecodePolymorphicAccesscpp">trunk/Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCorejitAssemblyHelperscpp">trunk/Source/JavaScriptCore/jit/AssemblyHelpers.cpp</a></li>
<li><a href="#trunkSourceJavaScriptCorejitAssemblyHelpersh">trunk/Source/JavaScriptCore/jit/AssemblyHelpers.h</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (199165 => 199166)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2016-04-07 18:22:03 UTC (rev 199165)
+++ trunk/Source/JavaScriptCore/ChangeLog 2016-04-07 18:38:15 UTC (rev 199166)
</span><span class="lines">@@ -1,3 +1,43 @@
</span><ins>+2016-04-07 Filip Pizlo <fpizlo@apple.com>
+
+ Rationalize the makeSpaceForCCall stuff
+ https://bugs.webkit.org/show_bug.cgi?id=156352
+
+ Reviewed by Mark Lam.
+
+ I want to add more code to PolymorphicAccess that makes C calls, so that I can finally fix
+ https://bugs.webkit.org/show_bug.cgi?id=130914 (allow transition caches to handle indexing
+ headers).
+
+ When trying to understand what it takes to make a C call, I came across code that was making
+ room on the stack for spilled arguments. This logic was guarded with some complicated
+ condition. At first, I tried to just refactor the code so that the same ugly condition
+ wouldn't have to be copy-pasted everywhere that we made C calls. But then I started thinking
+ about the condition, and realized that it was probably wrong: if the outer PolymorphicAccess
+ harness decides to reuse a register for the scratchGPR then the top of the stack will store
+ the old value of scratchGPR, but the condition wouldn't necessarily trigger. So if the call
+ then overwrote something on the stack, we'd have a bad time.
+
+ Making room on the stack for a call is a cheap operation. It's orders of magnitude cheaper
+ than the rest of the call. Therefore, I think that it's best to just unconditionally make
+ room on the stack.
+
+ This patch makes us do just that. I also made the relevant helpers not inline, because I
+ think that we have too many inline methods in our assemblers. Now it's much easier to make
+ C calls from PolymorphicAccess because you just call the AssemblyHelper methods for making
+ space. There are no special conditions or anything like that.
+
+ * bytecode/PolymorphicAccess.cpp:
+ (JSC::AccessCase::generate):
+ * jit/AssemblyHelpers.cpp:
+ (JSC::AssemblyHelpers::emitLoadStructure):
+ (JSC::AssemblyHelpers::makeSpaceOnStackForCCall):
+ (JSC::AssemblyHelpers::reclaimSpaceOnStackForCCall):
+ (JSC::emitRandomThunkImpl):
+ * jit/AssemblyHelpers.h:
+ (JSC::AssemblyHelpers::makeSpaceOnStackForCCall): Deleted.
+ (JSC::AssemblyHelpers::reclaimSpaceOnStackForCCall): Deleted.
+
</ins><span class="cx"> 2016-04-07 Commit Queue <commit-queue@webkit.org>
</span><span class="cx">
</span><span class="cx"> Unreviewed, rolling out r199128 and r199141.
</span></span></pre></div>
<a id="trunkSourceJavaScriptCorebytecodePolymorphicAccesscpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp (199165 => 199166)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp 2016-04-07 18:22:03 UTC (rev 199165)
+++ trunk/Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp 2016-04-07 18:38:15 UTC (rev 199166)
</span><span class="lines">@@ -892,7 +892,7 @@
</span><span class="cx">
</span><span class="cx"> done.link(&jit);
</span><span class="cx">
</span><del>- jit.addPtr(CCallHelpers::TrustedImm32((jit.codeBlock()->stackPointerOffset() * sizeof(Register)) - state.preservedReusedRegisterState.numberOfBytesPreserved - state.numberOfStackBytesUsedForRegisterPreservation()),
</del><ins>+ jit.addPtr(CCallHelpers::TrustedImm32((codeBlock->stackPointerOffset() * sizeof(Register)) - state.preservedReusedRegisterState.numberOfBytesPreserved - state.numberOfStackBytesUsedForRegisterPreservation()),
</ins><span class="cx"> GPRInfo::callFrameRegister, CCallHelpers::stackPointerRegister);
</span><span class="cx"> state.restoreLiveRegistersFromStackForCall(isGetter());
</span><span class="cx">
</span><span class="lines">@@ -908,12 +908,10 @@
</span><span class="cx"> CodeLocationLabel(vm.getCTIStub(linkCallThunkGenerator).code()));
</span><span class="cx"> });
</span><span class="cx"> } else {
</span><del>- // Need to make room for the C call so any of our stack spillage isn't overwritten.
- // We also need to make room because we may be an inline cache in the FTL and not
- // have a JIT call frame.
- bool needsToMakeRoomOnStackForCCall = state.numberOfStackBytesUsedForRegisterPreservation() || codeBlock->jitType() == JITCode::FTLJIT;
- if (needsToMakeRoomOnStackForCCall)
- jit.makeSpaceOnStackForCCall();
</del><ins>+ // Need to make room for the C call so any of our stack spillage isn't overwritten. It's
+ // hard to track if someone did spillage or not, so we just assume that we always need
+ // to make some space here.
+ jit.makeSpaceOnStackForCCall();
</ins><span class="cx">
</span><span class="cx"> // getter: EncodedJSValue (*GetValueFunc)(ExecState*, EncodedJSValue thisValue, PropertyName);
</span><span class="cx"> // setter: void (*PutValueFunc)(ExecState*, EncodedJSValue thisObject, EncodedJSValue value);
</span><span class="lines">@@ -944,8 +942,7 @@
</span><span class="cx"> operationCall = jit.call();
</span><span class="cx"> if (m_type == CustomValueGetter || m_type == CustomAccessorGetter)
</span><span class="cx"> jit.setupResults(valueRegs);
</span><del>- if (needsToMakeRoomOnStackForCCall)
- jit.reclaimSpaceOnStackForCCall();
</del><ins>+ jit.reclaimSpaceOnStackForCCall();
</ins><span class="cx">
</span><span class="cx"> CCallHelpers::Jump noException =
</span><span class="cx"> jit.emitExceptionCheck(CCallHelpers::InvertedExceptionCheck);
</span></span></pre></div>
<a id="trunkSourceJavaScriptCorejitAssemblyHelperscpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/jit/AssemblyHelpers.cpp (199165 => 199166)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/jit/AssemblyHelpers.cpp 2016-04-07 18:22:03 UTC (rev 199165)
+++ trunk/Source/JavaScriptCore/jit/AssemblyHelpers.cpp 2016-04-07 18:38:15 UTC (rev 199166)
</span><span class="lines">@@ -1,5 +1,5 @@
</span><span class="cx"> /*
</span><del>- * Copyright (C) 2011, 2013-2015 Apple Inc. All rights reserved.
</del><ins>+ * Copyright (C) 2011, 2013-2016 Apple Inc. All rights reserved.
</ins><span class="cx"> *
</span><span class="cx"> * Redistribution and use in source and binary forms, with or without
</span><span class="cx"> * modification, are permitted provided that the following conditions
</span><span class="lines">@@ -455,6 +455,20 @@
</span><span class="cx"> #endif
</span><span class="cx"> }
</span><span class="cx">
</span><ins>+void AssemblyHelpers::makeSpaceOnStackForCCall()
+{
+ unsigned stackOffset = WTF::roundUpToMultipleOf(stackAlignmentBytes(), maxFrameExtentForSlowPathCall);
+ if (stackOffset)
+ subPtr(TrustedImm32(stackOffset), stackPointerRegister);
+}
+
+void AssemblyHelpers::reclaimSpaceOnStackForCCall()
+{
+ unsigned stackOffset = WTF::roundUpToMultipleOf(stackAlignmentBytes(), maxFrameExtentForSlowPathCall);
+ if (stackOffset)
+ addPtr(TrustedImm32(stackOffset), stackPointerRegister);
+}
+
</ins><span class="cx"> #if USE(JSVALUE64)
</span><span class="cx"> template<typename LoadFromHigh, typename StoreToHigh, typename LoadFromLow, typename StoreToLow>
</span><span class="cx"> void emitRandomThunkImpl(AssemblyHelpers& jit, GPRReg scratch0, GPRReg scratch1, GPRReg scratch2, FPRReg result, const LoadFromHigh& loadFromHigh, const StoreToHigh& storeToHigh, const LoadFromLow& loadFromLow, const StoreToLow& storeToLow)
</span></span></pre></div>
<a id="trunkSourceJavaScriptCorejitAssemblyHelpersh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/jit/AssemblyHelpers.h (199165 => 199166)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/jit/AssemblyHelpers.h 2016-04-07 18:22:03 UTC (rev 199165)
+++ trunk/Source/JavaScriptCore/jit/AssemblyHelpers.h 2016-04-07 18:38:15 UTC (rev 199166)
</span><span class="lines">@@ -1,5 +1,5 @@
</span><span class="cx"> /*
</span><del>- * Copyright (C) 2011, 2013-2015 Apple Inc. All rights reserved.
</del><ins>+ * Copyright (C) 2011, 2013-2016 Apple Inc. All rights reserved.
</ins><span class="cx"> *
</span><span class="cx"> * Redistribution and use in source and binary forms, with or without
</span><span class="cx"> * modification, are permitted provided that the following conditions
</span><span class="lines">@@ -1325,20 +1325,9 @@
</span><span class="cx">
</span><span class="cx"> Vector<BytecodeAndMachineOffset>& decodedCodeMapFor(CodeBlock*);
</span><span class="cx">
</span><del>- void makeSpaceOnStackForCCall()
- {
- unsigned stackOffset = WTF::roundUpToMultipleOf(stackAlignmentBytes(), maxFrameExtentForSlowPathCall);
- if (stackOffset)
- subPtr(TrustedImm32(stackOffset), stackPointerRegister);
- }
</del><ins>+ void makeSpaceOnStackForCCall();
+ void reclaimSpaceOnStackForCCall();
</ins><span class="cx">
</span><del>- void reclaimSpaceOnStackForCCall()
- {
- unsigned stackOffset = WTF::roundUpToMultipleOf(stackAlignmentBytes(), maxFrameExtentForSlowPathCall);
- if (stackOffset)
- addPtr(TrustedImm32(stackOffset), stackPointerRegister);
- }
-
</del><span class="cx"> #if USE(JSVALUE64)
</span><span class="cx"> void emitRandomThunk(JSGlobalObject*, GPRReg scratch0, GPRReg scratch1, GPRReg scratch2, FPRReg result);
</span><span class="cx"> void emitRandomThunk(GPRReg scratch0, GPRReg scratch1, GPRReg scratch2, GPRReg scratch3, FPRReg result);
</span></span></pre>
</div>
</div>
</body>
</html>