Merge pull request #4690 from RalfJung/top-level-error

refactor and unify top-level error handling a bit

Author: RalfJung

src/tools/miri/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, 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<GenmcCtx>| {
+        let res = if config.genmc_config.is_some() {
+            assert!(self.many_seeds.is_none());
+            run_genmc_mode(tcx, &config, |genmc_ctx: Rc<GenmcCtx>| {
                 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.
@@ -743,6 +739,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 {

src/tools/miri/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");

src/tools/miri/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,17 +22,18 @@ pub struct GenmcCtx {}
 pub struct GenmcConfig {}
 
 mod run {
+    use std::num::NonZeroI32;
     use std::rc::Rc;
 
     use rustc_middle::ty::TyCtxt;
 
     use crate::{GenmcCtx, MiriConfig};
 
     pub fn run_genmc_mode<'tcx>(
-        _config: &MiriConfig,
-        _eval_entry: impl Fn(Rc<GenmcCtx>) -> Option<i32>,
         _tcx: TyCtxt<'tcx>,
-    ) -> Option<i32> {
+        _config: &MiriConfig,
+        _eval_entry: impl Fn(Rc<GenmcCtx>) -> 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(())
     }
 }

src/tools/miri/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<GenmcCtx>) -> Option<i32>,
     tcx: TyCtxt<'tcx>,
-) -> Option<i32> {
+    config: &MiriConfig,
+    eval_entry: impl Fn(Rc<GenmcCtx>) -> 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<GenmcCtx>) -> Option<i32>,
+    eval_entry: &impl Fn(Rc<GenmcCtx>) -> Result<(), NonZeroI32>,
     tcx: TyCtxt<'tcx>,
     mode: GenmcMode,
-) -> Option<i32> {
+) -> 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());
             }
         }
     }

src/tools/miri/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::<TerminationInfo>().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")));