Skip to content

Commit

Permalink
Prevent miscompilation in trivial loop {}
Browse files Browse the repository at this point in the history
Ideally, we would want to handle a broader set of cases to fully fix the
underlying bug here. That is currently relatively expensive at compile and
runtime, so we don't do that for now.
  • Loading branch information
Mark-Simulacrum committed Oct 15, 2020
1 parent 7f58716 commit e2efec8
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 9 deletions.
10 changes: 5 additions & 5 deletions compiler/rustc_codegen_llvm/src/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -334,8 +334,8 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> {
self.call(expect, &[cond, self.const_bool(expected)], None)
}

fn sideeffect(&mut self) {
if self.tcx.sess.opts.debugging_opts.insert_sideeffect {
fn sideeffect(&mut self, unconditional: bool) {
if unconditional || self.tcx.sess.opts.debugging_opts.insert_sideeffect {
let fnname = self.get_intrinsic(&("llvm.sideeffect"));
self.call(fnname, &[], None);
}
Expand Down Expand Up @@ -390,7 +390,7 @@ fn codegen_msvc_try(
) {
let llfn = get_rust_try_fn(bx, &mut |mut bx| {
bx.set_personality_fn(bx.eh_personality());
bx.sideeffect();
bx.sideeffect(false);

let mut normal = bx.build_sibling_block("normal");
let mut catchswitch = bx.build_sibling_block("catchswitch");
Expand Down Expand Up @@ -553,7 +553,7 @@ fn codegen_gnu_try(
// call %catch_func(%data, %ptr)
// ret 1

bx.sideeffect();
bx.sideeffect(false);

let mut then = bx.build_sibling_block("then");
let mut catch = bx.build_sibling_block("catch");
Expand Down Expand Up @@ -615,7 +615,7 @@ fn codegen_emcc_try(
// call %catch_func(%data, %catch_data)
// ret 1

bx.sideeffect();
bx.sideeffect(false);

let mut then = bx.build_sibling_block("then");
let mut catch = bx.build_sibling_block("catch");
Expand Down
20 changes: 18 additions & 2 deletions compiler/rustc_codegen_ssa/src/mir/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> {
target <= self.bb
&& target.start_location().is_predecessor_of(self.bb.start_location(), mir)
}) {
bx.sideeffect();
bx.sideeffect(false);
}
}
}
Expand Down Expand Up @@ -964,7 +964,23 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
}

mir::TerminatorKind::Goto { target } => {
helper.maybe_sideeffect(self.mir, &mut bx, &[target]);
if bb == target {
// This is an unconditional branch back to this same basic
// block. That means we have something like a `loop {}`
// statement. Currently LLVM miscompiles this because it
// assumes forward progress. We want to prevent this in all
// cases, but that has a fairly high cost to compile times
// currently. Instead, try to handle this specific case
// which comes up commonly in practice (e.g., in embedded
// code).
//
// The `true` here means we insert side effects regardless
// of -Zinsert-sideeffect being passed on unconditional
// branching to the same basic block.
bx.sideeffect(true);
} else {
helper.maybe_sideeffect(self.mir, &mut bx, &[target]);
}
helper.funclet_br(self, &mut bx, target);
}

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_codegen_ssa/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
bx.set_personality_fn(cx.eh_personality());
}

bx.sideeffect();
bx.sideeffect(false);

let cleanup_kinds = analyze::cleanup_kinds(&mir);
// Allocate a `Block` for every basic block, except
Expand Down
4 changes: 3 additions & 1 deletion compiler/rustc_codegen_ssa/src/traits/intrinsic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ pub trait IntrinsicCallMethods<'tcx>: BackendTypes {
fn abort(&mut self);
fn assume(&mut self, val: Self::Value);
fn expect(&mut self, cond: Self::Value, expected: bool) -> Self::Value;
fn sideeffect(&mut self);
/// Normally, sideeffect is only emitted if -Zinsert-sideeffect is passed;
/// in some cases though we want to emit it regardless.
fn sideeffect(&mut self, unconditional: bool);
/// Trait method used to inject `va_start` on the "spoofed" `VaListImpl` in
/// Rust defined C-variadic functions.
fn va_start(&mut self, val: Self::Value) -> Self::Value;
Expand Down
15 changes: 15 additions & 0 deletions src/test/codegen/loop.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// compile-flags: -C opt-level=3

#![crate_type = "lib"]

// CHECK-LABEL: @check_loop
#[no_mangle]
pub fn check_loop() -> u8 {
// CHECK-NOT: unreachable
call_looper()
}

#[no_mangle]
fn call_looper() -> ! {
loop {}
}

0 comments on commit e2efec8

Please sign in to comment.