<!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>[244058] trunk</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/244058">244058</a></dd>
<dt>Author</dt> <dd>ysuzuki@apple.com</dd>
<dt>Date</dt> <dd>2019-04-08 17:00:24 -0700 (Mon, 08 Apr 2019)</dd>
</dl>

<h3>Log Message</h3>
<pre>[JSC] isRope jump in StringSlice should not jump over register allocations
https://bugs.webkit.org/show_bug.cgi?id=196716

Reviewed by Saam Barati.

JSTests:

* stress/is-rope-check-in-string-slice-should-not-jump-over-register-allocations.js: Added.
(foo.bar):
(foo):

Source/JavaScriptCore:

Jumping over the register allocation code in DFG (like the following) is wrong.

    auto jump = m_jit.branchXXX();
    {
        GPRTemporary reg(this);
        GPRReg regGPR = reg.gpr();
        ...
    }
    jump.link(&m_jit);

When GPRTemporary::gpr allocates a new register, it can flush the previous register value into the stack and make the register usable.
Jumping over this register allocation code skips the flushing code, and makes the DFG's stack and register content tracking inconsistent:
DFG thinks that the content is flushed and stored in particular stack slot even while this flushing code is skipped.
In this patch, we perform register allocations before jumping to the slow path based on `isRope` condition in StringSlice.

* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileStringSlice):</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkJSTestsChangeLog">trunk/JSTests/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoredfgDFGSpeculativeJITcpp">trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp</a></li>
</ul>

<h3>Added Paths</h3>
<ul>
<li><a href="#trunkJSTestsstressisropecheckinstringsliceshouldnotjumpoverregisterallocationsjs">trunk/JSTests/stress/is-rope-check-in-string-slice-should-not-jump-over-register-allocations.js</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkJSTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/JSTests/ChangeLog (244057 => 244058)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/ChangeLog  2019-04-08 23:33:05 UTC (rev 244057)
+++ trunk/JSTests/ChangeLog     2019-04-09 00:00:24 UTC (rev 244058)
</span><span class="lines">@@ -1,5 +1,16 @@
</span><span class="cx"> 2019-04-08  Yusuke Suzuki  <ysuzuki@apple.com>
</span><span class="cx"> 
</span><ins>+        [JSC] isRope jump in StringSlice should not jump over register allocations
+        https://bugs.webkit.org/show_bug.cgi?id=196716
+
+        Reviewed by Saam Barati.
+
+        * stress/is-rope-check-in-string-slice-should-not-jump-over-register-allocations.js: Added.
+        (foo.bar):
+        (foo):
+
+2019-04-08  Yusuke Suzuki  <ysuzuki@apple.com>
+
</ins><span class="cx">         [JSC] to_index_string should not assume incoming value is Uint32
</span><span class="cx">         https://bugs.webkit.org/show_bug.cgi?id=196713
</span><span class="cx"> 
</span></span></pre></div>
<a id="trunkJSTestsstressisropecheckinstringsliceshouldnotjumpoverregisterallocationsjs"></a>
<div class="addfile"><h4>Added: trunk/JSTests/stress/is-rope-check-in-string-slice-should-not-jump-over-register-allocations.js (0 => 244058)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/stress/is-rope-check-in-string-slice-should-not-jump-over-register-allocations.js                          (rev 0)
+++ trunk/JSTests/stress/is-rope-check-in-string-slice-should-not-jump-over-register-allocations.js     2019-04-09 00:00:24 UTC (rev 244058)
</span><span class="lines">@@ -0,0 +1,17 @@
</span><ins>+//@ runDefault("--jitPolicyScale=0", "--useFTLJIT=0", "--useConcurrentJIT=0", "--collectContinuously=1")
+
+function foo() {
+    for (let i = 0; i < 400; i++) {
+        const s = 'a'+'a';
+        function bar() {
+            s.slice(0);
+        }
+        for (const _ in {}) {
+        }
+        const o = {
+        };
+        bar([], [], [], [], {});
+        o ** '';
+    }
+}
+foo();
</ins></span></pre></div>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (244057 => 244058)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog    2019-04-08 23:33:05 UTC (rev 244057)
+++ trunk/Source/JavaScriptCore/ChangeLog       2019-04-09 00:00:24 UTC (rev 244058)
</span><span class="lines">@@ -1,5 +1,30 @@
</span><span class="cx"> 2019-04-08  Yusuke Suzuki  <ysuzuki@apple.com>
</span><span class="cx"> 
</span><ins>+        [JSC] isRope jump in StringSlice should not jump over register allocations
+        https://bugs.webkit.org/show_bug.cgi?id=196716
+
+        Reviewed by Saam Barati.
+
+        Jumping over the register allocation code in DFG (like the following) is wrong.
+
+            auto jump = m_jit.branchXXX();
+            {
+                GPRTemporary reg(this);
+                GPRReg regGPR = reg.gpr();
+                ...
+            }
+            jump.link(&m_jit);
+
+        When GPRTemporary::gpr allocates a new register, it can flush the previous register value into the stack and make the register usable.
+        Jumping over this register allocation code skips the flushing code, and makes the DFG's stack and register content tracking inconsistent:
+        DFG thinks that the content is flushed and stored in particular stack slot even while this flushing code is skipped.
+        In this patch, we perform register allocations before jumping to the slow path based on `isRope` condition in StringSlice.
+
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileStringSlice):
+
+2019-04-08  Yusuke Suzuki  <ysuzuki@apple.com>
+
</ins><span class="cx">         [JSC] to_index_string should not assume incoming value is Uint32
</span><span class="cx">         https://bugs.webkit.org/show_bug.cgi?id=196713
</span><span class="cx"> 
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoredfgDFGSpeculativeJITcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp (244057 => 244058)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp    2019-04-08 23:33:05 UTC (rev 244057)
+++ trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp       2019-04-09 00:00:24 UTC (rev 244058)
</span><span class="lines">@@ -1547,16 +1547,15 @@
</span><span class="cx">     }
</span><span class="cx"> 
</span><span class="cx">     GPRTemporary temp(this);
</span><del>-    GPRReg tempGPR = temp.gpr();
-
-    m_jit.loadPtr(CCallHelpers::Address(stringGPR, JSString::offsetOfValue()), tempGPR);
-    auto isRope = m_jit.branchIfRopeStringImpl(tempGPR);
-
</del><span class="cx">     GPRTemporary temp2(this);
</span><span class="cx">     GPRTemporary startIndex(this);
</span><span class="cx"> 
</span><ins>+    GPRReg tempGPR = temp.gpr();
</ins><span class="cx">     GPRReg temp2GPR = temp2.gpr();
</span><span class="cx">     GPRReg startIndexGPR = startIndex.gpr();
</span><ins>+
+    m_jit.loadPtr(CCallHelpers::Address(stringGPR, JSString::offsetOfValue()), tempGPR);
+    auto isRope = m_jit.branchIfRopeStringImpl(tempGPR);
</ins><span class="cx">     {
</span><span class="cx">         m_jit.load32(MacroAssembler::Address(tempGPR, StringImpl::lengthMemoryOffset()), temp2GPR);
</span><span class="cx"> 
</span></span></pre>
</div>
</div>

</body>
</html>