From 8f93e6ed10e73750770823fe6d7a30304090f2d6 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 14 Nov 2025 22:42:14 +0100 Subject: [PATCH] refactor and unify top-level error handling a bit --- src/bin/miri.rs | 121 ++++++++++++++++---------------- src/concurrency/genmc/config.rs | 9 +-- src/concurrency/genmc/dummy.rs | 13 ++-- src/concurrency/genmc/run.rs | 28 +++++--- src/diagnostics.rs | 23 +++--- src/eval.rs | 72 +++++++++++-------- src/lib.rs | 2 +- tests/fail/deny_lint.rs | 2 - tests/fail/deny_lint.stderr | 4 +- 9 files changed, 143 insertions(+), 131 deletions(-) diff --git a/src/bin/miri.rs b/src/bin/miri.rs index 920fc29481..ed6de7a120 100644 --- a/src/bin/miri.rs +++ b/src/bin/miri.rs @@ -23,18 +23,18 @@ extern crate rustc_span; mod log; use std::env; -use std::num::NonZero; +use std::num::{NonZero, NonZeroI32}; use std::ops::Range; use std::rc::Rc; use std::str::FromStr; -use std::sync::atomic::{AtomicI32, AtomicU32, Ordering}; +use std::sync::atomic::{AtomicU32, Ordering}; use miri::{ BacktraceStyle, BorrowTrackerMethod, GenmcConfig, GenmcCtx, MiriConfig, MiriEntryFnType, ProvenanceMode, RetagFields, TreeBorrowsParams, ValidationMode, run_genmc_mode, }; use rustc_abi::ExternAbi; -use rustc_data_structures::sync; +use rustc_data_structures::sync::{self, DynSync}; use rustc_driver::Compilation; use rustc_hir::def_id::LOCAL_CRATE; use rustc_hir::{self as hir, Node}; @@ -120,15 +120,47 @@ fn entry_fn(tcx: TyCtxt<'_>) -> (DefId, MiriEntryFnType) { } } +fn run_many_seeds( + many_seeds: ManySeedsConfig, + eval_entry_once: impl Fn(u64) -> Result<(), NonZeroI32> + DynSync, +) -> Result<(), NonZeroI32> { + let exit_code = + sync::IntoDynSyncSend(AtomicU32::new(rustc_driver::EXIT_SUCCESS.cast_unsigned())); + let num_failed = sync::IntoDynSyncSend(AtomicU32::new(0)); + sync::par_for_each_in(many_seeds.seeds.clone(), |&seed| { + if let Err(return_code) = eval_entry_once(seed.into()) { + eprintln!("FAILING SEED: {seed}"); + if !many_seeds.keep_going { + // `abort_if_errors` would unwind but would not actually stop miri, since + // `par_for_each` waits for the rest of the threads to finish. + exit(return_code.get()); + } + // Preserve the "maximum" return code (when interpreted as `u32`), to make + // the result order-independent and to make it 0 only if all executions were 0. + exit_code.fetch_max(return_code.get().cast_unsigned(), Ordering::Relaxed); + num_failed.fetch_add(1, Ordering::Relaxed); + } + }); + let num_failed = num_failed.0.into_inner(); + let exit_code = exit_code.0.into_inner().cast_signed(); + if num_failed > 0 { + eprintln!("{num_failed}/{total} SEEDS FAILED", total = many_seeds.seeds.count()); + Err(NonZeroI32::new(exit_code).unwrap()) + } else { + assert!(exit_code == 0); + Ok(()) + } +} + impl rustc_driver::Callbacks for MiriCompilerCalls { fn after_analysis<'tcx>( &mut self, _: &rustc_interface::interface::Compiler, tcx: TyCtxt<'tcx>, ) -> Compilation { - if tcx.sess.dcx().has_errors_or_delayed_bugs().is_some() { - tcx.dcx().fatal("miri cannot be run on programs that fail compilation"); - } + tcx.dcx().abort_if_errors(); + tcx.dcx().flush_delayed(); + if !tcx.crate_types().contains(&CrateType::Executable) { tcx.dcx().fatal("miri only makes sense on bin crates"); } @@ -161,64 +193,28 @@ impl rustc_driver::Callbacks for MiriCompilerCalls { optimizations is usually marginal at best."); } - // Run in GenMC mode if enabled. - if config.genmc_config.is_some() { - // Validate GenMC settings. - if let Err(err) = GenmcConfig::validate(&mut config, tcx) { - fatal_error!("Invalid settings: {err}"); - } - - // This is the entry point used in GenMC mode. - // This closure will be called multiple times to explore the concurrent execution space of the program. - let eval_entry_once = |genmc_ctx: Rc| { + let res = if config.genmc_config.is_some() { + assert!(self.many_seeds.is_none()); + run_genmc_mode(tcx, &config, |genmc_ctx: Rc| { miri::eval_entry(tcx, entry_def_id, entry_type, &config, Some(genmc_ctx)) - }; - let return_code = run_genmc_mode(&config, eval_entry_once, tcx).unwrap_or_else(|| { - tcx.dcx().abort_if_errors(); - rustc_driver::EXIT_FAILURE - }); - exit(return_code); - }; - - if let Some(many_seeds) = self.many_seeds.take() { + }) + } else if let Some(many_seeds) = self.many_seeds.take() { assert!(config.seed.is_none()); - let exit_code = sync::IntoDynSyncSend(AtomicI32::new(rustc_driver::EXIT_SUCCESS)); - let num_failed = sync::IntoDynSyncSend(AtomicU32::new(0)); - sync::par_for_each_in(many_seeds.seeds.clone(), |seed| { + run_many_seeds(many_seeds, |seed| { let mut config = config.clone(); - config.seed = Some((*seed).into()); + config.seed = Some(seed); eprintln!("Trying seed: {seed}"); - let return_code = miri::eval_entry( - tcx, - entry_def_id, - entry_type, - &config, - /* genmc_ctx */ None, - ) - .unwrap_or(rustc_driver::EXIT_FAILURE); - if return_code != rustc_driver::EXIT_SUCCESS { - eprintln!("FAILING SEED: {seed}"); - if !many_seeds.keep_going { - // `abort_if_errors` would actually not stop, since `par_for_each` waits for the - // rest of the to finish, so we just exit immediately. - exit(return_code); - } - exit_code.store(return_code, Ordering::Relaxed); - num_failed.fetch_add(1, Ordering::Relaxed); - } - }); - let num_failed = num_failed.0.into_inner(); - if num_failed > 0 { - eprintln!("{num_failed}/{total} SEEDS FAILED", total = many_seeds.seeds.count()); - } - exit(exit_code.0.into_inner()); + miri::eval_entry(tcx, entry_def_id, entry_type, &config, /* genmc_ctx */ None) + }) } else { - let return_code = miri::eval_entry(tcx, entry_def_id, entry_type, &config, None) - .unwrap_or_else(|| { - tcx.dcx().abort_if_errors(); - rustc_driver::EXIT_FAILURE - }); - exit(return_code); + miri::eval_entry(tcx, entry_def_id, entry_type, &config, None) + }; + + if let Err(return_code) = res { + tcx.dcx().abort_if_errors(); + exit(return_code.get()); + } else { + exit(rustc_driver::EXIT_SUCCESS); } // Unreachable. @@ -747,6 +743,13 @@ fn main() { ); }; + // Validate GenMC settings. + if miri_config.genmc_config.is_some() + && let Err(err) = GenmcConfig::validate(&mut miri_config) + { + fatal_error!("Invalid settings: {err}"); + } + debug!("rustc arguments: {:?}", rustc_args); debug!("crate arguments: {:?}", miri_config.args); if !miri_config.native_lib.is_empty() && miri_config.native_lib_enable_tracing { diff --git a/src/concurrency/genmc/config.rs b/src/concurrency/genmc/config.rs index a05cda46f3..ea5760f7ac 100644 --- a/src/concurrency/genmc/config.rs +++ b/src/concurrency/genmc/config.rs @@ -1,6 +1,4 @@ use genmc_sys::LogLevel; -use rustc_abi::Endian; -use rustc_middle::ty::TyCtxt; use super::GenmcParams; use crate::{IsolatedOp, MiriConfig, RejectOpWith}; @@ -86,16 +84,11 @@ impl GenmcConfig { /// /// Unsupported configurations return an error. /// Adjusts Miri settings where required, printing a warnings if the change might be unexpected for the user. - pub fn validate(miri_config: &mut MiriConfig, tcx: TyCtxt<'_>) -> Result<(), &'static str> { + pub fn validate(miri_config: &mut MiriConfig) -> Result<(), &'static str> { let Some(genmc_config) = miri_config.genmc_config.as_mut() else { return Ok(()); }; - // Check for supported target. - if tcx.data_layout.endian != Endian::Little || tcx.data_layout.pointer_size().bits() != 64 { - return Err("GenMC only supports 64bit little-endian targets"); - } - // Check for disallowed configurations. if !miri_config.data_race_detector { return Err("Cannot disable data race detection in GenMC mode"); diff --git a/src/concurrency/genmc/dummy.rs b/src/concurrency/genmc/dummy.rs index b9e09e34dc..d643bda28c 100644 --- a/src/concurrency/genmc/dummy.rs +++ b/src/concurrency/genmc/dummy.rs @@ -1,6 +1,5 @@ use rustc_abi::{Align, Size}; use rustc_const_eval::interpret::{AllocId, InterpCx, InterpResult}; -use rustc_middle::ty::TyCtxt; pub use self::intercept::EvalContextExt as GenmcEvalContextExt; pub use self::run::run_genmc_mode; @@ -23,6 +22,7 @@ pub struct GenmcCtx {} pub struct GenmcConfig {} mod run { + use std::num::NonZeroI32; use std::rc::Rc; use rustc_middle::ty::TyCtxt; @@ -30,10 +30,10 @@ mod run { use crate::{GenmcCtx, MiriConfig}; pub fn run_genmc_mode<'tcx>( - _config: &MiriConfig, - _eval_entry: impl Fn(Rc) -> Option, _tcx: TyCtxt<'tcx>, - ) -> Option { + _config: &MiriConfig, + _eval_entry: impl Fn(Rc) -> Result<(), NonZeroI32>, + ) -> Result<(), NonZeroI32> { unreachable!(); } } @@ -240,10 +240,7 @@ impl GenmcConfig { } } - pub fn validate( - _miri_config: &mut crate::MiriConfig, - _tcx: TyCtxt<'_>, - ) -> Result<(), &'static str> { + pub fn validate(_miri_config: &mut crate::MiriConfig) -> Result<(), &'static str> { Ok(()) } } diff --git a/src/concurrency/genmc/run.rs b/src/concurrency/genmc/run.rs index 6eb51e1b50..2b0de62ccd 100644 --- a/src/concurrency/genmc/run.rs +++ b/src/concurrency/genmc/run.rs @@ -1,8 +1,10 @@ +use std::num::NonZeroI32; use std::rc::Rc; use std::sync::Arc; use std::time::Instant; use genmc_sys::EstimationResult; +use rustc_abi::Endian; use rustc_log::tracing; use rustc_middle::ty::TyCtxt; @@ -24,10 +26,15 @@ pub(super) enum GenmcMode { /// /// Returns `None` is an error is detected, or `Some(return_value)` with the return value of the last run of the program. pub fn run_genmc_mode<'tcx>( - config: &MiriConfig, - eval_entry: impl Fn(Rc) -> Option, tcx: TyCtxt<'tcx>, -) -> Option { + config: &MiriConfig, + eval_entry: impl Fn(Rc) -> Result<(), NonZeroI32>, +) -> Result<(), NonZeroI32> { + // Check for supported target. + if tcx.data_layout.endian != Endian::Little || tcx.data_layout.pointer_size().bits() != 64 { + tcx.dcx().fatal("GenMC only supports 64bit little-endian targets"); + } + let genmc_config = config.genmc_config.as_ref().unwrap(); // Run in Estimation mode if requested. if genmc_config.do_estimation { @@ -41,10 +48,10 @@ pub fn run_genmc_mode<'tcx>( fn run_genmc_mode_impl<'tcx>( config: &MiriConfig, - eval_entry: &impl Fn(Rc) -> Option, + eval_entry: &impl Fn(Rc) -> Result<(), NonZeroI32>, tcx: TyCtxt<'tcx>, mode: GenmcMode, -) -> Option { +) -> Result<(), NonZeroI32> { let time_start = Instant::now(); let genmc_config = config.genmc_config.as_ref().unwrap(); @@ -62,9 +69,9 @@ fn run_genmc_mode_impl<'tcx>( genmc_ctx.prepare_next_execution(); // Execute the program until completion to get the return value, or return if an error happens: - let Some(return_code) = eval_entry(genmc_ctx.clone()) else { + if let Err(err) = eval_entry(genmc_ctx.clone()) { genmc_ctx.print_genmc_output(genmc_config, tcx); - return None; + return Err(err); }; // We inform GenMC that the execution is complete. @@ -80,18 +87,17 @@ fn run_genmc_mode_impl<'tcx>( genmc_ctx.print_verification_output(genmc_config, elapsed_time_sec); } // Return the return code of the last execution. - return Some(return_code); + return Ok(()); } ExecutionEndResult::Error(error) => { // This can be reached for errors that affect the entire execution, not just a specific event. // For instance, linearizability checking and liveness checking report their errors this way. - // Neither are supported by Miri-GenMC at the moment though. However, GenMC also - // treats races on deallocation as global errors, so this code path is still reachable. + // Neither are supported by Miri-GenMC at the moment though. // Since we don't have any span information for the error at this point, // we just print GenMC's error string, and the full GenMC output if requested. eprintln!("(GenMC) Error detected: {error}"); genmc_ctx.print_genmc_output(genmc_config, tcx); - return None; + return Err(NonZeroI32::new(rustc_driver::EXIT_FAILURE).unwrap()); } } } diff --git a/src/diagnostics.rs b/src/diagnostics.rs index 2ddb3ff49d..47d60f912c 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -226,19 +226,20 @@ pub fn prune_stacktrace<'tcx>( } } -/// Emit a custom diagnostic without going through the miri-engine machinery. +/// Report the result of a Miri execution. /// -/// Returns `Some` if this was regular program termination with a given exit code and a `bool` indicating whether a leak check should happen; `None` otherwise. -pub fn report_error<'tcx>( +/// Returns `Some` if this was regular program termination with a given exit code and a `bool` +/// indicating whether a leak check should happen; `None` otherwise. +pub fn report_result<'tcx>( ecx: &InterpCx<'tcx, MiriMachine<'tcx>>, - e: InterpErrorInfo<'tcx>, + res: InterpErrorInfo<'tcx>, ) -> Option<(i32, bool)> { use InterpErrorKind::*; use UndefinedBehaviorInfo::*; let mut labels = vec![]; - let (title, helps) = if let MachineStop(info) = e.kind() { + let (title, helps) = if let MachineStop(info) = res.kind() { let info = info.downcast_ref::().expect("invalid MachineStop payload"); use TerminationInfo::*; let title = match info { @@ -334,7 +335,7 @@ pub fn report_error<'tcx>( }; (title, helps) } else { - let title = match e.kind() { + let title = match res.kind() { UndefinedBehavior(ValidationError(validation_err)) if matches!( validation_err.kind, @@ -344,7 +345,7 @@ pub fn report_error<'tcx>( ecx.handle_ice(); // print interpreter backtrace (this is outside the eval `catch_unwind`) bug!( "This validation error should be impossible in Miri: {}", - format_interp_error(ecx.tcx.dcx(), e) + format_interp_error(ecx.tcx.dcx(), res) ); } UndefinedBehavior(_) => "Undefined Behavior", @@ -363,12 +364,12 @@ pub fn report_error<'tcx>( ecx.handle_ice(); // print interpreter backtrace (this is outside the eval `catch_unwind`) bug!( "This error should be impossible in Miri: {}", - format_interp_error(ecx.tcx.dcx(), e) + format_interp_error(ecx.tcx.dcx(), res) ); } }; #[rustfmt::skip] - let helps = match e.kind() { + let helps = match res.kind() { Unsupported(_) => vec![ note!("this is likely not a bug in the program; it indicates that the program performed an operation that Miri does not support"), @@ -422,7 +423,7 @@ pub fn report_error<'tcx>( // We want to dump the allocation if this is `InvalidUninitBytes`. // Since `format_interp_error` consumes `e`, we compute the outut early. let mut extra = String::new(); - match e.kind() { + match res.kind() { UndefinedBehavior(InvalidUninitBytes(Some((alloc_id, access)))) => { writeln!( extra, @@ -448,7 +449,7 @@ pub fn report_error<'tcx>( if let Some(title) = title { write!(primary_msg, "{title}: ").unwrap(); } - write!(primary_msg, "{}", format_interp_error(ecx.tcx.dcx(), e)).unwrap(); + write!(primary_msg, "{}", format_interp_error(ecx.tcx.dcx(), res)).unwrap(); if labels.is_empty() { labels.push(format!("{} occurred here", title.unwrap_or("error"))); diff --git a/src/eval.rs b/src/eval.rs index 6daee0ba69..5decb44854 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -1,6 +1,7 @@ //! Main evaluator loop and setting up the initial stack frame. use std::ffi::{OsStr, OsString}; +use std::num::NonZeroI32; use std::panic::{self, AssertUnwindSafe}; use std::path::PathBuf; use std::rc::Rc; @@ -462,7 +463,7 @@ pub fn eval_entry<'tcx>( entry_type: MiriEntryFnType, config: &MiriConfig, genmc_ctx: Option>, -) -> Option { +) -> Result<(), NonZeroI32> { // Copy setting before we move `config`. let ignore_leaks = config.ignore_leaks; @@ -482,35 +483,50 @@ pub fn eval_entry<'tcx>( ecx.handle_ice(); panic::resume_unwind(panic_payload) }); - // `Ok` can never happen; the interpreter loop always exits with an "error" - // (but that "error" might be just "regular program termination"). - let Err(err) = res.report_err(); - - // Show diagnostic, if any. - let (return_code, leak_check) = report_error(&ecx, err)?; - - // If we get here there was no fatal error. - - // Possibly check for memory leaks. - if leak_check && !ignore_leaks { - // Check for thread leaks. - if !ecx.have_all_terminated() { - tcx.dcx().err("the main thread terminated without waiting for all remaining threads"); - tcx.dcx().note("set `MIRIFLAGS=-Zmiri-ignore-leaks` to disable this check"); - return None; - } - // Check for memory leaks. - info!("Additional static roots: {:?}", ecx.machine.static_roots); - let leaks = ecx.take_leaked_allocations(|ecx| &ecx.machine.static_roots); - if !leaks.is_empty() { - report_leaks(&ecx, leaks); - tcx.dcx().note("set `MIRIFLAGS=-Zmiri-ignore-leaks` to disable this check"); - // Ignore the provided return code - let the reported error - // determine the return code. - return None; + // Obtain the result of the execution. This is always an `Err`, but that doesn't necessarily + // indicate an error. + let Err(res) = res.report_err(); + + // Error reporting: if we survive all checks, we return the exit code the program gave us. + 'miri_error: { + // Show diagnostic, if any. + let Some((return_code, leak_check)) = report_result(&ecx, res) else { + break 'miri_error; + }; + + // If we get here there was no fatal error -- yet. + // Possibly check for memory leaks. + if leak_check && !ignore_leaks { + // Check for thread leaks. + if !ecx.have_all_terminated() { + tcx.dcx() + .err("the main thread terminated without waiting for all remaining threads"); + tcx.dcx().note("set `MIRIFLAGS=-Zmiri-ignore-leaks` to disable this check"); + break 'miri_error; + } + // Check for memory leaks. + info!("Additional static roots: {:?}", ecx.machine.static_roots); + let leaks = ecx.take_leaked_allocations(|ecx| &ecx.machine.static_roots); + if !leaks.is_empty() { + report_leaks(&ecx, leaks); + tcx.dcx().note("set `MIRIFLAGS=-Zmiri-ignore-leaks` to disable this check"); + // Ignore the provided return code - let the reported error + // determine the return code. + break 'miri_error; + } } + + // The interpreter has not reported an error. + // (There could still be errors in the session if there are other interpreters.) + return match NonZeroI32::new(return_code) { + None => Ok(()), + Some(return_code) => Err(return_code), + }; } - Some(return_code) + + // The interpreter reported an error. + assert!(tcx.dcx().has_errors().is_some()); + Err(NonZeroI32::new(rustc_driver::EXIT_FAILURE).unwrap()) } /// Turns an array of arguments into a Windows command line string. diff --git a/src/lib.rs b/src/lib.rs index 3137cfc833..fe9b9665a1 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -136,7 +136,7 @@ pub use crate::concurrency::{GenmcConfig, GenmcCtx, run_genmc_mode}; pub use crate::data_structures::dedup_range_map::DedupRangeMap; pub use crate::data_structures::mono_hash_map::MonoHashMap; pub use crate::diagnostics::{ - EvalContextExt as _, NonHaltingDiagnostic, TerminationInfo, report_error, + EvalContextExt as _, NonHaltingDiagnostic, TerminationInfo, report_result, }; pub use crate::eval::{MiriConfig, MiriEntryFnType, create_ecx, eval_entry}; pub use crate::helpers::{EvalContextExt as _, ToU64 as _, ToUsize as _}; diff --git a/tests/fail/deny_lint.rs b/tests/fail/deny_lint.rs index f49fa49d09..b71c30ceac 100644 --- a/tests/fail/deny_lint.rs +++ b/tests/fail/deny_lint.rs @@ -1,5 +1,3 @@ -//@error-in-other-file: miri cannot be run on programs that fail compilation - #![deny(warnings, unused)] struct Foo; diff --git a/tests/fail/deny_lint.stderr b/tests/fail/deny_lint.stderr index fe96edf934..053a3dec5f 100644 --- a/tests/fail/deny_lint.stderr +++ b/tests/fail/deny_lint.stderr @@ -11,7 +11,5 @@ LL | #![deny(warnings, unused)] | ^^^^^^ = note: `#[deny(dead_code)]` implied by `#[deny(unused)]` -error: miri cannot be run on programs that fail compilation - -error: aborting due to 2 previous errors +error: aborting due to 1 previous error