Skip to content

Commit

Permalink
Auto merge of #95161 - JakobDegen:fix-early-otherwise-branch, r=wesle…
Browse files Browse the repository at this point in the history
…ywiser

Disable almost certainly unsound early otherwise branch MIR opt

Originally thought this was just an MIR semantics issue, but it's not.

r? rust-lang/mir-opt
  • Loading branch information
bors committed Mar 22, 2022
2 parents 3c17c84 + a2f3a17 commit cf85310
Showing 1 changed file with 32 additions and 1 deletion.
33 changes: 32 additions & 1 deletion compiler/rustc_mir_transform/src/early_otherwise_branch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ pub struct EarlyOtherwiseBranch;

impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch {
fn is_enabled(&self, sess: &rustc_session::Session) -> bool {
sess.mir_opt_level() >= 2
sess.mir_opt_level() >= 3 && sess.opts.debugging_opts.unsound_mir_opts
}

fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
Expand Down Expand Up @@ -226,6 +226,37 @@ impl<'tcx> MirPass<'tcx> for EarlyOtherwiseBranch {

/// Returns true if computing the discriminant of `place` may be hoisted out of the branch
fn may_hoist<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>, place: Place<'tcx>) -> bool {
// FIXME(JakobDegen): This is unsound. Someone could write code like this:
// ```rust
// let Q = val;
// if discriminant(P) == otherwise {
// let ptr = &mut Q as *mut _ as *mut u8;
// unsafe { *ptr = 10; } // Any invalid value for the type
// }
//
// match P {
// A => match Q {
// A => {
// // code
// }
// _ => {
// // don't use Q
// }
// }
// _ => {
// // don't use Q
// }
// };
// ```
//
// Hoisting the `discriminant(Q)` out of the `A` arm causes us to compute the discriminant of an
// invalid value, which is UB.
//
// In order to fix this, we would either need to show that the discriminant computation of
// `place` is computed in all branches, including the `otherwise` branch, or we would need
// another analysis pass to determine that the place is fully initialized. It might even be best
// to have the hoisting be performed in a different pass and just do the CFG changing in this
// pass.
for (place, proj) in place.iter_projections() {
match proj {
// Dereferencing in the computation of `place` might cause issues from one of two
Expand Down

0 comments on commit cf85310

Please sign in to comment.