Auto merge of #109061 - saethlin:leak-backtraces, r=oli-obk

Add a backtrace to Allocation, display it in leak reports

This addresses #2813

Information like this from diagnostics is indispensable for diagnosing problems that are difficult to reproduce such as https://github.com/rust-lang/miri-test-libstd/actions/runs/4395316008/jobs/7697019211#step:4:770 (which has not been reproduced or diagnosed).

Author: bors

README.md

@@ -301,18 +301,22 @@ environment variable. We first document the most relevant and most commonly used
 * `-Zmiri-disable-isolation` disables host isolation.  As a consequence,
   the program has access to host resources such as environment variables, file
   systems, and randomness.
+* `-Zmiri-disable-leak-backtraces` disables backtraces reports for memory leaks. By default, a
+  backtrace is captured for every allocation when it is created, just in case it leaks. This incurs
+  some memory overhead to store data that is almost never used. This flag is implied by
+  `-Zmiri-ignore-leaks`.
+* `-Zmiri-env-forward=<var>` forwards the `var` environment variable to the interpreted program. Can
+  be used multiple times to forward several variables. Execution will still be deterministic if the
+  value of forwarded variables stays the same. Has no effect if `-Zmiri-disable-isolation` is set.
+* `-Zmiri-ignore-leaks` disables the memory leak checker, and also allows some
+  remaining threads to exist when the main thread exits.
 * `-Zmiri-isolation-error=<action>` configures Miri's response to operations
   requiring host access while isolation is enabled. `abort`, `hide`, `warn`,
   and `warn-nobacktrace` are the supported actions. The default is to `abort`,
   which halts the machine. Some (but not all) operations also support continuing
   execution with a "permission denied" error being returned to the program.
   `warn` prints a full backtrace when that happens; `warn-nobacktrace` is less
   verbose. `hide` hides the warning entirely.
-* `-Zmiri-env-forward=<var>` forwards the `var` environment variable to the interpreted program. Can
-  be used multiple times to forward several variables. Execution will still be deterministic if the
-  value of forwarded variables stays the same. Has no effect if `-Zmiri-disable-isolation` is set.
-* `-Zmiri-ignore-leaks` disables the memory leak checker, and also allows some
-  remaining threads to exist when the main thread exits.
 * `-Zmiri-num-cpus` states the number of available CPUs to be reported by miri. By default, the
   number of available CPUs is `1`. Note that this flag does not affect how miri handles threads in
   any way.

src/bin/miri.rs

@@ -359,6 +359,8 @@ fn main() {
                 isolation_enabled = Some(false);
             }
             miri_config.isolated_op = miri::IsolatedOp::Allow;
+        } else if arg == "-Zmiri-disable-leak-backtraces" {
+            miri_config.collect_leak_backtraces = false;
         } else if arg == "-Zmiri-disable-weak-memory-emulation" {
             miri_config.weak_memory_emulation = false;
         } else if arg == "-Zmiri-track-weak-memory-loads" {
@@ -385,6 +387,7 @@ fn main() {
             };
         } else if arg == "-Zmiri-ignore-leaks" {
             miri_config.ignore_leaks = true;
+            miri_config.collect_leak_backtraces = false;
         } else if arg == "-Zmiri-panic-on-unsupported" {
             miri_config.panic_on_unsupported = true;
         } else if arg == "-Zmiri-tag-raw-pointers" {

src/borrow_tracker/mod.rs

@@ -352,7 +352,7 @@ pub enum AllocState {
     TreeBorrows(Box<RefCell<tree_borrows::AllocState>>),
 }
 
-impl machine::AllocExtra {
+impl machine::AllocExtra<'_> {
     #[track_caller]
     pub fn borrow_tracker_sb(&self) -> &RefCell<stacked_borrows::AllocState> {
         match self.borrow_tracker {

src/diagnostics.rs

@@ -105,7 +105,7 @@ pub enum NonHaltingDiagnostic {
 }
 
 /// Level of Miri specific diagnostics
-enum DiagLevel {
+pub enum DiagLevel {
     Error,
     Warning,
     Note,
@@ -114,7 +114,7 @@ enum DiagLevel {
 /// Attempts to prune a stacktrace to omit the Rust runtime, and returns a bool indicating if any
 /// frames were pruned. If the stacktrace does not have any local frames, we conclude that it must
 /// be pointing to a problem in the Rust runtime itself, and do not prune it at all.
-fn prune_stacktrace<'tcx>(
+pub fn prune_stacktrace<'tcx>(
     mut stacktrace: Vec<FrameInfo<'tcx>>,
     machine: &MiriMachine<'_, 'tcx>,
 ) -> (Vec<FrameInfo<'tcx>>, bool) {
@@ -338,12 +338,45 @@ pub fn report_error<'tcx, 'mir>(
     None
 }
 
+pub fn report_leaks<'mir, 'tcx>(
+    ecx: &InterpCx<'mir, 'tcx, MiriMachine<'mir, 'tcx>>,
+    leaks: Vec<(AllocId, MemoryKind<MiriMemoryKind>, Allocation<Provenance, AllocExtra<'tcx>>)>,
+) {
+    let mut any_pruned = false;
+    for (id, kind, mut alloc) in leaks {
+        let Some(backtrace) = alloc.extra.backtrace.take() else {
+            continue;
+        };
+        let (backtrace, pruned) = prune_stacktrace(backtrace, &ecx.machine);
+        any_pruned |= pruned;
+        report_msg(
+            DiagLevel::Error,
+            &format!(
+                "memory leaked: {id:?} ({}, size: {:?}, align: {:?}), allocated here:",
+                kind,
+                alloc.size().bytes(),
+                alloc.align.bytes()
+            ),
+            vec![],
+            vec![],
+            vec![],
+            &backtrace,
+            &ecx.machine,
+        );
+    }
+    if any_pruned {
+        ecx.tcx.sess.diagnostic().note_without_error(
+            "some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace",
+        );
+    }
+}
+
 /// Report an error or note (depending on the `error` argument) with the given stacktrace.
 /// Also emits a full stacktrace of the interpreter stack.
 /// We want to present a multi-line span message for some errors. Diagnostics do not support this
 /// directly, so we pass the lines as a `Vec<String>` and display each line after the first with an
 /// additional `span_label` or `note` call.
-fn report_msg<'tcx>(
+pub fn report_msg<'tcx>(
     diag_level: DiagLevel,
     title: &str,
     span_msg: Vec<String>,

src/eval.rs

@@ -10,6 +10,7 @@ use std::thread;
 use log::info;
 
 use crate::borrow_tracker::RetagFields;
+use crate::diagnostics::report_leaks;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_hir::def::Namespace;
 use rustc_hir::def_id::DefId;
@@ -145,6 +146,8 @@ pub struct MiriConfig {
     pub num_cpus: u32,
     /// Requires Miri to emulate pages of a certain size
     pub page_size: Option<u64>,
+    /// Whether to collect a backtrace when each allocation is created, just in case it leaks.
+    pub collect_leak_backtraces: bool,
 }
 
 impl Default for MiriConfig {
@@ -179,6 +182,7 @@ impl Default for MiriConfig {
             gc_interval: 10_000,
             num_cpus: 1,
             page_size: None,
+            collect_leak_backtraces: true,
         }
     }
 }
@@ -457,10 +461,17 @@ pub fn eval_entry<'tcx>(
         }
         // Check for memory leaks.
         info!("Additonal static roots: {:?}", ecx.machine.static_roots);
-        let leaks = ecx.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");
+        let leaks = ecx.find_leaked_allocations(&ecx.machine.static_roots);
+        if !leaks.is_empty() {
+            report_leaks(&ecx, leaks);
+            let leak_message = "the evaluated program leaked memory, pass `-Zmiri-ignore-leaks` to disable this check";
+            if ecx.machine.collect_leak_backtraces {
+                // If we are collecting leak backtraces, each leak is a distinct error diagnostic.
+                tcx.sess.note_without_error(leak_message);
+            } else {
+                // If we do not have backtraces, we just report an error without any span.
+                tcx.sess.err(leak_message);
+            };
             // Ignore the provided return code - let the reported error
             // determine the return code.
             return None;

src/machine.rs

@@ -253,20 +253,25 @@ impl ProvenanceExtra {
 
 /// Extra per-allocation data
 #[derive(Debug, Clone)]
-pub struct AllocExtra {
+pub struct AllocExtra<'tcx> {
     /// Global state of the borrow tracker, if enabled.
     pub borrow_tracker: Option<borrow_tracker::AllocState>,
-    /// Data race detection via the use of a vector-clock,
-    ///  this is only added if it is enabled.
+    /// Data race detection via the use of a vector-clock.
+    /// This is only added if it is enabled.
     pub data_race: Option<data_race::AllocState>,
-    /// Weak memory emulation via the use of store buffers,
-    ///  this is only added if it is enabled.
+    /// Weak memory emulation via the use of store buffers.
+    /// This is only added if it is enabled.
     pub weak_memory: Option<weak_memory::AllocState>,
+    /// A backtrace to where this allocation was allocated.
+    /// As this is recorded for leak reports, it only exists
+    /// if this allocation is leakable. The backtrace is not
+    /// pruned yet; that should be done before printing it.
+    pub backtrace: Option<Vec<FrameInfo<'tcx>>>,
 }
 
-impl VisitTags for AllocExtra {
+impl VisitTags for AllocExtra<'_> {
     fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) {
-        let AllocExtra { borrow_tracker, data_race, weak_memory } = self;
+        let AllocExtra { borrow_tracker, data_race, weak_memory, backtrace: _ } = self;
 
         borrow_tracker.visit_tags(visit);
         data_race.visit_tags(visit);
@@ -467,12 +472,17 @@ pub struct MiriMachine<'mir, 'tcx> {
     pub(crate) gc_interval: u32,
     /// The number of blocks that passed since the last BorTag GC pass.
     pub(crate) since_gc: u32,
+
     /// The number of CPUs to be reported by miri.
     pub(crate) num_cpus: u32,
+
     /// Determines Miri's page size and associated values
     pub(crate) page_size: u64,
     pub(crate) stack_addr: u64,
     pub(crate) stack_size: u64,
+
+    /// Whether to collect a backtrace when each allocation is created, just in case it leaks.
+    pub(crate) collect_leak_backtraces: bool,
 }
 
 impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> {
@@ -581,6 +591,7 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> {
             page_size,
             stack_addr,
             stack_size,
+            collect_leak_backtraces: config.collect_leak_backtraces,
         }
     }
 
@@ -728,6 +739,7 @@ impl VisitTags for MiriMachine<'_, '_> {
             page_size: _,
             stack_addr: _,
             stack_size: _,
+            collect_leak_backtraces: _,
         } = self;
 
         threads.visit_tags(visit);
@@ -773,7 +785,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
     type ExtraFnVal = Dlsym;
 
     type FrameExtra = FrameExtra<'tcx>;
-    type AllocExtra = AllocExtra;
+    type AllocExtra = AllocExtra<'tcx>;
 
     type Provenance = Provenance;
     type ProvenanceExtra = ProvenanceExtra;
@@ -967,9 +979,24 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
             )
         });
         let buffer_alloc = ecx.machine.weak_memory.then(weak_memory::AllocState::new_allocation);
+
+        // If an allocation is leaked, we want to report a backtrace to indicate where it was
+        // allocated. We don't need to record a backtrace for allocations which are allowed to
+        // leak.
+        let backtrace = if kind.may_leak() || !ecx.machine.collect_leak_backtraces {
+            None
+        } else {
+            Some(ecx.generate_stacktrace())
+        };
+
         let alloc: Allocation<Provenance, Self::AllocExtra> = alloc.adjust_from_tcx(
             &ecx.tcx,
-            AllocExtra { borrow_tracker, data_race: race_alloc, weak_memory: buffer_alloc },
+            AllocExtra {
+                borrow_tracker,
+                data_race: race_alloc,
+                weak_memory: buffer_alloc,
+                backtrace,
+            },
             |ptr| ecx.global_base_pointer(ptr),
         )?;
         Ok(Cow::Owned(alloc))
@@ -1049,7 +1076,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
     fn before_memory_read(
         _tcx: TyCtxt<'tcx>,
         machine: &Self,
-        alloc_extra: &AllocExtra,
+        alloc_extra: &AllocExtra<'tcx>,
         (alloc_id, prov_extra): (AllocId, Self::ProvenanceExtra),
         range: AllocRange,
     ) -> InterpResult<'tcx> {
@@ -1069,7 +1096,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
     fn before_memory_write(
         _tcx: TyCtxt<'tcx>,
         machine: &mut Self,
-        alloc_extra: &mut AllocExtra,
+        alloc_extra: &mut AllocExtra<'tcx>,
         (alloc_id, prov_extra): (AllocId, Self::ProvenanceExtra),
         range: AllocRange,
     ) -> InterpResult<'tcx> {
@@ -1089,7 +1116,7 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for MiriMachine<'mir, 'tcx> {
     fn before_memory_deallocation(
         _tcx: TyCtxt<'tcx>,
         machine: &mut Self,
-        alloc_extra: &mut AllocExtra,
+        alloc_extra: &mut AllocExtra<'tcx>,
         (alloc_id, prove_extra): (AllocId, Self::ProvenanceExtra),
         range: AllocRange,
     ) -> InterpResult<'tcx> {

src/tag_gc.rs

@@ -125,7 +125,7 @@ impl VisitTags for Operand<Provenance> {
     }
 }
 
-impl VisitTags for Allocation<Provenance, AllocExtra> {
+impl VisitTags for Allocation<Provenance, AllocExtra<'_>> {
     fn visit_tags(&self, visit: &mut dyn FnMut(BorTag)) {
         for prov in self.provenance().provenances() {
             prov.visit_tags(visit);

tests/fail/memleak.rs

@@ -1,4 +1,4 @@
-//@error-pattern: the evaluated program leaked memory
+//@error-pattern: memory leaked
 //@normalize-stderr-test: ".*│.*" -> "$$stripped$$"
 
 fn main() {

tests/fail/memleak.stderr

@@ -1,10 +1,23 @@
-The following memory was leaked: ALLOC (Rust heap, size: 4, align: 4) {
-$stripped$
-}
+error: memory leaked: ALLOC (Rust heap, size: 4, align: 4), allocated here:
+  --> RUSTLIB/alloc/src/alloc.rs:LL:CC
+   |
+LL |     unsafe { __rust_alloc(layout.size(), layout.align()) }
+   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: inside `std::alloc::alloc` at RUSTLIB/alloc/src/alloc.rs:LL:CC
+   = note: inside `std::alloc::Global::alloc_impl` at RUSTLIB/alloc/src/alloc.rs:LL:CC
+   = note: inside `<std::alloc::Global as std::alloc::Allocator>::allocate` at RUSTLIB/alloc/src/alloc.rs:LL:CC
+   = note: inside `alloc::alloc::exchange_malloc` at RUSTLIB/alloc/src/alloc.rs:LL:CC
+   = note: inside `std::boxed::Box::<i32>::new` at RUSTLIB/alloc/src/boxed.rs:LL:CC
+note: inside `main`
+  --> $DIR/memleak.rs:LL:CC
+   |
+LL |     std::mem::forget(Box::new(42));
+   |                      ^^^^^^^^^^^^
 
-error: the evaluated program leaked memory
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
 
-note: pass `-Zmiri-ignore-leaks` to disable this check
+note: the evaluated program leaked memory, pass `-Zmiri-ignore-leaks` to disable this check
 
 error: aborting due to previous error
 

tests/fail/memleak_no_backtrace.rs

@@ -0,0 +1,7 @@
+//@compile-flags: -Zmiri-disable-leak-backtraces
+//@error-pattern: the evaluated program leaked memory
+//@normalize-stderr-test: ".*│.*" -> "$$stripped$$"
+
+fn main() {
+    std::mem::forget(Box::new(42));
+}

tests/fail/memleak_no_backtrace.stderr

@@ -0,0 +1,4 @@
+error: the evaluated program leaked memory, pass `-Zmiri-ignore-leaks` to disable this check
+
+error: aborting due to previous error
+

tests/fail/memleak_rc.32bit.stderr

@@ -1,10 +1,24 @@
-The following memory was leaked: ALLOC (Rust heap, size: 16, align: 4) {
-$stripped$
-}
+error: memory leaked: ALLOC (Rust heap, size: 16, align: 4), allocated here:
+  --> RUSTLIB/alloc/src/alloc.rs:LL:CC
+   |
+LL |     unsafe { __rust_alloc(layout.size(), layout.align()) }
+   |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: inside `std::alloc::alloc` at RUSTLIB/alloc/src/alloc.rs:LL:CC
+   = note: inside `std::alloc::Global::alloc_impl` at RUSTLIB/alloc/src/alloc.rs:LL:CC
+   = note: inside `<std::alloc::Global as std::alloc::Allocator>::allocate` at RUSTLIB/alloc/src/alloc.rs:LL:CC
+   = note: inside `alloc::alloc::exchange_malloc` at RUSTLIB/alloc/src/alloc.rs:LL:CC
+   = note: inside `std::boxed::Box::<std::rc::RcBox<std::cell::RefCell<std::option::Option<Dummy>>>>::new` at RUSTLIB/alloc/src/boxed.rs:LL:CC
+   = note: inside `std::rc::Rc::<std::cell::RefCell<std::option::Option<Dummy>>>::new` at RUSTLIB/alloc/src/rc.rs:LL:CC
+note: inside `main`
+  --> $DIR/memleak_rc.rs:LL:CC
+   |
+LL |     let x = Dummy(Rc::new(RefCell::new(None)));
+   |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-error: the evaluated program leaked memory
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
 
-note: pass `-Zmiri-ignore-leaks` to disable this check
+note: the evaluated program leaked memory, pass `-Zmiri-ignore-leaks` to disable this check
 
 error: aborting due to previous error