<!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>[171705] 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/171705">171705</a></dd>
<dt>Author</dt> <dd>benjamin@webkit.org</dd>
<dt>Date</dt> <dd>2014-07-28 15:29:37 -0700 (Mon, 28 Jul 2014)</dd>
</dl>
<h3>Log Message</h3>
<pre>[JSC] JIT::assertStackPointerOffset() crashes on ARM64
https://bugs.webkit.org/show_bug.cgi?id=135316
Patch by Benjamin Poulain <bpoulain@apple.com> on 2014-07-28
Reviewed by Geoffrey Garen.
JIT::assertStackPointerOffset() does a compare between an arbitrary register
and the stack pointer. This was not supported by the ARM64 assembler.
There are no variation that can take a stack pointer for Xd. There is one version of subs
that can take a stack pointer, but only for the Xn: the shift+extend one.
To solve the problem, I changed cmp to swap the registers if necessary, and I fixed
the implementation of sub.
* assembler/ARM64Assembler.h:
(JSC::ARM64Assembler::sub):
In the generic sub(reg, reg), I added assertions to catch the condition that cannot be generated
with either version of sub.
In sub(with shift), I remove the weird special case for SP. First, it was quite misleading because
the Rd case only works if "setflag == false". The other confusing part is going to addSubtractShiftedRegister()
gives you a reduce shift range, which could create subtle bug that only appear when SP is used.
Since I removed the weird case, I need to differentiate between the sub() that support SP, and the one that does
not elsewhere. That is why that branch has moved to the generic sub(reg, reg). Since at that point we know
the shift value must be zero, it is safe to call either variant.
* assembler/MacroAssemblerARM64.h:
(JSC::MacroAssemblerARM64::branch64):
With the changes described above, we can now use SP for the left register. What do we do if the rightmost
register is SP?
For the case of JIT::assertStackPointerOffset(), the comparison is Equal so the order really does not matter,
we just switch the registers before generating the instruction.
For the generic case, just move the value of SP to a GPR before doing the CMP.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreassemblerARM64Assemblerh">trunk/Source/JavaScriptCore/assembler/ARM64Assembler.h</a></li>
<li><a href="#trunkSourceJavaScriptCoreassemblerMacroAssemblerARM64h">trunk/Source/JavaScriptCore/assembler/MacroAssemblerARM64.h</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (171704 => 171705)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2014-07-28 22:26:52 UTC (rev 171704)
+++ trunk/Source/JavaScriptCore/ChangeLog 2014-07-28 22:29:37 UTC (rev 171705)
</span><span class="lines">@@ -1,3 +1,41 @@
</span><ins>+2014-07-28 Benjamin Poulain <bpoulain@apple.com>
+
+ [JSC] JIT::assertStackPointerOffset() crashes on ARM64
+ https://bugs.webkit.org/show_bug.cgi?id=135316
+
+ Reviewed by Geoffrey Garen.
+
+ JIT::assertStackPointerOffset() does a compare between an arbitrary register
+ and the stack pointer. This was not supported by the ARM64 assembler.
+
+ There are no variation that can take a stack pointer for Xd. There is one version of subs
+ that can take a stack pointer, but only for the Xn: the shift+extend one.
+ To solve the problem, I changed cmp to swap the registers if necessary, and I fixed
+ the implementation of sub.
+
+ * assembler/ARM64Assembler.h:
+ (JSC::ARM64Assembler::sub):
+ In the generic sub(reg, reg), I added assertions to catch the condition that cannot be generated
+ with either version of sub.
+
+ In sub(with shift), I remove the weird special case for SP. First, it was quite misleading because
+ the Rd case only works if "setflag == false". The other confusing part is going to addSubtractShiftedRegister()
+ gives you a reduce shift range, which could create subtle bug that only appear when SP is used.
+
+ Since I removed the weird case, I need to differentiate between the sub() that support SP, and the one that does
+ not elsewhere. That is why that branch has moved to the generic sub(reg, reg). Since at that point we know
+ the shift value must be zero, it is safe to call either variant.
+
+ * assembler/MacroAssemblerARM64.h:
+ (JSC::MacroAssemblerARM64::branch64):
+ With the changes described above, we can now use SP for the left register. What do we do if the rightmost
+ register is SP?
+
+ For the case of JIT::assertStackPointerOffset(), the comparison is Equal so the order really does not matter,
+ we just switch the registers before generating the instruction.
+
+ For the generic case, just move the value of SP to a GPR before doing the CMP.
+
</ins><span class="cx"> 2014-07-28 Brian J. Burg <burg@cs.washington.edu>
</span><span class="cx">
</span><span class="cx"> Unreviewed build fix after r171682.
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreassemblerARM64Assemblerh"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/assembler/ARM64Assembler.h (171704 => 171705)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/assembler/ARM64Assembler.h 2014-07-28 22:26:52 UTC (rev 171704)
+++ trunk/Source/JavaScriptCore/assembler/ARM64Assembler.h 2014-07-28 22:29:37 UTC (rev 171705)
</span><span class="lines">@@ -1958,7 +1958,13 @@
</span><span class="cx"> template<int datasize, SetFlags setFlags = DontSetFlags>
</span><span class="cx"> ALWAYS_INLINE void sub(RegisterID rd, RegisterID rn, RegisterID rm)
</span><span class="cx"> {
</span><del>- sub<datasize, setFlags>(rd, rn, rm, LSL, 0);
</del><ins>+ ASSERT_WITH_MESSAGE(!isSp(rd) || setFlags == DontSetFlags, "SUBS with shifted register does not support SP for Xd, it uses XZR for the register 31. SUBS with extended register support SP for Xd, but only if SetFlag is not used, otherwise register 31 is Xd.");
+ ASSERT_WITH_MESSAGE(!isSp(rm), "No encoding of SUBS supports SP for the third operand.");
+
+ if (isSp(rd) || isSp(rn))
+ sub<datasize, setFlags>(rd, rn, rm, UXTX, 0);
+ else
+ sub<datasize, setFlags>(rd, rn, rm, LSL, 0);
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> template<int datasize, SetFlags setFlags = DontSetFlags>
</span><span class="lines">@@ -1972,12 +1978,8 @@
</span><span class="cx"> ALWAYS_INLINE void sub(RegisterID rd, RegisterID rn, RegisterID rm, ShiftType shift, int amount)
</span><span class="cx"> {
</span><span class="cx"> CHECK_DATASIZE();
</span><del>- if (isSp(rd) || isSp(rn)) {
- ASSERT(shift == LSL);
- ASSERT(!isSp(rm));
- sub<datasize, setFlags>(rd, rn, rm, UXTX, amount);
- } else
- insn(addSubtractShiftedRegister(DATASIZE, AddOp_SUB, setFlags, shift, rm, amount, rn, rd));
</del><ins>+ ASSERT(!isSp(rd) && !isSp(rn) && !isSp(rm));
+ insn(addSubtractShiftedRegister(DATASIZE, AddOp_SUB, setFlags, shift, rm, amount, rn, rd));
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> template<int datasize>
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoreassemblerMacroAssemblerARM64h"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/assembler/MacroAssemblerARM64.h (171704 => 171705)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/assembler/MacroAssemblerARM64.h 2014-07-28 22:26:52 UTC (rev 171704)
+++ trunk/Source/JavaScriptCore/assembler/MacroAssemblerARM64.h 2014-07-28 22:29:37 UTC (rev 171705)
</span><span class="lines">@@ -1651,6 +1651,16 @@
</span><span class="cx">
</span><span class="cx"> Jump branch64(RelationalCondition cond, RegisterID left, RegisterID right)
</span><span class="cx"> {
</span><ins>+ if (right == ARM64Registers::sp) {
+ if (cond == Equal && left != ARM64Registers::sp) {
+ // CMP can only use SP for the left argument, since we are testing for equality, the order
+ // does not matter here.
+ std::swap(left, right);
+ } else {
+ move(right, getCachedDataTempRegisterIDAndInvalidate());
+ right = dataTempRegister;
+ }
+ }
</ins><span class="cx"> m_assembler.cmp<64>(left, right);
</span><span class="cx"> return Jump(makeBranch(cond));
</span><span class="cx"> }
</span></span></pre>
</div>
</div>
</body>
</html>