Auto merge of #1858 - RalfJung:thread-leaks, r=oli-obk

also ignore 'thread leaks' with -Zmiri-ignore-leaks

This is a step towards #1371. The remaining hard part would be supporting checking for memory leaks when there are threads still running. For now we elegantly avoid this problem by using the same flag to control both of these checks. :)

README.md

@@ -230,7 +230,8 @@ environment variable:
   the host so that it cannot be accessed by the program.  Can be used multiple
   times to exclude several variables.  On Windows, the `TERM` environment
   variable is excluded by default.
-* `-Zmiri-ignore-leaks` disables the memory leak checker.
+* `-Zmiri-ignore-leaks` disables the memory leak checker, and also allows some
+  remaining threads to exist when the main thread exits.
 * `-Zmiri-measureme=<name>` enables `measureme` profiling for the interpreted program.
    This can be used to find which parts of your program are executing slowly under Miri.
    The profile is written out to a file with the prefix `<name>`, and can be processed

src/eval.rs

@@ -306,10 +306,20 @@ pub fn eval_main<'tcx>(tcx: TyCtxt<'tcx>, main_id: DefId, config: MiriConfig) ->
     match res {
         Ok(return_code) => {
             if !ignore_leaks {
+                // Check for thread leaks.
+                if !ecx.have_all_terminated() {
+                    tcx.sess.err(
+                        "the main thread terminated without waiting for all remaining threads",
+                    );
+                    tcx.sess.note_without_error("pass `-Zmiri-ignore-leaks` to disable this check");
+                    return None;
+                }
+                // Check for memory leaks.
                 info!("Additonal static roots: {:?}", ecx.machine.static_roots);
                 let leaks = ecx.memory.leak_report(&ecx.machine.static_roots);
                 if leaks != 0 {
                     tcx.sess.err("the evaluated program leaked memory");
+                    tcx.sess.note_without_error("pass `-Zmiri-ignore-leaks` to disable this check");
                     // Ignore the provided return code - let the reported error
                     // determine the return code.
                     return None;

src/thread.rs

@@ -302,6 +302,11 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> {
         self.threads[thread_id].state == ThreadState::Terminated
     }
 
+    /// Have all threads terminated?
+    fn have_all_terminated(&self) -> bool {
+        self.threads.iter().all(|thread| thread.state == ThreadState::Terminated)
+    }
+
     /// Enable the thread for execution. The thread must be terminated.
     fn enable_thread(&mut self, thread_id: ThreadId) {
         assert!(self.has_terminated(thread_id));
@@ -491,15 +496,7 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> {
         // If we get here again and the thread is *still* terminated, there are no more dtors to run.
         if self.threads[MAIN_THREAD].state == ThreadState::Terminated {
             // The main thread terminated; stop the program.
-            if self.threads.iter().any(|thread| thread.state != ThreadState::Terminated) {
-                // FIXME: This check should be either configurable or just emit
-                // a warning. For example, it seems normal for a program to
-                // terminate without waiting for its detached threads to
-                // terminate. However, this case is not trivial to support
-                // because we also probably do not want to consider the memory
-                // owned by these threads as leaked.
-                throw_unsup_format!("the main thread terminated without waiting for other threads");
-            }
+            // We do *not* run TLS dtors of remaining threads, which seems to match rustc behavior.
             return Ok(SchedulingAction::Stop);
         }
         // This thread and the program can keep going.
@@ -645,6 +642,12 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         this.machine.threads.has_terminated(thread_id)
     }
 
+    #[inline]
+    fn have_all_terminated(&self) -> bool {
+        let this = self.eval_context_ref();
+        this.machine.threads.have_all_terminated()
+    }
+
     #[inline]
     fn enable_thread(&mut self, thread_id: ThreadId) {
         let this = self.eval_context_mut();

tests/compile-fail/concurrency/libc_pthread_create_main_terminate.rs

@@ -1,5 +1,5 @@
 // ignore-windows: No libc on Windows
-// error-pattern: unsupported operation: the main thread terminated without waiting for other threads
+// error-pattern: the main thread terminated without waiting for all remaining threads
 
 // Check that we terminate the program when the main thread terminates.
 
@@ -10,7 +10,7 @@ extern crate libc;
 use std::{mem, ptr};
 
 extern "C" fn thread_start(_null: *mut libc::c_void) -> *mut libc::c_void {
-    ptr::null_mut()
+    loop {}
 }
 
 fn main() {

tests/run-pass/threadleak_ignored.rs

@@ -0,0 +1,37 @@
+// ignore-windows: Concurrency on Windows is not supported yet.
+// compile-flags: -Zmiri-ignore-leaks
+
+//! Test that leaking threads works, and that their destructors are not executed.
+
+use std::cell::RefCell;
+
+struct LoudDrop(i32);
+impl Drop for LoudDrop {
+    fn drop(&mut self) {
+        eprintln!("Dropping {}", self.0);
+    }
+}
+
+thread_local! {
+    static X: RefCell<Option<LoudDrop>> = RefCell::new(None);
+}
+
+fn main() {
+    X.with(|x| *x.borrow_mut() = Some(LoudDrop(0)));
+
+    // Set up a channel so that we can learn when the other thread initialized `X`
+    // (so that we are sure there is something to drop).
+    let (send, recv) = std::sync::mpsc::channel::<()>();
+
+    let _detached = std::thread::spawn(move || {
+        X.with(|x| *x.borrow_mut() = Some(LoudDrop(1)));
+        send.send(()).unwrap();
+        std::thread::yield_now();
+        loop {}
+    });
+
+    std::thread::yield_now();
+
+    // Wait until child thread has initialized its `X`.
+    let () = recv.recv().unwrap();
+}

tests/run-pass/threadleak_ignored.stderr

@@ -0,0 +1,3 @@
+warning: thread support is experimental and incomplete: weak memory effects are not emulated.
+
+Dropping 0