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

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.
This commit is contained in:
Chris Fallin
2022-04-18 10:59:26 -07:00
committed by GitHub
parent 3f3afb455e
commit 5aa9bdc7eb

View File

@@ -253,6 +253,34 @@ impl BlockLoweringOrder {
block_in_count[entry] += 1; block_in_count[entry] += 1;
} }
// All blocks ending in conditional branches or br_tables must
// have edge-moves inserted at the top of successor blocks,
// not at the end of themselves. This is because the moves
// would have to be inserted prior to the branch's register
// use; but RA2's model is that the moves happen *on* the
// edge, after every def/use in the block. RA2 will check for
// "branch register use safety" and panic if such a problem
// occurs. To avoid this, we force the below algorithm to
// never merge the edge block onto the end of a block that
// ends in a conditional branch. We do this by "faking" more
// than one successor, even if there is only one.
//
// (One might ask, isn't that always the case already? It
// could not be, in cases of br_table with no table and just a
// default label, for example.)
for block in f.layout.blocks() {
for inst in f.layout.block_likely_branches(block) {
// If the block has a branch with any "fixed args"
// (not blockparam args) ...
if f.dfg[inst].opcode().is_branch() && f.dfg.inst_fixed_args(inst).len() > 0 {
// ... then force a minimum successor count of
// two, so the below algorithm cannot put
// edge-moves on the end of the block.
block_out_count[block] = std::cmp::max(2, block_out_count[block]);
}
}
}
// Here we define the implicit CLIF-plus-edges graph. There are // Here we define the implicit CLIF-plus-edges graph. There are
// conceptually two such graphs: the original, with every edge explicit, // conceptually two such graphs: the original, with every edge explicit,
// and the merged one, with blocks (represented by `LoweredBlock` // and the merged one, with blocks (represented by `LoweredBlock`