Skip to content

Fix and improve unreachable parsing #8617

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tlively wants to merge 1 commit into
base: simplify-unreachable-print
Choose a base branch
from
+103 −32
Open

Fix and improve unreachable parsing #8617

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 55 additions & 32 deletions src/wasm/wasm-ir-builder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,7 @@ MaybeResult<IRBuilder::HoistedVal> 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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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");
}
Expand All @@ -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
Expand Down Expand Up @@ -356,32 +361,26 @@ struct IRBuilder::ChildPopper
Result<> popConstrainedChildren(std::vector<Child>& children) {
auto& scope = builder.getScope();

// The index of the shallowest unreachable instruction on the stack, found
// by checkNeedsUnreachableFallback.
std::optional<size_t> 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<size_t> 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;
Expand All @@ -397,9 +396,10 @@ struct IRBuilder::ChildPopper
return Ok{};
}

bool checkNeedsUnreachableFallback(const std::vector<Child>& children,
std::optional<size_t>& unreachableIndex) {
std::optional<size_t>
checkNeedsUnreachableFallback(const std::vector<Child>& children) {
auto& scope = builder.getScope();
std::optional<size_t> unreachableFallbackSize;

// Two-part indices into the stack of available expressions and the vector
// of requirements, allowing them to move independently with the granularity
Expand All @@ -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) {
Expand All @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -848,6 +864,13 @@ Result<Expression*> 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 &&
Expand Down
48 changes: 48 additions & 0 deletions test/lit/wat-kitchen-sink.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading
⚡