Skip to content

Cranelift: fix fuzzbug in critical-edge splitting. - #4044

Merged
cfallin merged 1 commit into
bytecodealliance:mainfrom
cfallin:fix-empty-brtable-fuzzbug
Apr 18, 2022
Merged

Cranelift: fix fuzzbug in critical-edge splitting.#4044
cfallin merged 1 commit into
bytecodealliance:mainfrom
cfallin:fix-empty-brtable-fuzzbug

Conversation

@cfallin

@cfallin cfallin commented Apr 16, 2022

Copy link
Copy Markdown
Member

regalloc2 is a bit pickier about critical edges than regalloc.rs was,
because of how it inserts moves. In particular, if a branch has any
arguments (e.g., a conditional branch or br_table), its successors must
all have only one predecessor, so we can do edge moves at the top of
successor blocks rather than at the end of this block. Otherwise, moves
that semantically must come after the block's last uses (the branch's
args) would be placed before it.

This is almost always the case, because crit-edge splitting ensures that
if we have more than one succ, all our succs will have only one pred.
This is because branch kinds that take arguments (fixed args, not the
blockparam args) tend to have more than one successor: conditionals and
br_tables.

However, a fuzzbug recently illuminated one corner case I had missed: a
br_table can have one successor only, if it has a default target and
an empty table. In this case, crit-edge splitting will happily skip a
split and assume that we can insert edge moves at the end of the block
with the br_table. But this will fail.

regalloc2 explicitly checks this and bails with a panic, rather than
continue, so no miscompilation is possible; but without this fix, we
will get these panics on br_tables with empty tables.

@cfallin
cfallin requested review from alexcrichton and fitzgen April 16, 2022 03:52
@github-actions github-actions Bot added cranelift Issues related to the Cranelift code generator cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. labels Apr 16, 2022
@alexcrichton

Copy link
Copy Markdown
Member

Ah I'm not really all that certain what this is doing, so I'll leave review to @fitzgen

Comment thread cranelift/codegen/src/machinst/blockorder.rs Outdated
@cfallin
cfallin force-pushed the fix-empty-brtable-fuzzbug branch from 670ab10 to 4a04d66 Compare April 18, 2022 17:17
@cfallin

cfallin commented Apr 18, 2022

Copy link
Copy Markdown
Member Author

Thanks! Updated, hopefully the comment is more clear now.

regalloc2 is a bit pickier about critical edges than regalloc.rs was,
because of how it inserts moves. In particular, if a branch has any
arguments (e.g., a conditional branch or br_table), its successors must
all have only one predecessor, so we can do edge moves at the top of
successor blocks rather than at the end of this block. Otherwise, moves
that semantically must come after the block's last uses (the branch's
args) would be placed before it.

This is almost always the case, because crit-edge splitting ensures that
if we have more than one succ, all our succs will have only one pred.
This is because branch kinds that take arguments (fixed args, not the
blockparam args) tend to have more than one successor: conditionals and
br_tables.

However, a fuzzbug recently illuminated one corner case I had missed: a
br_table can have *one* successor only, if it has a default target and
an empty table. In this case, crit-edge splitting will happily skip a
split and assume that we can insert edge moves at the end of the block
with the br_table. But this will fail.

regalloc2 explicitly checks this and bails with a panic, rather than
continue, so no miscompilation is possible; but without this fix, we
will get these panics on br_tables with empty tables.
@cfallin
cfallin force-pushed the fix-empty-brtable-fuzzbug branch from 4a04d66 to 0e28ff2 Compare April 18, 2022 17:18

@fitzgen fitzgen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Okay, this makes sense now. Thanks for updating the comment!

@cfallin
cfallin merged commit 5aa9bdc into bytecodealliance:main Apr 18, 2022
@cfallin
cfallin deleted the fix-empty-brtable-fuzzbug branch April 18, 2022 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cranelift:area:machinst Issues related to instruction selection and the new MachInst backend. cranelift Issues related to the Cranelift code generator

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants