<!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>[279082] 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/279082">279082</a></dd>
<dt>Author</dt> <dd>commit-queue@webkit.org</dd>
<dt>Date</dt> <dd>2021-06-21 14:06:49 -0700 (Mon, 21 Jun 2021)</dd>
</dl>

<h3>Log Message</h3>
<pre>[JSC] Fix consistency check during stack splitting in Wasm::LLIntGenerator::addLoop
https://bugs.webkit.org/show_bug.cgi?id=226012

Patch by Xan Lopez <xan@igalia.com> on 2021-06-21
Reviewed by Tadeu Zagallo.

JSTests:

* stress/wasm-loop-consistency.js: Added.
(vm.isWasmSupported):

Source/JavaScriptCore:

It is possible for the wasm llint generator to call
checkConsistency() on a stack that is only halfway through being
properly setup. Specifically, when generating a loop block, we use
splitStack() to pop the arguments for the loop into a new stack,
and materializeConstantsAndLocals() to materialize the constants
and aliases in the loop arguments, but the arguments won't be
added back to the stack until the very end of the loop code
generation. Since materializeConstantsAndLocals() will check the
correctness of the expression stack, which isn't yet fully formed,
we'll fail its ASSERT. To workaround this, we create a variant of
materializeConstantsAndLocals() that does not check for
correctness (similar to what we do in push()), and manually check
the correctness of the new split stack in
Wasm::LLIntGenerator::addLoop(), which is the place that knows the
details of this intermediate state.

For more details, see: https://bugs.webkit.org/show_bug.cgi?id=226012#c8

* wasm/WasmLLIntGenerator.cpp:
(JSC::Wasm::LLIntGenerator::checkConsistencyOfExpressionStack):
(JSC::Wasm::LLIntGenerator::checkConsistency):
(JSC::Wasm::LLIntGenerator::materializeConstantsAndLocals):
(JSC::Wasm::LLIntGenerator::addLoop):</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="#trunkSourceJavaScriptCorewasmWasmLLIntGeneratorcpp">trunk/Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp</a></li>
</ul>

<h3>Added Paths</h3>
<ul>
<li><a href="#trunkJSTestsstresswasmloopconsistencyjs">trunk/JSTests/stress/wasm-loop-consistency.js</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkJSTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/JSTests/ChangeLog (279081 => 279082)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/ChangeLog  2021-06-21 20:55:58 UTC (rev 279081)
+++ trunk/JSTests/ChangeLog     2021-06-21 21:06:49 UTC (rev 279082)
</span><span class="lines">@@ -1,3 +1,13 @@
</span><ins>+2021-06-21  Xan Lopez  <xan@igalia.com>
+
+        [JSC] Fix consistency check during stack splitting in Wasm::LLIntGenerator::addLoop
+        https://bugs.webkit.org/show_bug.cgi?id=226012
+
+        Reviewed by Tadeu Zagallo.
+
+        * stress/wasm-loop-consistency.js: Added.
+        (vm.isWasmSupported):
+
</ins><span class="cx"> 2021-06-21  Yusuke Suzuki  <ysuzuki@apple.com>
</span><span class="cx"> 
</span><span class="cx">         Release assert memory in JSC::Wasm::Memory::growShared(JSC::Wasm::PageCount)::<lambda()>
</span></span></pre></div>
<a id="trunkJSTestsstresswasmloopconsistencyjs"></a>
<div class="addfile"><h4>Added: trunk/JSTests/stress/wasm-loop-consistency.js (0 => 279082)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/JSTests/stress/wasm-loop-consistency.js                            (rev 0)
+++ trunk/JSTests/stress/wasm-loop-consistency.js       2021-06-21 21:06:49 UTC (rev 279082)
</span><span class="lines">@@ -0,0 +1,14 @@
</span><ins>+// https://bugs.webkit.org/show_bug.cgi?id=226012
+if ($vm.isWasmSupported()) {
+    // (module
+    // (type (;0;) (func (param i32)))
+    // (func (;0;) (type 0) (param i32)
+    //   local.get 0
+    //   local.get 0
+    //   loop (param i32)  ;; label = @1
+    //     drop
+    //   end
+    //   drop))
+    var wasmCode = new WebAssembly.Module(new Uint8Array([0, 97, 115, 109, 1, 0, 0, 0, 1, 5, 1, 96, 1, 127, 0, 3, 2, 1, 0, 10, 13, 1, 11, 0, 0x20, 0, 0x20, 0, 0x3, 0, 0x1A, 0xb, 0x1A, 0xb]));
+}
+
</ins></span></pre></div>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (279081 => 279082)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog    2021-06-21 20:55:58 UTC (rev 279081)
+++ trunk/Source/JavaScriptCore/ChangeLog       2021-06-21 21:06:49 UTC (rev 279082)
</span><span class="lines">@@ -1,3 +1,34 @@
</span><ins>+2021-06-21  Xan Lopez  <xan@igalia.com>
+
+        [JSC] Fix consistency check during stack splitting in Wasm::LLIntGenerator::addLoop
+        https://bugs.webkit.org/show_bug.cgi?id=226012
+
+        Reviewed by Tadeu Zagallo.
+
+        It is possible for the wasm llint generator to call
+        checkConsistency() on a stack that is only halfway through being
+        properly setup. Specifically, when generating a loop block, we use
+        splitStack() to pop the arguments for the loop into a new stack,
+        and materializeConstantsAndLocals() to materialize the constants
+        and aliases in the loop arguments, but the arguments won't be
+        added back to the stack until the very end of the loop code
+        generation. Since materializeConstantsAndLocals() will check the
+        correctness of the expression stack, which isn't yet fully formed,
+        we'll fail its ASSERT. To workaround this, we create a variant of
+        materializeConstantsAndLocals() that does not check for
+        correctness (similar to what we do in push()), and manually check
+        the correctness of the new split stack in
+        Wasm::LLIntGenerator::addLoop(), which is the place that knows the
+        details of this intermediate state.
+
+        For more details, see: https://bugs.webkit.org/show_bug.cgi?id=226012#c8
+
+        * wasm/WasmLLIntGenerator.cpp:
+        (JSC::Wasm::LLIntGenerator::checkConsistencyOfExpressionStack):
+        (JSC::Wasm::LLIntGenerator::checkConsistency):
+        (JSC::Wasm::LLIntGenerator::materializeConstantsAndLocals):
+        (JSC::Wasm::LLIntGenerator::addLoop):
+
</ins><span class="cx"> 2021-06-21  Yusuke Suzuki  <ysuzuki@apple.com>
</span><span class="cx"> 
</span><span class="cx">         Release assert memory in JSC::Wasm::Memory::growShared(JSC::Wasm::PageCount)::<lambda()>
</span></span></pre></div>
<a id="trunkSourceJavaScriptCorewasmWasmLLIntGeneratorcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp (279081 => 279082)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp  2021-06-21 20:55:58 UTC (rev 279081)
+++ trunk/Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp     2021-06-21 21:06:49 UTC (rev 279082)
</span><span class="lines">@@ -379,12 +379,8 @@
</span><span class="cx"> #endif // ASSERT_ENABLED
</span><span class="cx">     }
</span><span class="cx"> 
</span><del>-    void materializeConstantsAndLocals(Stack& expressionStack)
</del><ins>+    void materializeConstantsAndLocals(Stack& expressionStack, NoConsistencyCheckTag)
</ins><span class="cx">     {
</span><del>-        if (expressionStack.isEmpty())
-            return;
-
-        checkConsistency();
</del><span class="cx">         walkExpressionStack(expressionStack, [&](TypedExpression& expression, VirtualRegister slot) {
</span><span class="cx">             ASSERT(expression.value() == slot || expression.value().isConstant() || expression.value().isArgument() || static_cast<unsigned>(expression.value().toLocal()) < m_codeBlock->m_numVars);
</span><span class="cx">             if (expression.value() == slot)
</span><span class="lines">@@ -392,7 +388,16 @@
</span><span class="cx">             WasmMov::emit(this, slot, expression);
</span><span class="cx">             expression = TypedExpression { expression.type(), slot };
</span><span class="cx">         });
</span><ins>+    }
+
+    void materializeConstantsAndLocals(Stack& expressionStack)
+    {
+        if (expressionStack.isEmpty())
+            return;
+
</ins><span class="cx">         checkConsistency();
</span><ins>+        materializeConstantsAndLocals(expressionStack, NoConsistencyCheck);
+        checkConsistency();
</ins><span class="cx">     }
</span><span class="cx"> 
</span><span class="cx">     void splitStack(BlockSignature signature, Stack& enclosingStack, Stack& newStack)
</span><span class="lines">@@ -871,7 +876,20 @@
</span><span class="cx"> auto LLIntGenerator::addLoop(BlockSignature signature, Stack& enclosingStack, ControlType& block, Stack& newStack, uint32_t loopIndex) -> PartialResult
</span><span class="cx"> {
</span><span class="cx">     splitStack(signature, enclosingStack, newStack);
</span><del>-    materializeConstantsAndLocals(newStack);
</del><ins>+    materializeConstantsAndLocals(newStack, NoConsistencyCheck);
+#if ASSERT_ENABLED
+    // We cannot yet call checkConsistency, since the arguments we are
+    // materializing for the loop are not neither in the expression
+    // nor the control stack, and it won't know what to do in this
+    // intermediate state. As a sanity check just verify that
+    // everything in newStack is a virtual register that is actually
+    // pointing to each stack position, which is what we should have
+    // after we split the stack and the previous call materializes
+    // constants and aliases if needed.
+    walkExpressionStack(newStack, [](VirtualRegister expression, VirtualRegister slot) {
+        ASSERT(expression == slot);
+    });
+#endif
</ins><span class="cx"> 
</span><span class="cx">     Ref<Label> body = newEmittedLabel();
</span><span class="cx">     Ref<Label> continuation = newLabel();
</span></span></pre>
</div>
</div>

</body>
</html>