Skip to content

[EH] Fix missing outer block for catchless try #6519

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.

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

Merged
merged 4 commits into from
Apr 23, 2024

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Apr 22, 2024

When translating a try expression, we may need an 'outer' block that wraps the newly generated try_table so we can jump out of the expression when an exception does not occur. (The condition we use is when the try has any catches or if the try is a target of any inner try-delegates:

if (it != delegateTargetToBrTarget.end() || curr->isCatch()) {
)

In case the try has either of catch or delegate, when we have the 'outer' block, we add the newly created try_table in the 'outer' block and replace the whole expression with the block:

replaceCurrent(outerBlock);
// If we need an outer block for other reasons (if this is a target of a
// delegate), we insert the new try_table into it. If not we just replace
// the current try with the new try_table.
if (outerBlock) {
outerBlock->list.push_back(tryTable);
replaceCurrent(outerBlock);
} else {
replaceCurrent(tryTable);
}

But in case of a catchless try, we forgot to do that:

replaceCurrent(tryTable);

So this PR fixes it.

When translating a `try` expression, we may need an 'outer' block that
wraps the newly generated `try_table` so we can jump out of the
expression when an exception does not occur. (The condition we use is
when the `try` has any catches or if the `try` is a target of any
inner `try-delegate`s:
https://github.com/WebAssembly/binaryen/blob/219e668e87b012c0634043ed702534b8be31231f/src/passes/TranslateEH.cpp#L677)

In case the `try` has either of `catch` or `delegate`, when we have the
'outer' block, we add the newly created `try_table` in the 'outer' block
and replace the whole expression with the block:
https://github.com/WebAssembly/binaryen/blob/219e668e87b012c0634043ed702534b8be31231f/src/passes/TranslateEH.cpp#L670
https://github.com/WebAssembly/binaryen/blob/219e668e87b012c0634043ed702534b8be31231f/src/passes/TranslateEH.cpp#L332-L340

But in case of a catchless `try`, we forgot to do that:
https://github.com/WebAssembly/binaryen/blob/219e668e87b012c0634043ed702534b8be31231f/src/passes/TranslateEH.cpp#L388

So this PR fixes it.
@aheejin aheejin requested a review from kripken April 22, 2024 06:11
;; CHECK-NEXT: (block $l00 (result exnref)
;; CHECK-NEXT: (try_table (catch_all_ref $l00)
;; CHECK-NEXT: (call $foo)
;; CHECK-NEXT: (block $outer1
Copy link
Member

Choose a reason for hiding this comment

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

Why was this outer block needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Before we go into actually processing catches and delegates, we create an outer block in this condition:

if (it != delegateTargetToBrTarget.end() || curr->isCatch()) {

Because

  1. In case of a delegate, it will be translated into a throw_ref to the delegate target. But when an exception does not occur, we have to bail out of that throw_ref. And this outer block is that bail-out destination. It is explained here:

    // (br $outer)

    // (br $outer ;; Now has the try_table as a child.

  2. In case of catches, we proceed into catch handler parts unless we bail out in case an exception does not occur. It is explained here:

    // (br $outer)

    // (br $outer

But this test does not need a br because the body's type is unreachable due to a return. We omit that br here:

auto* brToOuter = curr->body->type == Type::unreachable
? nullptr
: builder.makeBreak(outerBlock->name);

It is hard to check this condition before creating an outer block. We can run BranchSeeker to find out if the block is actually used as a destination for a branch afterwards to remove this unnecessary block. Do you want me to do that? It is not hard, but I didn't end up doing that because we might be adding too much optimization in this pass which should ideally be done by other passes later. Currently we run this pass at the end and don't run any other optimization passes after this, I already baked some amount of optimization into it though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tried removing it without using BranchSeeker: 8c8fcc5

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Great!

@aheejin aheejin merged commit 3b9dc42 into WebAssembly:main Apr 23, 2024
@aheejin aheejin deleted the translate_fix_outerblock branch April 23, 2024 15:59
@gkdn gkdn mentioned this pull request Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants