- Revision
- 279082
- Author
- commit-qu...@webkit.org
- Date
- 2021-06-21 14:06:49 -0700 (Mon, 21 Jun 2021)
Log Message
[JSC] Fix consistency check during stack splitting in Wasm::LLIntGenerator::addLoop
https://bugs.webkit.org/show_bug.cgi?id=226012
Patch by Xan Lopez <x...@igalia.com> on 2021-06-21
Reviewed by Tadeu Zagallo.
JSTests:
* stress/wasm-loop-consistency.js: Added.
(vm.isWasmSupported):
Source/_javascript_Core:
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):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (279081 => 279082)
--- trunk/JSTests/ChangeLog 2021-06-21 20:55:58 UTC (rev 279081)
+++ trunk/JSTests/ChangeLog 2021-06-21 21:06:49 UTC (rev 279082)
@@ -1,3 +1,13 @@
+2021-06-21 Xan Lopez <x...@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):
+
2021-06-21 Yusuke Suzuki <ysuz...@apple.com>
Release assert memory in JSC::Wasm::Memory::growShared(JSC::Wasm::PageCount)::<lambda()>
Added: trunk/JSTests/stress/wasm-loop-consistency.js (0 => 279082)
--- 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)
@@ -0,0 +1,14 @@
+// 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]));
+}
+
Modified: trunk/Source/_javascript_Core/ChangeLog (279081 => 279082)
--- trunk/Source/_javascript_Core/ChangeLog 2021-06-21 20:55:58 UTC (rev 279081)
+++ trunk/Source/_javascript_Core/ChangeLog 2021-06-21 21:06:49 UTC (rev 279082)
@@ -1,3 +1,34 @@
+2021-06-21 Xan Lopez <x...@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):
+
2021-06-21 Yusuke Suzuki <ysuz...@apple.com>
Release assert memory in JSC::Wasm::Memory::growShared(JSC::Wasm::PageCount)::<lambda()>
Modified: trunk/Source/_javascript_Core/wasm/WasmLLIntGenerator.cpp (279081 => 279082)
--- trunk/Source/_javascript_Core/wasm/WasmLLIntGenerator.cpp 2021-06-21 20:55:58 UTC (rev 279081)
+++ trunk/Source/_javascript_Core/wasm/WasmLLIntGenerator.cpp 2021-06-21 21:06:49 UTC (rev 279082)
@@ -379,12 +379,8 @@
#endif // ASSERT_ENABLED
}
- void materializeConstantsAndLocals(Stack& expressionStack)
+ void materializeConstantsAndLocals(Stack& expressionStack, NoConsistencyCheckTag)
{
- if (expressionStack.isEmpty())
- return;
-
- checkConsistency();
walkExpressionStack(expressionStack, [&](TypedExpression& _expression_, VirtualRegister slot) {
ASSERT(_expression_.value() == slot || _expression_.value().isConstant() || _expression_.value().isArgument() || static_cast<unsigned>(_expression_.value().toLocal()) < m_codeBlock->m_numVars);
if (_expression_.value() == slot)
@@ -392,7 +388,16 @@
WasmMov::emit(this, slot, _expression_);
_expression_ = TypedExpression { _expression_.type(), slot };
});
+ }
+
+ void materializeConstantsAndLocals(Stack& expressionStack)
+ {
+ if (expressionStack.isEmpty())
+ return;
+
checkConsistency();
+ materializeConstantsAndLocals(expressionStack, NoConsistencyCheck);
+ checkConsistency();
}
void splitStack(BlockSignature signature, Stack& enclosingStack, Stack& newStack)
@@ -871,7 +876,20 @@
auto LLIntGenerator::addLoop(BlockSignature signature, Stack& enclosingStack, ControlType& block, Stack& newStack, uint32_t loopIndex) -> PartialResult
{
splitStack(signature, enclosingStack, newStack);
- materializeConstantsAndLocals(newStack);
+ 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
Ref<Label> body = newEmittedLabel();
Ref<Label> continuation = newLabel();
