Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a backtrace to Allocation, display it in leak reports #109061

Merged
merged 3 commits into from
Apr 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions compiler/rustc_const_eval/src/interpret/eval_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,11 +132,10 @@ pub struct Frame<'mir, 'tcx, Prov: Provenance = AllocId, Extra = ()> {
}

/// What we store about a frame in an interpreter backtrace.
#[derive(Debug)]
#[derive(Clone, Debug)]
pub struct FrameInfo<'tcx> {
pub instance: ty::Instance<'tcx>,
pub span: Span,
pub lint_root: Option<hir::HirId>,
}

#[derive(Clone, Copy, Eq, PartialEq, Debug)] // Miri debug-prints these
Expand Down Expand Up @@ -947,10 +946,8 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// This deliberately does *not* honor `requires_caller_location` since it is used for much
// more than just panics.
for frame in stack.iter().rev() {
let lint_root = frame.lint_root();
let span = frame.current_span();

frames.push(FrameInfo { span, instance: frame.instance, lint_root });
frames.push(FrameInfo { span, instance: frame.instance });
}
trace!("generate stacktrace: {:#?}", frames);
frames
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/interpret/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ pub trait Machine<'mir, 'tcx>: Sized {
type FrameExtra;

/// Extra data stored in every allocation.
type AllocExtra: Debug + Clone + 'static;
type AllocExtra: Debug + Clone + 'tcx;

/// Type for the bytes of the allocation.
type Bytes: AllocBytes + 'static;
Expand Down
27 changes: 15 additions & 12 deletions compiler/rustc_const_eval/src/interpret/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
self.allocate_raw_ptr(alloc, kind)
}

/// This can fail only of `alloc` contains provenance.
/// This can fail only if `alloc` contains provenance.
pub fn allocate_raw_ptr(
&mut self,
alloc: Allocation,
Expand Down Expand Up @@ -807,9 +807,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
DumpAllocs { ecx: self, allocs }
}

/// Print leaked memory. Allocations reachable from `static_roots` or a `Global` allocation
/// are not considered leaked. Leaks whose kind `may_leak()` returns true are not reported.
pub fn leak_report(&self, static_roots: &[AllocId]) -> usize {
/// Find leaked allocations. Allocations reachable from `static_roots` or a `Global` allocation
/// are not considered leaked, as well as leaks whose kind's `may_leak()` returns true.
pub fn find_leaked_allocations(
&self,
static_roots: &[AllocId],
) -> Vec<(AllocId, MemoryKind<M::MemoryKind>, Allocation<M::Provenance, M::AllocExtra, M::Bytes>)>
{
// Collect the set of allocations that are *reachable* from `Global` allocations.
let reachable = {
let mut reachable = FxHashSet::default();
Expand All @@ -833,14 +837,13 @@ impl<'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
};

// All allocations that are *not* `reachable` and *not* `may_leak` are considered leaking.
let leaks: Vec<_> = self.memory.alloc_map.filter_map_collect(|&id, &(kind, _)| {
if kind.may_leak() || reachable.contains(&id) { None } else { Some(id) }
});
let n = leaks.len();
if n > 0 {
eprintln!("The following memory was leaked: {:?}", self.dump_allocs(leaks));
}
n
self.memory.alloc_map.filter_map_collect(|id, (kind, alloc)| {
if kind.may_leak() || reachable.contains(id) {
None
} else {
Some((*id, *kind, alloc.clone()))
}
})
}
}

Expand Down
14 changes: 9 additions & 5 deletions src/tools/miri/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
3 changes: 3 additions & 0 deletions src/tools/miri/src/bin/miri.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Expand All @@ -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" {
Expand Down
2 changes: 1 addition & 1 deletion src/tools/miri/src/borrow_tracker/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
39 changes: 36 additions & 3 deletions src/tools/miri/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ pub enum NonHaltingDiagnostic {
}

/// Level of Miri specific diagnostics
enum DiagLevel {
pub enum DiagLevel {
Error,
Warning,
Note,
Expand All @@ -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) {
Expand Down Expand Up @@ -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>,
Expand Down
19 changes: 15 additions & 4 deletions src/tools/miri/src/eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -179,6 +182,7 @@ impl Default for MiriConfig {
gc_interval: 10_000,
num_cpus: 1,
page_size: None,
collect_leak_backtraces: true,
}
}
}
Expand Down Expand Up @@ -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;
Expand Down
51 changes: 39 additions & 12 deletions src/tools/miri/src/machine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -581,6 +591,7 @@ impl<'mir, 'tcx> MiriMachine<'mir, 'tcx> {
page_size,
stack_addr,
stack_size,
collect_leak_backtraces: config.collect_leak_backtraces,
}
}

Expand Down Expand Up @@ -728,6 +739,7 @@ impl VisitTags for MiriMachine<'_, '_> {
page_size: _,
stack_addr: _,
stack_size: _,
collect_leak_backtraces: _,
} = self;

threads.visit_tags(visit);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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())
};
saethlin marked this conversation as resolved.
Show resolved Hide resolved

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))
Expand Down Expand Up @@ -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> {
Expand All @@ -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> {
Expand All @@ -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> {
Expand Down
2 changes: 1 addition & 1 deletion src/tools/miri/src/tag_gc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/tools/miri/tests/fail/memleak.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
//@error-pattern: the evaluated program leaked memory
//@error-pattern: memory leaked
//@normalize-stderr-test: ".*│.*" -> "$$stripped$$"

fn main() {
Expand Down
Loading