OpenE2K
/
rust
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Remove the -Zinsert-sideeffect 

This removes all of the code we had in place to work-around LLVM's
handling of forward progress. From this removal excluded is a workaround
where we'd insert a `sideeffect` into clearly infinite loops such as
`loop {}`. This code remains conditionally effective when the LLVM
version is earlier than 12.0, which fixed the forward progress related
miscompilations at their root.
This commit is contained in:
Simonas Kazlauskas 2021-03-08 01:59:10 +02:00
parent 861872bc45
commit 0517acd543
11 changed files with 52 additions and 87 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

14
compiler/rustc_codegen_llvm/src/intrinsic.rs
View File

@ -334,8 +334,11 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> {
self.call(expect, &[cond, self.const_bool(expected)], None)
}
fn sideeffect(&mut self, unconditional: bool) {
if unconditional || self.tcx.sess.opts.debugging_opts.insert_sideeffect {
fn sideeffect(&mut self) {
// This kind of check would make a ton of sense in the caller, but currently the only
// caller of this function is in `rustc_codegen_ssa`, which is agnostic to whether LLVM
// codegen backend being used, and so is unable to check the LLVM version.
if unsafe { llvm::LLVMRustVersionMajor() } < 12 {
let fnname = self.get_intrinsic(&("llvm.sideeffect"));
self.call(fnname, &[], None);
}
@ -390,7 +393,6 @@ fn codegen_msvc_try(
) {
let llfn = get_rust_try_fn(bx, &mut |mut bx| {
bx.set_personality_fn(bx.eh_personality());
bx.sideeffect(false);
let mut normal = bx.build_sibling_block("normal");
let mut catchswitch = bx.build_sibling_block("catchswitch");
@ -552,9 +554,6 @@ fn codegen_gnu_try(
// (%ptr, _) = landingpad
// call %catch_func(%data, %ptr)
// ret 1
bx.sideeffect(false);
let mut then = bx.build_sibling_block("then");
let mut catch = bx.build_sibling_block("catch");
@ -614,9 +613,6 @@ fn codegen_emcc_try(
// %catch_data[1] = %is_rust_panic
// call %catch_func(%data, %catch_data)
// ret 1
bx.sideeffect(false);
let mut then = bx.build_sibling_block("then");
let mut catch = bx.build_sibling_block("catch");

58
compiler/rustc_codegen_ssa/src/mir/block.rs
View File

@ -146,24 +146,6 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'tcx> {
}
}
}
// Generate sideeffect intrinsic if jumping to any of the targets can form
// a loop.
fn maybe_sideeffect<Bx: BuilderMethods<'a, 'tcx>>(
&self,
mir: &'tcx mir::Body<'tcx>,
bx: &mut Bx,
targets: &[mir::BasicBlock],
) {
if bx.tcx().sess.opts.debugging_opts.insert_sideeffect {
if targets.iter().any(|&target| {
target <= self.bb
&& target.start_location().is_predecessor_of(self.bb.start_location(), mir)
}) {
bx.sideeffect(false);
}
}
}
}
/// Codegen implementations for some terminator variants.
@ -198,8 +180,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let discr = self.codegen_operand(&mut bx, &discr);
// `switch_ty` is redundant, sanity-check that.
assert_eq!(discr.layout.ty, switch_ty);
helper.maybe_sideeffect(self.mir, &mut bx, targets.all_targets());
let mut target_iter = targets.iter();
if target_iter.len() == 1 {
// If there are two targets (one conditional, one fallback), emit br instead of switch
@ -308,7 +288,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
if let ty::InstanceDef::DropGlue(_, None) = drop_fn.def {
// we don't actually need to drop anything.
helper.maybe_sideeffect(self.mir, &mut bx, &[target]);
helper.funclet_br(self, &mut bx, target);
return;
}
@ -337,7 +316,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
}
_ => (bx.get_fn_addr(drop_fn), FnAbi::of_instance(&bx, drop_fn, &[])),
};
helper.maybe_sideeffect(self.mir, &mut bx, &[target]);
helper.do_call(
self,
&mut bx,
@ -379,7 +357,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
// Don't codegen the panic block if success if known.
if const_cond == Some(expected) {
helper.maybe_sideeffect(self.mir, &mut bx, &[target]);
helper.funclet_br(self, &mut bx, target);
return;
}
@ -390,7 +367,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
// Create the failure block and the conditional branch to it.
let lltarget = helper.llblock(self, target);
let panic_block = self.new_block("panic");
helper.maybe_sideeffect(self.mir, &mut bx, &[target]);
if expected {
bx.cond_br(cond, lltarget, panic_block.llbb());
} else {
@ -491,9 +467,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
let fn_abi = FnAbi::of_instance(bx, instance, &[]);
let llfn = bx.get_fn_addr(instance);
if let Some((_, target)) = destination.as_ref() {
helper.maybe_sideeffect(self.mir, bx, &[*target]);
}
// Codegen the actual panic invoke/call.
helper.do_call(
self,
@ -507,7 +480,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
} else {
// a NOP
let target = destination.as_ref().unwrap().1;
helper.maybe_sideeffect(self.mir, bx, &[target]);
helper.funclet_br(self, bx, target)
}
true
@ -551,7 +523,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
if let Some(ty::InstanceDef::DropGlue(_, None)) = def {
// Empty drop glue; a no-op.
let &(_, target) = destination.as_ref().unwrap();
helper.maybe_sideeffect(self.mir, &mut bx, &[target]);
helper.funclet_br(self, &mut bx, target);
return;
}
@ -586,7 +557,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
if let Some(destination_ref) = destination.as_ref() {
let &(dest, target) = destination_ref;
self.codegen_transmute(&mut bx, &args[0], dest);
helper.maybe_sideeffect(self.mir, &mut bx, &[target]);
helper.funclet_br(self, &mut bx, target);
} else {
// If we are trying to transmute to an uninhabited type,
@ -634,8 +604,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
location.val.store(&mut bx, tmp);
}
self.store_return(&mut bx, ret_dest, &fn_abi.ret, location.immediate());
helper.maybe_sideeffect(self.mir, &mut bx, &[*target]);
helper.funclet_br(self, &mut bx, *target);
}
return;
@ -700,7 +668,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
}
if let Some((_, target)) = *destination {
helper.maybe_sideeffect(self.mir, &mut bx, &[target]);
helper.funclet_br(self, &mut bx, target);
} else {
bx.unreachable();
@ -817,9 +784,6 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
_ => span_bug!(span, "no llfn for call"),
};
if let Some((_, target)) = destination.as_ref() {
helper.maybe_sideeffect(self.mir, &mut bx, &[*target]);
}
helper.do_call(
self,
&mut bx,
@ -969,22 +933,16 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
mir::TerminatorKind::Goto { 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).
// This is an unconditional branch back to this same basic block. That means we
// have something like a `loop {}` statement. LLVM versions before 12.0
// miscompile this because they assume forward progress. For older versions
// try to handle just 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]);
// NB: the `sideeffect` currently checks for the LLVM version used internally.
bx.sideeffect();
}
helper.funclet_br(self, &mut bx, target);
}

2
compiler/rustc_codegen_ssa/src/mir/mod.rs
View File

@ -149,8 +149,6 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
bx.set_personality_fn(cx.eh_personality());
}
bx.sideeffect(false);
let cleanup_kinds = analyze::cleanup_kinds(&mir);
// Allocate a `Block` for every basic block, except
// the start block, if nothing loops back to it.

7
compiler/rustc_codegen_ssa/src/traits/intrinsic.rs
View File

@ -20,9 +20,10 @@ 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;
/// 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);
/// Emits a forced side effect.
///
/// Currently has any effect only when LLVM versions prior to 12.0 are used as the backend.
fn sideeffect(&mut self);
/// 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;

1
compiler/rustc_interface/src/tests.rs
View File

@ -560,7 +560,6 @@ fn test_debugging_options_tracking_hash() {
tracked!(inline_mir, Some(true));
tracked!(inline_mir_threshold, Some(123));
tracked!(inline_mir_hint_threshold, Some(123));
tracked!(insert_sideeffect, true);
tracked!(instrument_coverage, true);
tracked!(instrument_mcount, true);
tracked!(link_only, true);

4
compiler/rustc_session/src/options.rs
View File

@ -967,10 +967,6 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
"control whether `#[inline]` functions are in all CGUs"),
input_stats: bool = (false, parse_bool, [UNTRACKED],
"gather statistics about the input (default: no)"),
insert_sideeffect: bool = (false, parse_bool, [TRACKED],
"fix undefined behavior when a thread doesn't eventually make progress \
(such as entering an empty infinite loop) by inserting llvm.sideeffect \
(default: no)"),
instrument_coverage: bool = (false, parse_bool, [TRACKED],
"instrument the generated code to support LLVM source-based code coverage \
reports (note, the compiler build config must include `profiler = true`, \

15
src/test/codegen/loop.rs
View File

@ -1,15 +0,0 @@
// 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 {}
}

3
src/test/codegen/non-terminate/infinite-loop-1.rs
View File

@ -1,4 +1,5 @@
// compile-flags: -C opt-level=3 -Z insert-sideeffect
// min-llvm-version: 12.0
// compile-flags: -C opt-level=3
#![crate_type = "lib"]

3
src/test/codegen/non-terminate/infinite-loop-2.rs
View File

@ -1,4 +1,5 @@
// compile-flags: -C opt-level=3 -Z insert-sideeffect
// min-llvm-version: 12.0
// compile-flags: -C opt-level=3
#![crate_type = "lib"]

3
src/test/codegen/non-terminate/infinite-recursion.rs
View File

@ -1,4 +1,5 @@
// compile-flags: -C opt-level=3 -Z insert-sideeffect
// min-llvm-version: 12.0
// compile-flags: -C opt-level=3
#![crate_type = "lib"]

29
src/test/codegen/non-terminate/nonempty-infinite-loop.rs Normal file
View File

@ -0,0 +1,29 @@
// min-llvm-version: 12.0
// compile-flags: -C opt-level=3
#![crate_type = "lib"]
// Verify that we don't miscompile this even if rustc didn't apply the trivial loop detection to
// insert the sideeffect intrinsic.
fn infinite_loop() -> u8 {
let mut x = 0;
// CHECK-NOT: sideeffect
loop {
if x == 42 {
x = 0;
} else {
x = 42;
}
}
}
// CHECK-LABEL: @test
#[no_mangle]
fn test() -> u8 {
// CHECK-NOT: unreachable
// CHECK: br label %{{.+}}
// CHECK-NOT: unreachable
let x = infinite_loop();
x
}