From 1744c8e5697f1b8e2e458b36c205301475fe5629 Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Thu, 16 Apr 2026 19:51:00 -0700 Subject: [PATCH 1/2] [NFC] Simplify printing of unreachable replacements When an instruction has a type immediate that we cannot print because the expression it is supposed to come from has unreachable or null type, we instead print an unreachable block with a comment saying what instruction we failed to print. We previously handled this via different code paths for type immediates that come from child expressions that type immediates that come from the printed expression's own return type. Unify these code paths in the printer by improving `Properties::hasUnwritableTypeImmediate` to handle both cases. Also fix the printing of `ShallowExpression` to check for unwritable type immediates first to avoid assertion failures. --- src/ir/properties.h | 23 ++++++++++++-- src/passes/Print.cpp | 74 +++----------------------------------------- 2 files changed, 25 insertions(+), 72 deletions(-) diff --git a/src/ir/properties.h b/src/ir/properties.h index e52902597a0..8d1f0faaf84 100644 --- a/src/ir/properties.h +++ b/src/ir/properties.h @@ -521,8 +521,8 @@ inline MemoryOrder getMemoryOrder(Expression* curr) { } // Whether this instruction will be unwritable in the text and binary formats -// because it requires a type index immediate giving the type of a child that -// has unreachable or null type, and therefore does not have a type index. +// because it requires a type index immediate computed from an expression with +// unreachable or null type, and therefore no type index. inline bool hasUnwritableTypeImmediate(Expression* curr) { #define DELEGATE_ID curr->_id @@ -552,6 +552,25 @@ inline bool hasUnwritableTypeImmediate(Expression* curr) { #include "wasm-delegations-fields.def" + if (curr->type == Type::unreachable) { + if (curr->is() || curr->is() || + curr->is() || curr->is() || + curr->is() || curr->is() || + curr->is()) { + return true; + } + if (auto* cast = curr->dynCast()) { + if (!cast->desc) { + return true; + } + if (!cast->desc->type.isRef()) { + return true; + } + if (!cast->desc->type.getHeapType().getDescribedType()) { + return true; + }; + } + } return false; } diff --git a/src/passes/Print.cpp b/src/passes/Print.cpp index 11a73315b74..68981953211 100644 --- a/src/passes/Print.cpp +++ b/src/passes/Print.cpp @@ -320,63 +320,6 @@ struct PrintSExpression : public UnifiedExpressionVisitor { void visitTryTable(TryTable* curr); void printUnreachableReplacement(Expression* curr); - bool maybePrintUnreachableReplacement(Expression* curr, Type type); - void visitRefCast(RefCast* curr) { - if ((curr->desc && curr->desc->type != Type::unreachable) || - !maybePrintUnreachableReplacement(curr, curr->type)) { - visitExpression(curr); - } - } - void visitStructNew(StructNew* curr) { - if (!maybePrintUnreachableReplacement(curr, curr->type)) { - visitExpression(curr); - } - } - void visitArrayNew(ArrayNew* curr) { - if (!maybePrintUnreachableReplacement(curr, curr->type)) { - visitExpression(curr); - } - } - void visitArrayNewData(ArrayNewData* curr) { - if (!maybePrintUnreachableReplacement(curr, curr->type)) { - visitExpression(curr); - } - } - void visitArrayNewElem(ArrayNewElem* curr) { - if (!maybePrintUnreachableReplacement(curr, curr->type)) { - visitExpression(curr); - } - } - void visitArrayNewFixed(ArrayNewFixed* curr) { - if (!maybePrintUnreachableReplacement(curr, curr->type)) { - visitExpression(curr); - } - } - void visitContNew(ContNew* curr) { - if (!maybePrintUnreachableReplacement(curr, curr->type)) { - visitExpression(curr); - } - } - void visitContBind(ContBind* curr) { - if (!maybePrintUnreachableReplacement(curr, curr->type)) { - visitExpression(curr); - } - } - void visitResume(Resume* curr) { - if (!maybePrintUnreachableReplacement(curr, curr->type)) { - visitExpression(curr); - } - } - void visitResumeThrow(ResumeThrow* curr) { - if (!maybePrintUnreachableReplacement(curr, curr->type)) { - visitExpression(curr); - } - } - void visitStackSwitch(StackSwitch* curr) { - if (!maybePrintUnreachableReplacement(curr, curr->type)) { - visitExpression(curr); - } - } // Module-level visitors void handleSignature(Function* curr, bool printImplicitNames = false); @@ -3143,19 +3086,6 @@ void PrintSExpression::printUnreachableReplacement(Expression* curr) { decIndent(); } -bool PrintSExpression::maybePrintUnreachableReplacement(Expression* curr, - Type type) { - // When we cannot print an instruction because the child from which it's - // supposed to get a type immediate is unreachable, then we print a - // semantically-equivalent block that drops each of the children and ends in - // an unreachable. - if (type == Type::unreachable) { - printUnreachableReplacement(curr); - return true; - } - return false; -} - static bool requiresExplicitFuncType(HeapType type) { // When the `(type $f)` in a function's typeuse is omitted, the typeuse // matches or declares an MVP function type. When the intended type is not an @@ -4009,6 +3939,10 @@ std::ostream& operator<<(std::ostream& o, wasm::ModuleExpression pair) { } std::ostream& operator<<(std::ostream& o, wasm::ShallowExpression expression) { + if (Properties::hasUnwritableTypeImmediate(expression.expr)) { + o << "(; unreachable " << getExpressionName(expression.expr) << " ;)"; + return o; + } wasm::PrintSExpression printer(o); printer.setModule(expression.module); wasm::PrintExpressionContents(printer).visit(expression.expr); From c0d13e289ecf1b902219cd07a02877783966bce6 Mon Sep 17 00:00:00 2001 From: Thomas Lively Date: Thu, 16 Apr 2026 21:23:04 -0700 Subject: [PATCH 2/2] Fix and improve unreachable parsing The recent change (#8608) that reduced the number of scratch locals introduced by IRBuilder also inadvertently introduced a bug. That commit changed the conditions under which we package multiple expressions into a block to be popped together. We previously did this whenever there was a get of a scratch local to hoist a value to the top of the stack. After #8608, we started using blocks whenever there were multiple expressions hoisted. These conditions are usually identical, but we missed the case where there are multiple expressions and also no `get` because the value is unreachable. The introduction of a block where there was none before invalidated our logic for determining whether or not to pop back past an unreachable expression based on whether the expressions underneath it would satisfy the type constraints of the parent. Simplify that logic by calculating the stack size at which we must avoid popping past an unreachable rather than calculating the index of the unreachable itself. This makes the logic work correctly whether or not the unreachable will be popped as part of a block. That in turn lets us remove an extra unreachable we were conservatively pushing onto the stack when "hoisting" unreachable values. Add tests for the cases where we can and cannot pop past an unreachable that is followed by a none-typed expression. The former case previously parsed incorrectly and the latter case is now parsed without introducing extra unreachable instructions. --- src/wasm/wasm-ir-builder.cpp | 87 +++++++++++++++++++++------------- test/lit/wat-kitchen-sink.wast | 48 +++++++++++++++++++ 2 files changed, 103 insertions(+), 32 deletions(-) diff --git a/src/wasm/wasm-ir-builder.cpp b/src/wasm/wasm-ir-builder.cpp index 55ef512b103..6552df7e1a2 100644 --- a/src/wasm/wasm-ir-builder.cpp +++ b/src/wasm/wasm-ir-builder.cpp @@ -87,10 +87,7 @@ MaybeResult IRBuilder::hoistLastValue(bool greedy) { } auto*& expr = stack[valIndex]; if (expr->type == Type::unreachable) { - // Make sure the top of the stack also has an unreachable expression. - if (stack.back()->type != Type::unreachable) { - pushSynthetic(builder.makeUnreachable()); - } + // No need for a scratch local to hoist an unreachable. return HoistedVal{Index(hoistIndex), nullptr}; } // Hoist with a scratch local. Normally the scratch local is the same type as @@ -129,6 +126,11 @@ Result<> IRBuilder::packageHoistedValue(const HoistedVal& hoisted, }; auto type = scope.exprStack.back()->type; + if (type == Type::none) { + // If we did not have a value on top of the stack and did not add a scratch + // local, then there must have been an unreachable. + type = Type::unreachable; + } if (type.size() == sizeHint || type.size() <= 1) { if (hoisted.hoistIndex < scope.exprStack.size() - 1) { @@ -280,6 +282,8 @@ void IRBuilder::dump() { if (tryy->name) { std::cerr << " " << tryy->name; } + } else if (scope.getTryTable()) { + std::cerr << "try_table"; } else { WASM_UNREACHABLE("unexpected scope"); } @@ -303,7 +307,8 @@ void IRBuilder::dump() { std::cerr << ":\n"; for (auto* expr : scope.exprStack) { - std::cerr << " " << ShallowExpression{expr} << "\n"; + std::cerr << " " << ShallowExpression{expr} << " (; " + << expr->type.toString() << " ;)\n"; } } #endif // IR_BUILDER_DEBUG @@ -356,32 +361,26 @@ struct IRBuilder::ChildPopper Result<> popConstrainedChildren(std::vector& children) { auto& scope = builder.getScope(); - // The index of the shallowest unreachable instruction on the stack, found - // by checkNeedsUnreachableFallback. - std::optional unreachableIndex; - - // Whether popping the children past the unreachable would produce a type - // mismatch or try to pop from an empty stack. - bool needUnreachableFallback = false; + // The stack size at which we are about to pop an unreachable instruction + // that we must not pop past because the expressions underneath it do not + // have the types we need. + std::optional unreachableFallbackSize; // We only need to check requirements if there is an unreachable. // Otherwise the validator will catch any problems. if (scope.unreachable) { - needUnreachableFallback = - checkNeedsUnreachableFallback(children, unreachableIndex); + unreachableFallbackSize = checkNeedsUnreachableFallback(children); } // We have checked all the constraints, so we are ready to pop children. for (int i = children.size() - 1; i >= 0; --i) { - if (needUnreachableFallback && - scope.exprStack.size() == *unreachableIndex + 1 && i > 0) { + if (unreachableFallbackSize && + scope.exprStack.size() == *unreachableFallbackSize && i > 0) { // The next item on the stack is the unreachable instruction we must - // not pop past. We cannot insert unreachables in front of it because - // it might be a branch we actually have to execute, so this next item - // must be child 0. But we are not ready to pop child 0 yet, so - // synthesize an unreachable instead of popping. The deeper - // instructions that would otherwise have been popped will remain on - // the stack to become prior children of future expressions or to be + // not pop past. We could pop it as child 0, but we are not ready to pop + // child 0 yet, so synthesize an unreachable instead of popping. The + // deeper instructions that would otherwise have been popped will remain + // on the stack to become prior children of future expressions or to be // implicitly dropped at the end of the scope. *children[i].childp = builder.builder.makeUnreachable(); continue; @@ -397,9 +396,10 @@ struct IRBuilder::ChildPopper return Ok{}; } - bool checkNeedsUnreachableFallback(const std::vector& children, - std::optional& unreachableIndex) { + std::optional + checkNeedsUnreachableFallback(const std::vector& children) { auto& scope = builder.getScope(); + std::optional unreachableFallbackSize; // Two-part indices into the stack of available expressions and the vector // of requirements, allowing them to move independently with the granularity @@ -409,6 +409,9 @@ struct IRBuilder::ChildPopper size_t childIndex = children.size(); size_t childTupleIndex = 0; + // Whether we are deeper than some concrete expression. + bool seenConcrete = false; + // Check whether the values on the stack will be able to meet the given // requirements. while (true) { @@ -435,7 +438,7 @@ struct IRBuilder::ChildPopper // the input unreachable instruction is executed first. If we are // not reaching past an unreachable, the error will be caught when // we pop. - return true; + return unreachableFallbackSize; } --stackIndex; stackTupleIndex = scope.exprStack[stackIndex]->type.size() - 1; @@ -450,22 +453,35 @@ struct IRBuilder::ChildPopper } // We have an available type and a constraint. Only check constraints if - // we are past an unreachable, since otherwise we can leave problems to be - // caught by the validator later. + // we are deeper than an unreachable, since otherwise we can leave + // problems to be caught by the validator later. auto type = scope.exprStack[stackIndex]->type[stackTupleIndex]; - if (unreachableIndex) { + if (unreachableFallbackSize) { auto constraint = children[childIndex].constraint[childTupleIndex]; if (!PrincipalType::matches(type, constraint)) { - return true; + return unreachableFallbackSize; } } - // No problems for children after this unreachable. + // We may need an unreachable fallback if we find violated constraints in + // expressions deeper than this unreachable. Calculate the stack size at + // which this unreachable will be the next thing popped. This size is + // usually one more than the current index, but if there are no shallower + // concrete expressions, then everything down to this unreachable will be + // popped at once immediately at the current stack size. if (type == Type::unreachable) { - unreachableIndex = stackIndex; + unreachableFallbackSize = + seenConcrete ? stackIndex + 1 : scope.exprStack.size(); + } else { + // We skipped none-typed expressions and this isn't unreachable, so it + // must be concrete. + assert(type.isConcrete()); + seenConcrete = true; } } - return false; + + // No unreachable fallback necessary. + return std::nullopt; } // If `greedy`, then we will pop additional none-typed expressions that come @@ -848,6 +864,13 @@ Result IRBuilder::finishScope(Block* block) { return Err{"popping from empty stack"}; } + if (scope.exprStack.back()->type == Type::none) { + // Nothing was hoisted, which means there must have been an unreachable + // buried under none-type expressions. It is not valid to end a concretely + // typed block with none-typed expressions, so add an extra unreachable. + pushSynthetic(builder.makeUnreachable()); + } + if (type.isTuple()) { auto hoistedType = scope.exprStack.back()->type; if (hoistedType != Type::unreachable && diff --git a/test/lit/wat-kitchen-sink.wast b/test/lit/wat-kitchen-sink.wast index 7a92d0dcfdf..0b04d385961 100644 --- a/test/lit/wat-kitchen-sink.wast +++ b/test/lit/wat-kitchen-sink.wast @@ -5260,6 +5260,54 @@ ) ) + ;; CHECK: (func $stacky-unreachable-fallback (type $0) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (i32.const 0) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (block ;; (replaces unreachable StructSet we can't emit) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (block + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $stacky-unreachable-fallback + ;; i32 cannot be the struct.set's ref child, so we cannot pop past the + ;; unreachable. The unreachable and the following nop are popped together. + i32.const 0 + unreachable + nop + struct.set $pair 0 + ) + + ;; CHECK: (func $stacky-unreachable-ok (type $0) + ;; CHECK-NEXT: (struct.set $pair $first + ;; CHECK-NEXT: (struct.new_default $pair) + ;; CHECK-NEXT: (block + ;; CHECK-NEXT: (unreachable) + ;; CHECK-NEXT: (nop) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + ;; CHECK-NEXT: ) + (func $stacky-unreachable-ok + ;; Now we can pop past the unreachable. The unreachable and nop are still + ;; popped together. + struct.new_default $pair + unreachable + nop + struct.set $pair 0 + ) + + ;; CHECK: (func $paren-in-string (type $0) + ;; CHECK-NEXT: (drop + ;; CHECK-NEXT: (string.const ")") + ;; CHECK-NEXT: ) (func $paren-in-string ;; We should not be tripped up by an extra close parenthesis inside a string. (drop