Skip to content
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

Jump to bottom

[Parser] Fix bug in unreachable fallback logic #6676

Merged
merged 2 commits into from
Jun 19, 2024
Merged

[Parser] Fix bug in unreachable fallback logic #6676

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5526930
[Parser] Fix bug in unreachable fallback logic
tlively Jun 18, 2024
cebdb16
fix comment
tlively Jun 18, 2024
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
3 changes: 0 additions & 3 deletions scripts/test/shared.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,9 +417,6 @@ def get_tests(test_dir, extensions=[], recursive=False):
'type.wast',
'unreached-invalid.wast',

# WAT parser error
'unwind.wast',

# WAST parser error
'binary.wast',

Expand Down
31 changes: 18 additions & 13 deletions src/wasm/wasm-ir-builder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,9 +407,12 @@ struct IRBuilder::ChildPopper
--stackTupleIndex;
} else {
if (stackIndex == 0) {
// No more available values. This is fine iff we are reaching past
// an unreachable. Any error will be caught later when we are
// popping.
// No more available values. This is valid iff we are reaching past
// an unreachable, but we still need the fallback behavior to ensure
// the input unreachable instruction is executed first. If we are
// not reaching past an unreachable, the error will be caught when
// we pop.
needUnreachableFallback = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to change the meaning of needUnreachableFallback slightly - worth updating the comment on line 379 perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That comment is actually already correct because it mentions "or try to pop from an empty stack." This fixes the code to agree with the comment.

goto pop;
}
--stackIndex;
Expand Down Expand Up @@ -458,18 +461,20 @@ struct IRBuilder::ChildPopper
// 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) {
// The expressions remaining on the stack may be executed, but they do
// not satisfy the requirements to be children of the current parent.
// Explicitly drop them so they will still be executed for their side
// effects and so the remaining children will be filled with
// unreachables.
assert(scope.exprStack.back()->type == Type::unreachable);
for (auto& expr : scope.exprStack) {
expr = Builder(builder.wasm).dropIfConcretelyTyped(expr);
}
scope.exprStack.size() == *unreachableIndex + 1 && 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am having a hard time following this. "The next item is the unreachable we must not pop past" fits scope.exprStack.size() == *unreachableIndex + 1. Then "this next item be child 0" seems to refer to the same "next item", which suggests *unreachableIndex == 0. Is that right? If so then I can't see why.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, *unreachableIndex is an index in the stack, not in the children we are popping. i is the index of the child we want to pop, so that's why we check i > 0 here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That helps, thanks. But if i is the index of the child we want to pop, and we check i > 0 before getting here, how can so this next item must be child 0 be true?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because inside this condition we are not popping a value from the stack, we are synthesizing an unreachable instead. We're deferring popping that value until i == 0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Ok, I think I understand the surrounding code a bit better now. But I am still quite confused 😄 Specifically, about this sentence:

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.

I can't follow the inference of "so". I can try to paraphrase the key part of it as "We can't insert unreachables, as it might be a branch, and therefore it must be child 0". Neither "we can't insert unreachables" nor "it might be a branch" suggest to me any reason why the child must be 0 and not any other index.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we put the unreachable instruction from the stack in the child 1 slot, then there would be nothing valid to do for the child 0 slot. Popping off the stack would be popping past the unreachable, which we determined we cannot do. Synthesizing an Unreachable for child 0 would insert that Unreachable before the unreachable instruction from the stack in the execution order and change the program semantics if the instruction from the stack was a branch.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is that not a problem for the child 2 slot as well? If we put the unreachable instruction from the stack in the child 2 slot then there would be nothing to do for the child 1 or child 0 slots, I think, for the exact reasons you said?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, it's a problem for any slot except for child 0. That's why the child 0 slot is special.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now, thanks... sorry for being dense!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha no worries! It's certainly a dense piece of code.

// 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;
}

// Pop a child normally.
auto val = pop(children[i].constraint.size());
CHECK_ERR(val);
*children[i].childp = *val;
Expand Down
38 changes: 37 additions & 1 deletion test/lit/wat-kitchen-sink.wast
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2728,6 +2728,42 @@
br 0
)

;; CHECK: (func $br-mismatch-after (type $1) (result i32)
;; CHECK-NEXT: (block $label (result i32)
;; CHECK-NEXT: (i32.add
;; CHECK-NEXT: (br $label
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $br-mismatch-after (result i32)
i32.const 1
br 0
i32.add
)

;; CHECK: (func $br-mismatch-after-extra (type $1) (result i32)
;; CHECK-NEXT: (block $label (result i32)
;; CHECK-NEXT: (drop
;; CHECK-NEXT: (i64.const 0)
;; CHECK-NEXT: )
;; CHECK-NEXT: (i32.add
;; CHECK-NEXT: (br $label
;; CHECK-NEXT: (i32.const 1)
;; CHECK-NEXT: )
;; CHECK-NEXT: (unreachable)
;; CHECK-NEXT: )
;; CHECK-NEXT: )
;; CHECK-NEXT: )
(func $br-mismatch-after-extra (result i32)
i64.const 0
i32.const 1
br 0
i32.add
)

;; CHECK: (func $br_if (type $0)
;; CHECK-NEXT: (block $l
;; CHECK-NEXT: (br_if $l
Expand Down Expand Up @@ -3669,7 +3705,7 @@
(func $ref-func
ref.func $ref-func
drop
ref.func 159
ref.func 161
drop
)

Expand Down
Loading