From a1b823886c072dfda3e3fdd34e5661beaf57d6d7 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 22 Mar 2020 18:50:12 +0100 Subject: [PATCH 1/4] give some context in error messages --- src/diagnostics.rs | 89 ++++++++++++++++++++++++++++------------------ 1 file changed, 54 insertions(+), 35 deletions(-) diff --git a/src/diagnostics.rs b/src/diagnostics.rs index b2590a843b..ee4084f42d 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -1,6 +1,7 @@ -use rustc_mir::interpret::InterpErrorInfo; use std::cell::RefCell; +use rustc_span::DUMMY_SP; + use crate::*; /// Miri specific diagnostics @@ -14,9 +15,16 @@ pub fn report_diagnostic<'tcx, 'mir>( ecx: &InterpCx<'mir, 'tcx, Evaluator<'tcx>>, mut e: InterpErrorInfo<'tcx>, ) -> Option { - // Special treatment for some error kinds + use InterpError::*; + let title = match e.kind { + Unsupported(_) => "unsupported operation", + UndefinedBehavior(_) => "Undefined Behavior", + InvalidProgram(_) => bug!("This error should be impossible in Miri: {}", e), + ResourceExhaustion(_) => "resource exhaustion", + MachineStop(_) => "program stopped", + }; let msg = match e.kind { - InterpError::MachineStop(ref info) => { + MachineStop(ref info) => { let info = info.downcast_ref::().expect("invalid MachineStop payload"); match info { TerminationInfo::Exit(code) => return Some(*code), @@ -24,52 +32,63 @@ pub fn report_diagnostic<'tcx, 'mir>( TerminationInfo::Abort(Some(msg)) => format!("the evaluated program aborted execution: {}", msg), } } - err_unsup!(NoMirFor(..)) => format!( - "{}. Did you set `MIRI_SYSROOT` to a Miri-enabled sysroot? You can prepare one with `cargo miri setup`.", - e - ), - InterpError::InvalidProgram(_) => bug!("This error should be impossible in Miri: {}", e), _ => e.to_string(), }; + let help = match e.kind { + Unsupported(UnsupportedOpInfo::NoMirFor(..)) => + Some("set `MIRI_SYSROOT` to a Miri sysroot, which you can prepare with `cargo miri setup`"), + Unsupported(_) => + Some("this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support"), + UndefinedBehavior(UndefinedBehaviorInfo::UbExperimental(_)) => + Some("this indicates a potential bug in the program: it violated *experimental* rules, and caused Undefined Behavior"), + UndefinedBehavior(_) => + Some("this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior"), + _ => None, + }; e.print_backtrace(); - report_msg(ecx, msg, true) + report_msg(ecx, &format!("{}: {}", title, msg), msg, help, true) } /// Report an error or note (depending on the `error` argument) at the current frame's current statement. /// Also emits a full stacktrace of the interpreter stack. pub fn report_msg<'tcx, 'mir>( ecx: &InterpCx<'mir, 'tcx, Evaluator<'tcx>>, - msg: String, + title: &str, + span_msg: String, + help: Option<&str>, error: bool, ) -> Option { - if let Some(frame) = ecx.stack().last() { - let span = frame.current_source_info().unwrap().span; - - let mut err = if error { - let msg = format!("Miri evaluation error: {}", msg); - ecx.tcx.sess.struct_span_err(span, msg.as_str()) + let span = if let Some(frame) = ecx.stack().last() { + frame.current_source_info().unwrap().span + } else { + DUMMY_SP + }; + let mut err = if error { + ecx.tcx.sess.struct_span_err(span, title) + } else { + ecx.tcx.sess.diagnostic().span_note_diag(span, title) + }; + err.span_label(span, span_msg); + if let Some(help) = help { + err.help(help); + } + // Add backtrace + let frames = ecx.generate_stacktrace(None); + // We iterate with indices because we need to look at the next frame (the caller). + for idx in 0..frames.len() { + let frame_info = &frames[idx]; + let call_site_is_local = frames + .get(idx + 1) + .map_or(false, |caller_info| caller_info.instance.def_id().is_local()); + if call_site_is_local { + err.span_note(frame_info.call_site, &frame_info.to_string()); } else { - ecx.tcx.sess.diagnostic().span_note_diag(span, msg.as_str()) - }; - let frames = ecx.generate_stacktrace(None); - err.span_label(span, msg); - // We iterate with indices because we need to look at the next frame (the caller). - for idx in 0..frames.len() { - let frame_info = &frames[idx]; - let call_site_is_local = frames - .get(idx + 1) - .map_or(false, |caller_info| caller_info.instance.def_id().is_local()); - if call_site_is_local { - err.span_note(frame_info.call_site, &frame_info.to_string()); - } else { - err.note(&frame_info.to_string()); - } + err.note(&frame_info.to_string()); } - err.emit(); - } else { - ecx.tcx.sess.err(&msg); } + err.emit(); + for (i, frame) in ecx.stack().iter().enumerate() { trace!("-------------------"); trace!("Frame {}", i); @@ -106,7 +125,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx CreatedAlloc(AllocId(id)) => format!("created allocation with id {}", id), }; - report_msg(this, msg, false); + report_msg(this, "tracking was triggered", msg, None, false); } }); } From 6dcca62b63135306aab50afc85075afa7427559a Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 22 Mar 2020 19:48:59 +0100 Subject: [PATCH 2/4] move -Zmiri-disable-isolation hint to help --- src/diagnostics.rs | 82 ++++++++++++++++++++++++++++++---------------- src/eval.rs | 8 +---- src/helpers.rs | 6 ++-- src/lib.rs | 6 ++-- 4 files changed, 60 insertions(+), 42 deletions(-) diff --git a/src/diagnostics.rs b/src/diagnostics.rs index ee4084f42d..c46ff5354d 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -4,6 +4,13 @@ use rustc_span::DUMMY_SP; use crate::*; +/// Details of premature program termination. +pub enum TerminationInfo { + Exit(i64), + Abort(Option), + UnsupportedInIsolation(String), +} + /// Miri specific diagnostics pub enum NonHaltingDiagnostic { PoppedTrackedPointerTag(Item), @@ -11,47 +18,64 @@ pub enum NonHaltingDiagnostic { } /// Emit a custom diagnostic without going through the miri-engine machinery -pub fn report_diagnostic<'tcx, 'mir>( +pub fn report_error<'tcx, 'mir>( ecx: &InterpCx<'mir, 'tcx, Evaluator<'tcx>>, mut e: InterpErrorInfo<'tcx>, ) -> Option { use InterpError::*; - let title = match e.kind { - Unsupported(_) => "unsupported operation", - UndefinedBehavior(_) => "Undefined Behavior", - InvalidProgram(_) => bug!("This error should be impossible in Miri: {}", e), - ResourceExhaustion(_) => "resource exhaustion", - MachineStop(_) => "program stopped", - }; - let msg = match e.kind { - MachineStop(ref info) => { + + e.print_backtrace(); + let (title, msg, help) = match e.kind { + MachineStop(info) => { let info = info.downcast_ref::().expect("invalid MachineStop payload"); - match info { - TerminationInfo::Exit(code) => return Some(*code), - TerminationInfo::Abort(None) => format!("the evaluated program aborted execution"), - TerminationInfo::Abort(Some(msg)) => format!("the evaluated program aborted execution: {}", msg), - } + use TerminationInfo::*; + let (title, msg) = match info { + Exit(code) => return Some(*code), + Abort(None) => + ("abnormal termination", format!("the evaluated program aborted execution")), + Abort(Some(msg)) => + ("abnormal termination", format!("the evaluated program aborted execution: {}", msg)), + UnsupportedInIsolation(msg) => + ("unsupported operation", format!("{}", msg)), + }; + let help = match info { + UnsupportedInIsolation(_) => + Some("pass the flag `-Zmiri-disable-isolation` to disable isolation"), + _ => None, + }; + (title, msg, help) + } + _ => { + let (title, msg) = match e.kind { + Unsupported(_) => + ("unsupported operation", e.to_string()), + UndefinedBehavior(_) => + ("Undefined Behavior", e.to_string()), + ResourceExhaustion(_) => + ("resource exhaustion", e.to_string()), + _ => + bug!("This error should be impossible in Miri: {}", e), + }; + let help = match e.kind { + Unsupported(UnsupportedOpInfo::NoMirFor(..)) => + Some("set `MIRI_SYSROOT` to a Miri sysroot, which you can prepare with `cargo miri setup`"), + Unsupported(_) => + Some("this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support"), + UndefinedBehavior(UndefinedBehaviorInfo::UbExperimental(_)) => + Some("this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental"), + UndefinedBehavior(_) => + Some("this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior"), + _ => None, + }; + (title, msg, help) } - _ => e.to_string(), - }; - let help = match e.kind { - Unsupported(UnsupportedOpInfo::NoMirFor(..)) => - Some("set `MIRI_SYSROOT` to a Miri sysroot, which you can prepare with `cargo miri setup`"), - Unsupported(_) => - Some("this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support"), - UndefinedBehavior(UndefinedBehaviorInfo::UbExperimental(_)) => - Some("this indicates a potential bug in the program: it violated *experimental* rules, and caused Undefined Behavior"), - UndefinedBehavior(_) => - Some("this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior"), - _ => None, }; - e.print_backtrace(); report_msg(ecx, &format!("{}: {}", title, msg), msg, help, true) } /// Report an error or note (depending on the `error` argument) at the current frame's current statement. /// Also emits a full stacktrace of the interpreter stack. -pub fn report_msg<'tcx, 'mir>( +fn report_msg<'tcx, 'mir>( ecx: &InterpCx<'mir, 'tcx, Evaluator<'tcx>>, title: &str, span_msg: String, diff --git a/src/eval.rs b/src/eval.rs index de6c9fac1d..d2ca57c39a 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -51,12 +51,6 @@ impl Default for MiriConfig { } } -/// Details of premature program termination. -pub enum TerminationInfo { - Exit(i64), - Abort(Option), -} - /// Returns a freshly created `InterpCx`, along with an `MPlaceTy` representing /// the location where the return value of the `start` lang item will be /// written to. @@ -229,6 +223,6 @@ pub fn eval_main<'tcx>(tcx: TyCtxt<'tcx>, main_id: DefId, config: MiriConfig) -> } Some(return_code) } - Err(e) => report_diagnostic(&ecx, e), + Err(e) => report_error(&ecx, e), } } diff --git a/src/helpers.rs b/src/helpers.rs index 36d3181ce4..ab6e33f231 100644 --- a/src/helpers.rs +++ b/src/helpers.rs @@ -367,10 +367,10 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx /// case. fn check_no_isolation(&self, name: &str) -> InterpResult<'tcx> { if !self.eval_context_ref().machine.communicate { - throw_unsup_format!( - "`{}` not available when isolation is enabled (pass the flag `-Zmiri-disable-isolation` to disable isolation)", + throw_machine_stop!(TerminationInfo::UnsupportedInIsolation(format!( + "`{}` not available when isolation is enabled", name, - ) + ))) } Ok(()) } diff --git a/src/lib.rs b/src/lib.rs index 86fbabbd62..32eb5b41e5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -45,10 +45,10 @@ pub use crate::shims::tls::{EvalContextExt as TlsEvalContextExt, TlsData}; pub use crate::shims::EvalContextExt as ShimsEvalContextExt; pub use crate::diagnostics::{ - register_diagnostic, report_diagnostic, EvalContextExt as DiagnosticsEvalContextExt, - NonHaltingDiagnostic, + register_diagnostic, report_error, EvalContextExt as DiagnosticsEvalContextExt, + TerminationInfo, NonHaltingDiagnostic, }; -pub use crate::eval::{create_ecx, eval_main, MiriConfig, TerminationInfo}; +pub use crate::eval::{create_ecx, eval_main, MiriConfig}; pub use crate::helpers::EvalContextExt as HelpersEvalContextExt; pub use crate::machine::{ AllocExtra, Evaluator, FrameData, MemoryExtra, MiriEvalContext, MiriEvalContextExt, From dd4fef0cd95ba90ea6e1a13f054e2b66f36772ff Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 22 Mar 2020 20:09:14 +0100 Subject: [PATCH 3/4] fix outdated sysroot help message --- src/diagnostics.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/diagnostics.rs b/src/diagnostics.rs index c46ff5354d..1008c04684 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -58,7 +58,7 @@ pub fn report_error<'tcx, 'mir>( }; let help = match e.kind { Unsupported(UnsupportedOpInfo::NoMirFor(..)) => - Some("set `MIRI_SYSROOT` to a Miri sysroot, which you can prepare with `cargo miri setup`"), + Some("make sure to use a Miri sysroot, which you can prepare with `cargo miri setup`"), Unsupported(_) => Some("this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support"), UndefinedBehavior(UndefinedBehaviorInfo::UbExperimental(_)) => From 6e302b830a8ac07efc1dca3ba1354338801bf7d4 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 22 Mar 2020 23:32:19 +0100 Subject: [PATCH 4/4] link to some websites for UB explanations --- src/diagnostics.rs | 43 +++++++++++++++++++++++++----------------- src/stacked_borrows.rs | 29 ++++++++++++++++++---------- 2 files changed, 45 insertions(+), 27 deletions(-) diff --git a/src/diagnostics.rs b/src/diagnostics.rs index 1008c04684..a2cdffdf80 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -9,6 +9,7 @@ pub enum TerminationInfo { Exit(i64), Abort(Option), UnsupportedInIsolation(String), + ExperimentalUb { msg: String, url: String } } /// Miri specific diagnostics @@ -25,7 +26,7 @@ pub fn report_error<'tcx, 'mir>( use InterpError::*; e.print_backtrace(); - let (title, msg, help) = match e.kind { + let (title, msg, helps) = match e.kind { MachineStop(info) => { let info = info.downcast_ref::().expect("invalid MachineStop payload"); use TerminationInfo::*; @@ -37,13 +38,20 @@ pub fn report_error<'tcx, 'mir>( ("abnormal termination", format!("the evaluated program aborted execution: {}", msg)), UnsupportedInIsolation(msg) => ("unsupported operation", format!("{}", msg)), + ExperimentalUb { msg, .. } => + ("Undefined Behavior", format!("{}", msg)), }; - let help = match info { + let helps = match info { UnsupportedInIsolation(_) => - Some("pass the flag `-Zmiri-disable-isolation` to disable isolation"), - _ => None, + vec![format!("pass the flag `-Zmiri-disable-isolation` to disable isolation")], + ExperimentalUb { url, .. } => + vec![ + format!("this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental"), + format!("see {} for further information", url), + ], + _ => vec![], }; - (title, msg, help) + (title, msg, helps) } _ => { let (title, msg) = match e.kind { @@ -56,21 +64,22 @@ pub fn report_error<'tcx, 'mir>( _ => bug!("This error should be impossible in Miri: {}", e), }; - let help = match e.kind { + let helps = match e.kind { Unsupported(UnsupportedOpInfo::NoMirFor(..)) => - Some("make sure to use a Miri sysroot, which you can prepare with `cargo miri setup`"), + vec![format!("make sure to use a Miri sysroot, which you can prepare with `cargo miri setup`")], Unsupported(_) => - Some("this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support"), - UndefinedBehavior(UndefinedBehaviorInfo::UbExperimental(_)) => - Some("this indicates a potential bug in the program: it performed an invalid operation, but the rules it violated are still experimental"), + vec![format!("this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support")], UndefinedBehavior(_) => - Some("this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior"), - _ => None, + vec![ + format!("this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior"), + format!("see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information"), + ], + _ => vec![], }; - (title, msg, help) + (title, msg, helps) } }; - report_msg(ecx, &format!("{}: {}", title, msg), msg, help, true) + report_msg(ecx, &format!("{}: {}", title, msg), msg, &helps, true) } /// Report an error or note (depending on the `error` argument) at the current frame's current statement. @@ -79,7 +88,7 @@ fn report_msg<'tcx, 'mir>( ecx: &InterpCx<'mir, 'tcx, Evaluator<'tcx>>, title: &str, span_msg: String, - help: Option<&str>, + helps: &[String], error: bool, ) -> Option { let span = if let Some(frame) = ecx.stack().last() { @@ -93,7 +102,7 @@ fn report_msg<'tcx, 'mir>( ecx.tcx.sess.diagnostic().span_note_diag(span, title) }; err.span_label(span, span_msg); - if let Some(help) = help { + for help in helps { err.help(help); } // Add backtrace @@ -149,7 +158,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx CreatedAlloc(AllocId(id)) => format!("created allocation with id {}", id), }; - report_msg(this, "tracking was triggered", msg, None, false); + report_msg(this, "tracking was triggered", msg, &[], false); } }); } diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index 6188d05780..05b9c478cf 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -192,6 +192,15 @@ impl GlobalState { } } +/// Error reporting +fn err_sb_ub(msg: String) -> InterpError<'static> { + // FIXME: use `err_machine_stop!` macro, once that exists. + InterpError::MachineStop(Box::new(TerminationInfo::ExperimentalUb { + msg, + url: format!("https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md"), + })) +} + // # Stacked Borrows Core Begin /// We need to make at least the following things true: @@ -272,15 +281,15 @@ impl<'tcx> Stack { if let Some(call) = item.protector { if global.is_active(call) { if let Some(tag) = tag { - throw_ub!(UbExperimental(format!( + Err(err_sb_ub(format!( "not granting access to tag {:?} because incompatible item is protected: {:?}", tag, item - ))); + )))? } else { - throw_ub!(UbExperimental(format!( + Err(err_sb_ub(format!( "deallocating while item is protected: {:?}", item - ))); + )))? } } } @@ -294,10 +303,10 @@ impl<'tcx> Stack { // Step 1: Find granting item. let granting_idx = self.find_granting(access, tag).ok_or_else(|| { - err_ub!(UbExperimental(format!( + err_sb_ub(format!( "no item granting {} to tag {:?} found in borrow stack.", access, tag - ),)) + )) })?; // Step 2: Remove incompatible items above them. Make sure we do not remove protected @@ -338,10 +347,10 @@ impl<'tcx> Stack { fn dealloc(&mut self, tag: Tag, global: &GlobalState) -> InterpResult<'tcx> { // Step 1: Find granting item. self.find_granting(AccessKind::Write, tag).ok_or_else(|| { - err_ub!(UbExperimental(format!( + err_sb_ub(format!( "no item granting write access for deallocation to tag {:?} found in borrow stack", tag, - ))) + )) })?; // Step 2: Remove all items. Also checks for protectors. @@ -363,10 +372,10 @@ impl<'tcx> Stack { // Now we figure out which item grants our parent (`derived_from`) this kind of access. // We use that to determine where to put the new item. let granting_idx = self.find_granting(access, derived_from) - .ok_or_else(|| err_ub!(UbExperimental(format!( + .ok_or_else(|| err_sb_ub(format!( "trying to reborrow for {:?}, but parent tag {:?} does not have an appropriate item in the borrow stack", new.perm, derived_from, - ))))?; + )))?; // Compute where to put the new item. // Either way, we ensure that we insert the new item in a way such that between