Add llvm.sideeffect to potential infinite loops and recursions

LLVM assumes that a thread will eventually cause side effect. This is not true in Rust if a loop or recursion does nothing in its body, causing undefined behavior even in common cases like `loop {}`. Inserting llvm.sideeffect fixes the undefined behavior. As a micro-optimization, only insert llvm.sideeffect when jumping back in blocks or calling a function. A patch for LLVM is expected to allow empty non-terminate code by default and fix this issue from LLVM side. rust-lang#28728
sfanxiang · Mar 31, 2019 · 0ea0e06 · 0ea0e06
1 parent 2671421
commit 0ea0e06
Show file tree

Hide file tree

Showing 15 changed files with 109 additions and 12 deletions.
diff --git a/src/librustc_codegen_llvm/context.rs b/src/librustc_codegen_llvm/context.rs
@@ -536,6 +536,7 @@ impl CodegenCx<'b, 'tcx> {
         ifn!("llvm.trap", fn() -> void);
         ifn!("llvm.debugtrap", fn() -> void);
         ifn!("llvm.frameaddress", fn(t_i32) -> i8p);
+        ifn!("llvm.sideeffect", fn() -> void);
 
         ifn!("llvm.powi.f32", fn(t_f32, t_i32) -> t_f32);
         ifn!("llvm.powi.v2f32", fn(t_v2f32, t_i32) -> t_v2f32);

diff --git a/src/librustc_codegen_llvm/intrinsic.rs b/src/librustc_codegen_llvm/intrinsic.rs
@@ -121,6 +121,7 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> {
                 self.call(expect, &[args[0].immediate(), self.const_bool(false)], None)
             }
             "try" => {
+                self.sideeffect();
                 try_intrinsic(self,
                               args[0].immediate(),
                               args[1].immediate(),
@@ -718,6 +719,11 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> {
         self.call(expect, &[cond, self.const_bool(expected)], None)
     }
 
+    fn sideeffect(&mut self) {
+        let fnname = self.get_intrinsic(&("llvm.sideeffect"));
+        self.call(fnname, &[], None);
+    }
+
     fn va_start(&mut self, list: &'ll Value) -> &'ll Value {
         let target = &self.cx.tcx.sess.target.target;
         let arch = &target.arch;

diff --git a/src/librustc_codegen_ssa/mir/block.rs b/src/librustc_codegen_ssa/mir/block.rs
@@ -149,6 +149,18 @@ impl<'a, 'tcx> TerminatorCodegenHelper<'a, 'tcx> {
             }
         }
     }
+
+    // Generate sideeffect intrinsic if any of the targets' index is not
+    // greater than that of the current block.
+    fn maybe_sideeffect<'b, 'tcx2: 'b, Bx: BuilderMethods<'b, 'tcx2>>(
+        &self,
+        bx: &mut Bx,
+        targets: &[mir::BasicBlock],
+    ) {
+        if targets.iter().any(|target| target <= self.bb) {
+            bx.sideeffect();
+        }
+    }
 }
 
 /// Codegen implementations for some terminator variants.
@@ -197,6 +209,7 @@ impl<'a, 'tcx: 'a, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             let lltrue = helper.llblock(self, targets[0]);
             let llfalse = helper.llblock(self, targets[1]);
             if switch_ty == bx.tcx().types.bool {
+                helper.maybe_sideeffect(&mut bx, targets.as_slice());
                 // Don't generate trivial icmps when switching on bool
                 if let [0] = values[..] {
                     bx.cond_br(discr.immediate(), llfalse, lltrue);
@@ -210,9 +223,11 @@ impl<'a, 'tcx: 'a, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                 );
                 let llval = bx.const_uint_big(switch_llty, values[0]);
                 let cmp = bx.icmp(IntPredicate::IntEQ, discr.immediate(), llval);
+                helper.maybe_sideeffect(&mut bx, targets.as_slice());
                 bx.cond_br(cmp, lltrue, llfalse);
             }
         } else {
+            helper.maybe_sideeffect(&mut bx, targets.as_slice());
             let (otherwise, targets) = targets.split_last().unwrap();
             bx.switch(
                 discr.immediate(),
@@ -307,6 +322,7 @@ impl<'a, 'tcx: 'a, 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(&mut bx, &[target]);
             helper.funclet_br(self, &mut bx, target);
             return
         }
@@ -337,6 +353,7 @@ impl<'a, 'tcx: 'a, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                  bx.fn_type_of_instance(&drop_fn))
             }
         };
+        bx.sideeffect();
         helper.do_call(self, &mut bx, fn_ty, drop_fn, args,
                        Some((ReturnDest::Nothing, target)),
                        unwind);
@@ -372,6 +389,7 @@ impl<'a, 'tcx: 'a, 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(&mut bx, &[target]);
             helper.funclet_br(self, &mut bx, target);
             return;
         }
@@ -382,6 +400,7 @@ impl<'a, 'tcx: 'a, 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(&mut bx, &[target]);
         if expected {
             bx.cond_br(cond, lltarget, panic_block.llbb());
         } else {
@@ -435,6 +454,7 @@ impl<'a, 'tcx: 'a, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
         let fn_ty = bx.fn_type_of_instance(&instance);
         let llfn = bx.get_fn(instance);
 
+        bx.sideeffect();
         // Codegen the actual panic invoke/call.
         helper.do_call(self, &mut bx, fn_ty, llfn, &args, None, cleanup);
     }
@@ -486,6 +506,7 @@ impl<'a, 'tcx: 'a, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             if let Some(destination_ref) = destination.as_ref() {
                 let &(ref dest, target) = destination_ref;
                 self.codegen_transmute(&mut bx, &args[0], dest);
+                helper.maybe_sideeffect(&mut bx, &[target]);
                 helper.funclet_br(self, &mut bx, target);
             } else {
                 // If we are trying to transmute to an uninhabited type,
@@ -516,6 +537,7 @@ impl<'a, 'tcx: 'a, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             Some(ty::InstanceDef::DropGlue(_, None)) => {
                 // Empty drop glue; a no-op.
                 let &(_, target) = destination.as_ref().unwrap();
+                helper.maybe_sideeffect(&mut bx, &[target]);
                 helper.funclet_br(self, &mut bx, target);
                 return;
             }
@@ -552,6 +574,7 @@ impl<'a, 'tcx: 'a, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                 let fn_ty = bx.fn_type_of_instance(&instance);
                 let llfn = bx.get_fn(instance);
 
+                bx.sideeffect();
                 // Codegen the actual panic invoke/call.
                 helper.do_call(
                     self,
@@ -564,7 +587,9 @@ impl<'a, 'tcx: 'a, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                 );
             } else {
                 // a NOP
-                helper.funclet_br(self, &mut bx, destination.as_ref().unwrap().1)
+                let target = destination.as_ref().unwrap().1;
+                helper.maybe_sideeffect(&mut bx, &[target]);
+                helper.funclet_br(self, &mut bx, target);
             }
             return;
         }
@@ -668,6 +693,7 @@ impl<'a, 'tcx: 'a, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             }
 
             if let Some((_, target)) = *destination {
+                helper.maybe_sideeffect(&mut bx, &[target]);
                 helper.funclet_br(self, &mut bx, target);
             } else {
                 bx.unreachable();
@@ -790,6 +816,7 @@ impl<'a, 'tcx: 'a, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             _ => span_bug!(span, "no llfn for call"),
         };
 
+        bx.sideeffect();
         helper.do_call(self, &mut bx, fn_ty, fn_ptr, &llargs,
                        destination.as_ref().map(|&(_, target)| (ret_dest, target)),
                        cleanup);
@@ -839,6 +866,7 @@ impl<'a, 'tcx: 'a, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
             }
 
             mir::TerminatorKind::Goto { target } => {
+                helper.maybe_sideeffect(&mut bx, &[target]);
                 helper.funclet_br(self, &mut bx, target);
             }
 

diff --git a/src/librustc_codegen_ssa/mir/rvalue.rs b/src/librustc_codegen_ssa/mir/rvalue.rs
@@ -484,6 +484,7 @@ impl<'a, 'tcx: 'a, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
                 };
                 let instance = ty::Instance::mono(bx.tcx(), def_id);
                 let r = bx.cx().get_fn(instance);
+                bx.sideeffect();
                 let call = bx.call(r, &[llsize, llalign], None);
                 let val = bx.pointercast(call, llty_ptr);
 

diff --git a/src/librustc_codegen_ssa/traits/intrinsic.rs b/src/librustc_codegen_ssa/traits/intrinsic.rs
@@ -20,6 +20,7 @@ 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);
     /// Trait method used to inject `va_start` on the "spoofed" `VaList` in
     /// Rust defined C-variadic functions.
     fn va_start(&mut self, val: Self::Value) -> Self::Value;

diff --git a/src/test/codegen/alloc-optimisation.rs b/src/test/codegen/alloc-optimisation.rs
@@ -7,7 +7,8 @@
 pub fn alloc_test(data: u32) {
     // CHECK-LABEL: @alloc_test
     // CHECK-NEXT: start:
-    // CHECK-NEXT: ret void
+    // CHECK-NOT: alloc
+    // CHECK: ret void
     let x = Box::new(data);
     drop(x);
 }
diff --git a/src/test/codegen/dealloc-no-unwind.rs b/src/test/codegen/dealloc-no-unwind.rs
@@ -17,7 +17,7 @@ impl Drop for A {
 pub fn a(a: Box<i32>) {
     // CHECK-LABEL: define void @a
     // CHECK: call void @__rust_dealloc
-    // CHECK-NEXT: call void @foo
+    // CHECK: call void @foo
     let _a = A;
     drop(a);
 }
diff --git a/src/test/codegen/issue-34947-pow-i32.rs b/src/test/codegen/issue-34947-pow-i32.rs
@@ -6,8 +6,11 @@
 #[no_mangle]
 pub fn issue_34947(x: i32) -> i32 {
     // CHECK: mul
-    // CHECK-NEXT: mul
-    // CHECK-NEXT: mul
-    // CHECK-NEXT: ret
+    // CHECK-NOT: br label
+    // CHECK: mul
+    // CHECK-NOT: br label
+    // CHECK: mul
+    // CHECK-NOT: br label
+    // CHECK: ret
     x.pow(5)
 }
diff --git a/src/test/codegen/issue-45222.rs b/src/test/codegen/issue-45222.rs
@@ -1,3 +1,5 @@
+// ignore-test LLVM can't prove that these loops terminate.
+
 // compile-flags: -O
 
 #![crate_type = "lib"]

diff --git a/src/test/codegen/naked-functions.rs b/src/test/codegen/naked-functions.rs
@@ -53,27 +53,27 @@ pub fn naked_with_args_and_return(a: isize) -> isize {
 #[naked]
 pub fn naked_recursive() {
     // CHECK-NEXT: {{.+}}:
-    // CHECK-NEXT: call void @naked_empty()
+    // CHECK: call void @naked_empty()
 
     // FIXME(#39685) Avoid one block per call.
     // CHECK-NEXT: br label %bb1
     // CHECK: bb1:
 
     naked_empty();
 
-    // CHECK-NEXT: %{{[0-9]+}} = call i{{[0-9]+}} @naked_with_return()
+    // CHECK: %{{[0-9]+}} = call i{{[0-9]+}} @naked_with_return()
 
     // FIXME(#39685) Avoid one block per call.
     // CHECK-NEXT: br label %bb2
     // CHECK: bb2:
 
-    // CHECK-NEXT: %{{[0-9]+}} = call i{{[0-9]+}} @naked_with_args_and_return(i{{[0-9]+}} %{{[0-9]+}})
+    // CHECK: %{{[0-9]+}} = call i{{[0-9]+}} @naked_with_args_and_return(i{{[0-9]+}} %{{[0-9]+}})
 
     // FIXME(#39685) Avoid one block per call.
     // CHECK-NEXT: br label %bb3
     // CHECK: bb3:
 
-    // CHECK-NEXT: call void @naked_with_args(i{{[0-9]+}} %{{[0-9]+}})
+    // CHECK: call void @naked_with_args(i{{[0-9]+}} %{{[0-9]+}})
 
     // FIXME(#39685) Avoid one block per call.
     // CHECK-NEXT: br label %bb4

diff --git a/src/test/codegen/non-terminate/infinite-loop-1.rs b/src/test/codegen/non-terminate/infinite-loop-1.rs
@@ -0,0 +1,17 @@
+// compile-flags: -C opt-level=3
+
+#![crate_type = "lib"]
+
+fn infinite_loop() -> u8 {
+    loop {}
+}
+
+// CHECK-LABEL: @test
+#[no_mangle]
+fn test() -> u8 {
+    // CHECK-NOT: unreachable
+    // CHECK: br label %{{.+}}
+    // CHECK-NOT: unreachable
+    let x = infinite_loop();
+    x
+}
diff --git a/src/test/codegen/non-terminate/infinite-loop-2.rs b/src/test/codegen/non-terminate/infinite-loop-2.rs
@@ -0,0 +1,19 @@
+// compile-flags: -C opt-level=3
+
+#![crate_type = "lib"]
+
+fn infinite_loop() -> u8 {
+    let i = 2;
+    while i > 1 {}
+    1
+}
+
+// CHECK-LABEL: @test
+#[no_mangle]
+fn test() -> u8 {
+    // CHECK-NOT: unreachable
+    // CHECK: br label %{{.+}}
+    // CHECK-NOT: unreachable
+    let x = infinite_loop();
+    x
+}
diff --git a/src/test/codegen/non-terminate/infinite-recursion.rs b/src/test/codegen/non-terminate/infinite-recursion.rs
@@ -0,0 +1,14 @@
+// compile-flags: -C opt-level=3
+
+#![crate_type = "lib"]
+
+#![allow(unconditional_recursion)]
+
+// CHECK-LABEL: @infinite_recursion
+#[no_mangle]
+fn infinite_recursion() -> u8 {
+    // CHECK-NOT: ret i8 undef
+    // CHECK: br label %{{.+}}
+    // CHECK-NOT: ret i8 undef
+    infinite_recursion()
+}
diff --git a/src/test/codegen/repeat-trusted-len.rs b/src/test/codegen/repeat-trusted-len.rs
@@ -14,6 +14,10 @@ pub fn helper(_: usize) {
 // CHECK-LABEL: @repeat_take_collect
 #[no_mangle]
 pub fn repeat_take_collect() -> Vec<u8> {
-// CHECK: call void @llvm.memset.p0i8.[[USIZE]](i8* {{(nonnull )?}}align 1 %{{[0-9]+}}, i8 42, [[USIZE]] 100000, i1 false)
+// FIXME: At the time of writing LLVM transforms this loop into a single
+// `store` and then a `memset` with size = 99999. The correct check should be:
+//        call void @llvm.memset.p0i8.[[USIZE]](i8* {{(nonnull )?}}align 1 %{{[a-z0-9.]+}}, i8 42, [[USIZE]] 100000, i1 false)
+
+// CHECK: call void @llvm.memset.p0i8.[[USIZE]](i8* {{(nonnull )?}}align 1 %{{[a-z0-9.]+}}, i8 42, [[USIZE]] 99999, i1 false)
     iter::repeat(42).take(100000).collect()
 }
diff --git a/src/test/run-make-fulldeps/inline-always-many-cgu/Makefile b/src/test/run-make-fulldeps/inline-always-many-cgu/Makefile
@@ -2,7 +2,7 @@
 
 all:
 	$(RUSTC) foo.rs --emit llvm-ir -C codegen-units=2
-	if cat $(TMPDIR)/*.ll | $(CGREP) -e '\bcall\b'; then \
+	if cat $(TMPDIR)/*.ll | grep -v 'call void @llvm.sideeffect()' | $(CGREP) -e '\bcall\b'; then \
 		echo "found call instruction when one wasn't expected"; \
 		exit 1; \
 	fi