wasmtime / PR #10709 Cranelift: fix invalid regalloc cons... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #10709 Cranelift: fix invalid regalloc cons...

Wasmtime GitHub notifications bot (May 02 2025 at 17:41):

cfallin opened PR #10709 from cfallin:fix-try-call-with-empty-handlers to bytecodealliance:main:

(Reported by @bjorn3 -- thanks!)

We handle edge splitting for regalloc moves in a way that is designed to split minimally (for copmiler 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.

Wasmtime GitHub notifications bot (May 02 2025 at 17:41):

cfallin requested fitzgen for a review on PR #10709.

Wasmtime GitHub notifications bot (May 02 2025 at 17:41):

cfallin requested wasmtime-compiler-reviewers for a review on PR #10709.

Wasmtime GitHub notifications bot (May 02 2025 at 17:42):

cfallin updated PR #10709.

Wasmtime GitHub notifications bot (May 02 2025 at 17:42):

cfallin edited PR #10709:

(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.

Wasmtime GitHub notifications bot (May 02 2025 at 17:52):

bjorn3 commented on PR #10709:

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

Wasmtime GitHub notifications bot (May 02 2025 at 17:56):

cfallin updated PR #10709.

Wasmtime GitHub notifications bot (May 02 2025 at 17:56):

cfallin edited PR #10709:

(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.

Wasmtime GitHub notifications bot (May 02 2025 at 17:56):

cfallin commented on PR #10709:

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

Wasmtime GitHub notifications bot (May 02 2025 at 18:39):

fitzgen submitted PR review:

Thanks for the detailed description! Made understanding this easy :+1:

Wasmtime GitHub notifications bot (May 02 2025 at 19:01):

fitzgen merged PR #10709.

Last updated: Dec 06 2025 at 07:03 UTC