Skip to content

Comments

Cranelift: fix invalid regalloc constraints on try-call with empty handler list.#10709

Merged
fitzgen merged 1 commit intobytecodealliance:mainfrom
cfallin:fix-try-call-with-empty-handlers
May 2, 2025
Merged

Cranelift: fix invalid regalloc constraints on try-call with empty handler list.#10709
fitzgen merged 1 commit intobytecodealliance:mainfrom
cfallin:fix-try-call-with-empty-handlers

Conversation

@cfallin
Copy link
Member

@cfallin cfallin commented May 2, 2025

(Reported by bjorn3 -- thanks!)

We handle edge splitting for regalloc moves in a way that is designed to split minimally (for compiler performance and codegen quality): we only split edges that are truly critical, i.e., come from a block with more than one successor and go to a block with more than one predecessor. In all other cases, there is always a place for these moves to go: if a block only has one successor, then edge-moves can go before its jump; and if a block only has one predecessor, then edge-moves can go at the beginning of that block (before the blockparams' parallel-move). The former case -- before the branch -- works because of a restriction that regalloc2 imposes: branches with only one target (that is, unconditional branches) cannot have any arguments. Otherwise, those uses would occur after the edge-moves have already shuffled the register state into a state suitable for the next block. Violating this constraint leads to a panic.

With ordinary unconditional branches, this is no problem. With try_calls with at least one handler listed, this is also no problem: such a terminator is seen as a branch with (at least) two targets by regalloc, so no edge-moves are placed before it, so it's allowed to have arguments, such as the arguments to the call itself. However, a try_call with no handler clauses, though pathological, appears to regalloc as an unconditional branch and so should not have any arguments. The included test-case triggers this issue with such a try_call together with a normal-return target branch that has more than one incoming edge, forcing the location for the moves into the try_call's block. (The lack of actual edge-moves doesn't matter -- RA2 performs the check on the IR restriction first.) The result is a panic at compile time.

This PR fixes the issue by extending a similar fix for br_tables (which can trigger a very similar bug if they have only the default case, i.e., one target) to try_call{,_indirect} as well: the lowered-block order computation, where edge-splits are determined, pretends that they always have at least two successors. This ensures that edges will be split as necessary, satisfying the no-arguments-to-unconditional-terminators restriction.

@cfallin cfallin requested a review from a team as a code owner May 2, 2025 17:41
@cfallin cfallin requested review from fitzgen and removed request for a team May 2, 2025 17:41
@cfallin cfallin force-pushed the fix-try-call-with-empty-handlers branch from eff5001 to 9f8753e Compare May 2, 2025 17:42
@bjorn3
Copy link
Contributor

bjorn3 commented May 2, 2025

Please remove the @ mention from the commit message and PR description to avoid pinging me every time someone pushes it to their repo.

…ndler list.

(Reported by bjorn3 -- thanks!)

We handle edge splitting for regalloc moves in a way that is designed to
split minimally (for compiler performance and codegen quality): we only
split edges that are truly critical, i.e., come from a block with more
than one successor and go to a block with more than one predecessor.
In all other cases, there is always a place for these moves to go: if a
block only has one successor, then edge-moves can go before its jump;
and if a block only has one predecessor, then edge-moves can go at the
beginning of that block (before the blockparams' parallel-move). The
former case -- before the branch -- works because of a restriction that
regalloc2 imposes: branches with only one target (that is, unconditional
branches) cannot have any arguments. Otherwise, those uses would occur
after the edge-moves have already shuffled the register state into a
state suitable for the next block. Violating this constraint leads to a
panic.

With ordinary unconditional branches, this is no problem. With
`try_call`s with at least one handler listed, this is also no problem:
such a terminator is seen as a branch with (at least) two targets by
regalloc, so no edge-moves are placed before it, so it's allowed to have
arguments, such as the arguments to the call itself. However, a
`try_call` with *no* handler clauses, though pathological, appears to
regalloc as an unconditional branch and so should not have any
arguments. The included test-case triggers this issue with such a
`try_call` together with a normal-return target branch that has more
than one incoming edge, forcing the location for the moves into the
`try_call`'s block. (The lack of actual edge-moves doesn't matter -- RA2
performs the check on the IR restriction first.) The result is a panic
at compile time.

This PR fixes the issue by extending a similar fix for `br_table`s
(which can trigger a very similar bug if they have only the default
case, i.e., one target) to `try_call{,_indirect}` as well: the
lowered-block order computation, where edge-splits are determined,
pretends that they always have at least two successors. This ensures
that edges will be split as necessary, satisfying the
no-arguments-to-unconditional-terminators restriction.
@cfallin cfallin force-pushed the fix-try-call-with-empty-handlers branch from 9f8753e to c7c36f8 Compare May 2, 2025 17:56
@cfallin
Copy link
Member Author

cfallin commented May 2, 2025

Sorry about that -- amended the commit and edited the description.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks for the detailed description! Made understanding this easy 👍

@fitzgen fitzgen added this pull request to the merge queue May 2, 2025
Merged via the queue into bytecodealliance:main with commit 14db3bf May 2, 2025
41 checks passed
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.

3 participants