From b9f6b9721a72abfb9f83d8b25cb41ed5c618d755 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Tue, 24 Dec 2019 00:07:55 +0100 Subject: [PATCH 01/12] Split error reporting from main eval function --- src/eval.rs | 110 ++++++++++++++++++++++++++-------------------------- 1 file changed, 56 insertions(+), 54 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index eac9b8a83b..b2f290414c 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -8,6 +8,7 @@ use rand::SeedableRng; use rustc_hir::def_id::DefId; use rustc::ty::layout::{LayoutOf, Size}; use rustc::ty::{self, TyCtxt}; +use rustc_mir::interpret::InterpErrorInfo; use crate::*; @@ -205,64 +206,65 @@ pub fn eval_main<'tcx>(tcx: TyCtxt<'tcx>, main_id: DefId, config: MiriConfig) -> } return Some(return_code); } - Err(mut e) => { - // Special treatment for some error kinds - let msg = match e.kind { - InterpError::MachineStop(ref info) => { - let info = info - .downcast_ref::() - .expect("invalid MachineStop payload"); - match info { - TerminationInfo::Exit(code) => return Some(*code), - TerminationInfo::PoppedTrackedPointerTag(item) => - format!("popped tracked tag for item {:?}", item), - TerminationInfo::Abort => - format!("the evaluated program aborted execution"), - } - } - 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(), - }; - e.print_backtrace(); - if let Some(frame) = ecx.stack().last() { - let span = frame.current_source_info().unwrap().span; + Err(e) => report_err(&ecx, e), + } +} - let msg = format!("Miri evaluation error: {}", msg); - let mut err = ecx.tcx.sess.struct_span_err(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.emit(); - } else { - ecx.tcx.sess.err(&msg); +fn report_err<'tcx, 'mir>( + ecx: &InterpCx<'mir, 'tcx, Evaluator<'tcx>>, + mut e: InterpErrorInfo<'tcx>, +) -> Option { + // Special treatment for some error kinds + let msg = match e.kind { + InterpError::MachineStop(ref info) => { + let info = info.downcast_ref::().expect("invalid MachineStop payload"); + match info { + TerminationInfo::Exit(code) => return Some(*code), + TerminationInfo::PoppedTrackedPointerTag(item) => + format!("popped tracked tag for item {:?}", item), + TerminationInfo::Abort => format!("the evaluated program aborted execution"), } + } + 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(), + }; + e.print_backtrace(); + if let Some(frame) = ecx.stack().last() { + let span = frame.current_source_info().unwrap().span; - for (i, frame) in ecx.stack().iter().enumerate() { - trace!("-------------------"); - trace!("Frame {}", i); - trace!(" return: {:?}", frame.return_place.map(|p| *p)); - for (i, local) in frame.locals.iter().enumerate() { - trace!(" local {}: {:?}", i, local.value); - } + let msg = format!("Miri evaluation error: {}", msg); + let mut err = ecx.tcx.sess.struct_span_err(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()); } - // Let the reported error determine the return code. - return None; + } + err.emit(); + } else { + ecx.tcx.sess.err(&msg); + } + + for (i, frame) in ecx.stack().iter().enumerate() { + trace!("-------------------"); + trace!("Frame {}", i); + trace!(" return: {:?}", frame.return_place.map(|p| *p)); + for (i, local) in frame.locals.iter().enumerate() { + trace!(" local {}: {:?}", i, local.value); } } + // Let the reported error determine the return code. + return None; } From 2673ba99fd28abc8448159a51d288ec942e7a65e Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Tue, 24 Dec 2019 00:08:47 +0100 Subject: [PATCH 02/12] Trailing return --- src/eval.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/eval.rs b/src/eval.rs index b2f290414c..9c15fedca3 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -204,7 +204,7 @@ pub fn eval_main<'tcx>(tcx: TyCtxt<'tcx>, main_id: DefId, config: MiriConfig) -> return None; } } - return Some(return_code); + Some(return_code) } Err(e) => report_err(&ecx, e), } From 4de031b3da1c3dc8091a64daa46322ced3796c0f Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Tue, 24 Dec 2019 00:11:40 +0100 Subject: [PATCH 03/12] Move error reporting to its own module --- src/diagnostics.rs | 62 ++++++++++++++++++++++++++++++++++++++++++++++ src/eval.rs | 60 -------------------------------------------- src/lib.rs | 2 ++ 3 files changed, 64 insertions(+), 60 deletions(-) create mode 100644 src/diagnostics.rs diff --git a/src/diagnostics.rs b/src/diagnostics.rs new file mode 100644 index 0000000000..30be49ff77 --- /dev/null +++ b/src/diagnostics.rs @@ -0,0 +1,62 @@ +use rustc_mir::interpret::InterpErrorInfo; + +use crate::*; + +pub fn report_err<'tcx, 'mir>( + ecx: &InterpCx<'mir, 'tcx, Evaluator<'tcx>>, + mut e: InterpErrorInfo<'tcx>, +) -> Option { + // Special treatment for some error kinds + let msg = match e.kind { + InterpError::MachineStop(ref info) => { + let info = info.downcast_ref::().expect("invalid MachineStop payload"); + match info { + TerminationInfo::Exit(code) => return Some(*code), + TerminationInfo::PoppedTrackedPointerTag(item) => + format!("popped tracked tag for item {:?}", item), + TerminationInfo::Abort => format!("the evaluated program aborted execution"), + } + } + 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(), + }; + e.print_backtrace(); + if let Some(frame) = ecx.stack().last() { + let span = frame.current_source_info().unwrap().span; + + let msg = format!("Miri evaluation error: {}", msg); + let mut err = ecx.tcx.sess.struct_span_err(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.emit(); + } else { + ecx.tcx.sess.err(&msg); + } + + for (i, frame) in ecx.stack().iter().enumerate() { + trace!("-------------------"); + trace!("Frame {}", i); + trace!(" return: {:?}", frame.return_place.map(|p| *p)); + for (i, local) in frame.locals.iter().enumerate() { + trace!(" local {}: {:?}", i, local.value); + } + } + // Let the reported error determine the return code. + return None; +} diff --git a/src/eval.rs b/src/eval.rs index 9c15fedca3..e5dfbe32c1 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -8,7 +8,6 @@ use rand::SeedableRng; use rustc_hir::def_id::DefId; use rustc::ty::layout::{LayoutOf, Size}; use rustc::ty::{self, TyCtxt}; -use rustc_mir::interpret::InterpErrorInfo; use crate::*; @@ -209,62 +208,3 @@ pub fn eval_main<'tcx>(tcx: TyCtxt<'tcx>, main_id: DefId, config: MiriConfig) -> Err(e) => report_err(&ecx, e), } } - -fn report_err<'tcx, 'mir>( - ecx: &InterpCx<'mir, 'tcx, Evaluator<'tcx>>, - mut e: InterpErrorInfo<'tcx>, -) -> Option { - // Special treatment for some error kinds - let msg = match e.kind { - InterpError::MachineStop(ref info) => { - let info = info.downcast_ref::().expect("invalid MachineStop payload"); - match info { - TerminationInfo::Exit(code) => return Some(*code), - TerminationInfo::PoppedTrackedPointerTag(item) => - format!("popped tracked tag for item {:?}", item), - TerminationInfo::Abort => format!("the evaluated program aborted execution"), - } - } - 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(), - }; - e.print_backtrace(); - if let Some(frame) = ecx.stack().last() { - let span = frame.current_source_info().unwrap().span; - - let msg = format!("Miri evaluation error: {}", msg); - let mut err = ecx.tcx.sess.struct_span_err(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.emit(); - } else { - ecx.tcx.sess.err(&msg); - } - - for (i, frame) in ecx.stack().iter().enumerate() { - trace!("-------------------"); - trace!("Frame {}", i); - trace!(" return: {:?}", frame.return_place.map(|p| *p)); - for (i, local) in frame.locals.iter().enumerate() { - trace!(" local {}: {:?}", i, local.value); - } - } - // Let the reported error determine the return code. - return None; -} diff --git a/src/lib.rs b/src/lib.rs index 19a84db3e1..b5925bb26c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -16,6 +16,7 @@ extern crate rustc_data_structures; extern crate rustc_mir; extern crate rustc_target; +mod diagnostics; mod eval; mod helpers; mod intptrcast; @@ -41,6 +42,7 @@ pub use crate::shims::time::EvalContextExt as TimeEvalContextExt; pub use crate::shims::tls::{EvalContextExt as TlsEvalContextExt, TlsData}; pub use crate::shims::EvalContextExt as ShimsEvalContextExt; +pub use crate::diagnostics::report_err; pub use crate::eval::{create_ecx, eval_main, MiriConfig, TerminationInfo}; pub use crate::helpers::EvalContextExt as HelpersEvalContextExt; pub use crate::machine::{ From 4411903cca01933cc98ac903b3864768b6136024 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Tue, 24 Dec 2019 00:22:32 +0100 Subject: [PATCH 04/12] Add a scheme for registering and obtaining errors even without access to an `InterpCx` --- src/diagnostics.rs | 17 +++++++++++++++++ src/lib.rs | 2 +- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/diagnostics.rs b/src/diagnostics.rs index 30be49ff77..2431d9230f 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -60,3 +60,20 @@ pub fn report_err<'tcx, 'mir>( // Let the reported error determine the return code. return None; } + +use std::cell::RefCell; +thread_local! { + static ECX: RefCell>> = RefCell::new(Vec::new()); +} + +pub fn register_err(e: InterpErrorInfo<'static>) { + ECX.with(|ecx| ecx.borrow_mut().push(e)); +} + +pub fn process_errors(mut f: impl FnMut(InterpErrorInfo<'static>)) { + ECX.with(|ecx| { + for e in ecx.borrow_mut().drain(..) { + f(e); + } + }); +} diff --git a/src/lib.rs b/src/lib.rs index b5925bb26c..bf6b111754 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -42,7 +42,7 @@ pub use crate::shims::time::EvalContextExt as TimeEvalContextExt; pub use crate::shims::tls::{EvalContextExt as TlsEvalContextExt, TlsData}; pub use crate::shims::EvalContextExt as ShimsEvalContextExt; -pub use crate::diagnostics::report_err; +pub use crate::diagnostics::{process_errors, register_err, report_err}; pub use crate::eval::{create_ecx, eval_main, MiriConfig, TerminationInfo}; pub use crate::helpers::EvalContextExt as HelpersEvalContextExt; pub use crate::machine::{ From 96d6efdf32f346c45f58897b2f7fff38a476cfbe Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Tue, 24 Dec 2019 02:04:07 +0100 Subject: [PATCH 05/12] Emit errors without halting interpretation --- src/diagnostics.rs | 16 ++++++++++------ src/lib.rs | 4 +++- src/stacked_borrows.rs | 15 ++++++++++----- 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/src/diagnostics.rs b/src/diagnostics.rs index 2431d9230f..504863b134 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -70,10 +70,14 @@ pub fn register_err(e: InterpErrorInfo<'static>) { ECX.with(|ecx| ecx.borrow_mut().push(e)); } -pub fn process_errors(mut f: impl FnMut(InterpErrorInfo<'static>)) { - ECX.with(|ecx| { - for e in ecx.borrow_mut().drain(..) { - f(e); - } - }); +impl<'mir, 'tcx> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} +pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { + fn process_errors(&self) { + let this = self.eval_context_ref(); + ECX.with(|ecx| { + for e in ecx.borrow_mut().drain(..) { + report_err(this, e); + } + }); + } } diff --git a/src/lib.rs b/src/lib.rs index bf6b111754..ae92a777d8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -42,7 +42,9 @@ pub use crate::shims::time::EvalContextExt as TimeEvalContextExt; pub use crate::shims::tls::{EvalContextExt as TlsEvalContextExt, TlsData}; pub use crate::shims::EvalContextExt as ShimsEvalContextExt; -pub use crate::diagnostics::{process_errors, register_err, report_err}; +pub use crate::diagnostics::{ + register_err, report_err, EvalContextExt as DiagnosticsEvalContextExt, +}; pub use crate::eval::{create_ecx, eval_main, MiriConfig, TerminationInfo}; pub use crate::helpers::EvalContextExt as HelpersEvalContextExt; pub use crate::machine::{ diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index 53a94e74cb..3d446c6b46 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -10,11 +10,9 @@ use std::rc::Rc; use rustc_hir::Mutability; use rustc::mir::RetagKind; use rustc::ty::{self, layout::Size}; +use rustc_mir::interpret::InterpError; -use crate::{ - AllocId, HelpersEvalContextExt, ImmTy, Immediate, InterpResult, MPlaceTy, MemoryKind, - MiriMemoryKind, PlaceTy, Pointer, RangeMap, TerminationInfo, -}; +use crate::*; pub type PtrId = NonZeroU64; pub type CallId = NonZeroU64; @@ -269,7 +267,12 @@ impl<'tcx> Stack { fn check_protector(item: &Item, tag: Option, global: &GlobalState) -> InterpResult<'tcx> { if let Tag::Tagged(id) = item.tag { if Some(id) == global.tracked_pointer_tag { - throw_machine_stop!(TerminationInfo::PoppedTrackedPointerTag(item.clone())); + register_err( + InterpError::MachineStop(Box::new(TerminationInfo::PoppedTrackedPointerTag( + item.clone(), + ))) + .into(), + ); } } if let Some(call) = item.protector { @@ -630,6 +633,8 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.write_immediate(val, place)?; } + this.process_errors(); + Ok(()) } } From bb58e42da2315c30594222bccb64b8a63fb537be Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Tue, 24 Dec 2019 02:16:43 +0100 Subject: [PATCH 06/12] Tell the user about stacked borrow debugging flags --- src/stacked_borrows.rs | 44 +++++++++++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 13 deletions(-) diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index 3d446c6b46..77295e8dd5 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -4,6 +4,7 @@ use std::cell::RefCell; use std::collections::{HashMap, HashSet}; use std::fmt; +use std::fmt::Write; use std::num::NonZeroU64; use std::rc::Rc; @@ -278,10 +279,13 @@ impl<'tcx> Stack { if let Some(call) = item.protector { if global.is_active(call) { if let Some(tag) = tag { - throw_ub!(UbExperimental(format!( - "not granting access to tag {:?} because incompatible item is protected: {:?}", - tag, item - ))); + return Err(err_ub_experimental( + tag, + format!( + "not granting access to tag {:?} because incompatible item is protected: {:?}", + tag, item + ), + )); } else { throw_ub!(UbExperimental(format!( "deallocating while item is protected: {:?}", @@ -300,10 +304,10 @@ impl<'tcx> Stack { // Step 1: Find granting item. let granting_idx = self.find_granting(access, tag).ok_or_else(|| { - err_ub!(UbExperimental(format!( - "no item granting {} to tag {:?} found in borrow stack", - access, tag, - ))) + err_ub_experimental( + tag, + 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 @@ -344,10 +348,11 @@ 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_ub_experimental( + tag,format!( "no item granting write access for deallocation to tag {:?} found in borrow stack", tag, - ))) + )) })?; // Step 2: Remove all items. Also checks for protectors. @@ -369,9 +374,14 @@ 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!( - "trying to reborrow for {:?}, but parent tag {:?} does not have an appropriate item in the borrow stack", new.perm, derived_from, - ))))?; + .ok_or_else(|| + err_ub_experimental( + derived_from, + 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 @@ -638,3 +648,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx Ok(()) } } + +fn err_ub_experimental(tag: Tag, mut msg: String) -> InterpErrorInfo<'static> { + if let Tag::Tagged(id) = tag { + // FIXME: do not add this message when the flag is already set + write!(msg, " Rerun with `-Zmiri-track-pointer-tag={}` for more information", id).unwrap(); + } + err_ub!(UbExperimental(msg)).into() +} From aec175e0deb39d35a9b5c0f329141c4b22930e01 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Wed, 8 Jan 2020 12:49:46 +0100 Subject: [PATCH 07/12] Process delayed errors on every step --- src/eval.rs | 4 +++- src/stacked_borrows.rs | 2 -- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/eval.rs b/src/eval.rs index e5dfbe32c1..1e9332c9a3 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -183,7 +183,9 @@ pub fn eval_main<'tcx>(tcx: TyCtxt<'tcx>, main_id: DefId, config: MiriConfig) -> // Perform the main execution. let res: InterpResult<'_, i64> = (|| { - ecx.run()?; + while ecx.step()? { + ecx.process_errors(); + } // Read the return code pointer *before* we run TLS destructors, to assert // that it was written to by the time that `start` lang item returned. let return_code = ecx.read_scalar(ret_place.into())?.not_undef()?.to_machine_isize(&ecx)?; diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index 77295e8dd5..ace4fef3ce 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -643,8 +643,6 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx this.write_immediate(val, place)?; } - this.process_errors(); - Ok(()) } } From c0a7fd56021a375478bd9e202bf5692e4e48736a Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Wed, 8 Jan 2020 12:50:15 +0100 Subject: [PATCH 08/12] Remove debugging hint until we can actuall use `note:` --- src/stacked_borrows.rs | 51 +++++++++++++----------------------------- 1 file changed, 15 insertions(+), 36 deletions(-) diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index ace4fef3ce..7bb7dd6cec 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -4,7 +4,6 @@ use std::cell::RefCell; use std::collections::{HashMap, HashSet}; use std::fmt; -use std::fmt::Write; use std::num::NonZeroU64; use std::rc::Rc; @@ -279,13 +278,10 @@ impl<'tcx> Stack { if let Some(call) = item.protector { if global.is_active(call) { if let Some(tag) = tag { - return Err(err_ub_experimental( - tag, - format!( - "not granting access to tag {:?} because incompatible item is protected: {:?}", - tag, item - ), - )); + throw_ub!(UbExperimental(format!( + "not granting access to tag {:?} because incompatible item is protected: {:?}", + tag, item + ))); } else { throw_ub!(UbExperimental(format!( "deallocating while item is protected: {:?}", @@ -303,12 +299,9 @@ impl<'tcx> Stack { // Two main steps: Find granting item, remove incompatible items above. // Step 1: Find granting item. - let granting_idx = self.find_granting(access, tag).ok_or_else(|| { - err_ub_experimental( - tag, - format!("no item granting {} to tag {:?} found in borrow stack.", access, tag), - ) - })?; + let granting_idx = self.find_granting(access, tag).ok_or_else(|| err_ub!(UbExperimental( + 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 // items. Behavior differs for reads and writes. @@ -347,13 +340,10 @@ impl<'tcx> Stack { /// active protectors at all because we will remove all items. 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_experimental( - tag,format!( - "no item granting write access for deallocation to tag {:?} found in borrow stack", - tag, - )) - })?; + self.find_granting(AccessKind::Write, tag).ok_or_else(|| err_ub!(UbExperimental(format!( + "no item granting write access for deallocation to tag {:?} found in borrow stack", + tag, + ))))?; // Step 2: Remove all items. Also checks for protectors. for item in self.borrows.drain(..).rev() { @@ -374,14 +364,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_experimental( - derived_from, - format!( - "trying to reborrow for {:?}, but parent tag {:?} does not have an appropriate item in the borrow stack", - new.perm, derived_from, - ), - ))?; + .ok_or_else(|| err_ub!(UbExperimental(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 @@ -647,10 +633,3 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx } } -fn err_ub_experimental(tag: Tag, mut msg: String) -> InterpErrorInfo<'static> { - if let Tag::Tagged(id) = tag { - // FIXME: do not add this message when the flag is already set - write!(msg, " Rerun with `-Zmiri-track-pointer-tag={}` for more information", id).unwrap(); - } - err_ub!(UbExperimental(msg)).into() -} From 90a8f2f6a300505c528cf5afe8ead9c31c9bbfca Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Wed, 8 Jan 2020 13:02:55 +0100 Subject: [PATCH 09/12] Make the non-halting diagnostic scheme independent of `InterpError` --- src/diagnostics.rs | 34 ++++++++++++++++++++++++++-------- src/eval.rs | 1 - src/lib.rs | 2 +- src/stacked_borrows.rs | 8 +------- 4 files changed, 28 insertions(+), 17 deletions(-) diff --git a/src/diagnostics.rs b/src/diagnostics.rs index 504863b134..cf27a9c376 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -1,7 +1,12 @@ use rustc_mir::interpret::InterpErrorInfo; +use std::cell::RefCell; use crate::*; +pub enum NonHaltingDiagnostic { + PoppedTrackedPointerTag(Item), +} + pub fn report_err<'tcx, 'mir>( ecx: &InterpCx<'mir, 'tcx, Evaluator<'tcx>>, mut e: InterpErrorInfo<'tcx>, @@ -12,8 +17,6 @@ pub fn report_err<'tcx, 'mir>( let info = info.downcast_ref::().expect("invalid MachineStop payload"); match info { TerminationInfo::Exit(code) => return Some(*code), - TerminationInfo::PoppedTrackedPointerTag(item) => - format!("popped tracked tag for item {:?}", item), TerminationInfo::Abort => format!("the evaluated program aborted execution"), } } @@ -25,11 +28,23 @@ pub fn report_err<'tcx, 'mir>( _ => e.to_string(), }; e.print_backtrace(); + report_msg(ecx, msg, true) +} + +pub fn report_msg<'tcx, 'mir>( + ecx: &InterpCx<'mir, 'tcx, Evaluator<'tcx>>, + msg: String, + error: bool, +) -> Option { if let Some(frame) = ecx.stack().last() { let span = frame.current_source_info().unwrap().span; - let msg = format!("Miri evaluation error: {}", msg); - let mut err = ecx.tcx.sess.struct_span_err(span, msg.as_str()); + let mut err = if error { + let msg = format!("Miri evaluation error: {}", msg); + ecx.tcx.sess.struct_span_err(span, msg.as_str()) + } 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). @@ -61,12 +76,11 @@ pub fn report_err<'tcx, 'mir>( return None; } -use std::cell::RefCell; thread_local! { - static ECX: RefCell>> = RefCell::new(Vec::new()); + static ECX: RefCell> = RefCell::new(Vec::new()); } -pub fn register_err(e: InterpErrorInfo<'static>) { +pub fn register_err(e: NonHaltingDiagnostic) { ECX.with(|ecx| ecx.borrow_mut().push(e)); } @@ -76,7 +90,11 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx let this = self.eval_context_ref(); ECX.with(|ecx| { for e in ecx.borrow_mut().drain(..) { - report_err(this, e); + let msg = match e { + NonHaltingDiagnostic::PoppedTrackedPointerTag(item) => + format!("popped tracked tag for item {:?}", item), + }; + report_msg(this, msg, false); } }); } diff --git a/src/eval.rs b/src/eval.rs index 1e9332c9a3..171f2627ad 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -33,7 +33,6 @@ pub struct MiriConfig { /// Details of premature program termination. pub enum TerminationInfo { Exit(i64), - PoppedTrackedPointerTag(Item), Abort, } diff --git a/src/lib.rs b/src/lib.rs index ae92a777d8..60e5217721 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -43,7 +43,7 @@ pub use crate::shims::tls::{EvalContextExt as TlsEvalContextExt, TlsData}; pub use crate::shims::EvalContextExt as ShimsEvalContextExt; pub use crate::diagnostics::{ - register_err, report_err, EvalContextExt as DiagnosticsEvalContextExt, + register_err, report_err, EvalContextExt as DiagnosticsEvalContextExt, NonHaltingDiagnostic, }; pub use crate::eval::{create_ecx, eval_main, MiriConfig, TerminationInfo}; pub use crate::helpers::EvalContextExt as HelpersEvalContextExt; diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index 7bb7dd6cec..a98c8e26c6 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -10,7 +10,6 @@ use std::rc::Rc; use rustc_hir::Mutability; use rustc::mir::RetagKind; use rustc::ty::{self, layout::Size}; -use rustc_mir::interpret::InterpError; use crate::*; @@ -267,12 +266,7 @@ impl<'tcx> Stack { fn check_protector(item: &Item, tag: Option, global: &GlobalState) -> InterpResult<'tcx> { if let Tag::Tagged(id) = item.tag { if Some(id) == global.tracked_pointer_tag { - register_err( - InterpError::MachineStop(Box::new(TerminationInfo::PoppedTrackedPointerTag( - item.clone(), - ))) - .into(), - ); + register_err(NonHaltingDiagnostic::PoppedTrackedPointerTag(item.clone())); } } if let Some(call) = item.protector { From c69ebaaed229679e54b70e82824ce82c31efa7fc Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Wed, 8 Jan 2020 13:20:39 +0100 Subject: [PATCH 10/12] Use names that actually represent what's going on --- src/diagnostics.rs | 12 ++++++------ src/eval.rs | 2 +- src/lib.rs | 2 +- src/stacked_borrows.rs | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/diagnostics.rs b/src/diagnostics.rs index cf27a9c376..a4f40d51a3 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -77,19 +77,19 @@ pub fn report_msg<'tcx, 'mir>( } thread_local! { - static ECX: RefCell> = RefCell::new(Vec::new()); + static DIAGNOSTICS: RefCell> = RefCell::new(Vec::new()); } -pub fn register_err(e: NonHaltingDiagnostic) { - ECX.with(|ecx| ecx.borrow_mut().push(e)); +pub fn register_diagnostic(e: NonHaltingDiagnostic) { + DIAGNOSTICS.with(|diagnostics| diagnostics.borrow_mut().push(e)); } impl<'mir, 'tcx> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { - fn process_errors(&self) { + fn process_diagnostics(&self) { let this = self.eval_context_ref(); - ECX.with(|ecx| { - for e in ecx.borrow_mut().drain(..) { + DIAGNOSTICS.with(|diagnostics| { + for e in diagnostics.borrow_mut().drain(..) { let msg = match e { NonHaltingDiagnostic::PoppedTrackedPointerTag(item) => format!("popped tracked tag for item {:?}", item), diff --git a/src/eval.rs b/src/eval.rs index 171f2627ad..e9e74db9b7 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -183,7 +183,7 @@ pub fn eval_main<'tcx>(tcx: TyCtxt<'tcx>, main_id: DefId, config: MiriConfig) -> // Perform the main execution. let res: InterpResult<'_, i64> = (|| { while ecx.step()? { - ecx.process_errors(); + ecx.process_diagnostics(); } // Read the return code pointer *before* we run TLS destructors, to assert // that it was written to by the time that `start` lang item returned. diff --git a/src/lib.rs b/src/lib.rs index 60e5217721..4d2411a669 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -43,7 +43,7 @@ pub use crate::shims::tls::{EvalContextExt as TlsEvalContextExt, TlsData}; pub use crate::shims::EvalContextExt as ShimsEvalContextExt; pub use crate::diagnostics::{ - register_err, report_err, EvalContextExt as DiagnosticsEvalContextExt, NonHaltingDiagnostic, + register_diagnostic, report_err, EvalContextExt as DiagnosticsEvalContextExt, NonHaltingDiagnostic, }; pub use crate::eval::{create_ecx, eval_main, MiriConfig, TerminationInfo}; pub use crate::helpers::EvalContextExt as HelpersEvalContextExt; diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index a98c8e26c6..6030450314 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -266,7 +266,7 @@ impl<'tcx> Stack { fn check_protector(item: &Item, tag: Option, global: &GlobalState) -> InterpResult<'tcx> { if let Tag::Tagged(id) = item.tag { if Some(id) == global.tracked_pointer_tag { - register_err(NonHaltingDiagnostic::PoppedTrackedPointerTag(item.clone())); + register_diagnostic(NonHaltingDiagnostic::PoppedTrackedPointerTag(item.clone())); } } if let Some(call) = item.protector { From bfc7a7effd8836765fc8bcb28084b90b29d8906c Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Thu, 9 Jan 2020 12:38:58 +0100 Subject: [PATCH 11/12] Remove trailing newline --- src/stacked_borrows.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/stacked_borrows.rs b/src/stacked_borrows.rs index 6030450314..1b7a118e63 100644 --- a/src/stacked_borrows.rs +++ b/src/stacked_borrows.rs @@ -626,4 +626,3 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx Ok(()) } } - From dbffbe52148ec0ef6cf5522b4171de40c93d4d65 Mon Sep 17 00:00:00 2001 From: Oliver Scherer Date: Thu, 9 Jan 2020 12:42:56 +0100 Subject: [PATCH 12/12] Document all the things --- src/diagnostics.rs | 9 ++++++++- src/eval.rs | 2 +- src/lib.rs | 2 +- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/diagnostics.rs b/src/diagnostics.rs index a4f40d51a3..e68dfad1b9 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -3,11 +3,13 @@ use std::cell::RefCell; use crate::*; +/// Miri specific diagnostics pub enum NonHaltingDiagnostic { PoppedTrackedPointerTag(Item), } -pub fn report_err<'tcx, 'mir>( +/// Emit a custom diagnostic without going through the miri-engine machinery +pub fn report_diagnostic<'tcx, 'mir>( ecx: &InterpCx<'mir, 'tcx, Evaluator<'tcx>>, mut e: InterpErrorInfo<'tcx>, ) -> Option { @@ -31,6 +33,8 @@ pub fn report_err<'tcx, 'mir>( report_msg(ecx, msg, 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, @@ -80,12 +84,15 @@ thread_local! { static DIAGNOSTICS: RefCell> = RefCell::new(Vec::new()); } +/// Schedule a diagnostic for emitting. This function works even if you have no `InterpCx` available. +/// The diagnostic will be emitted after the current interpreter step is finished. pub fn register_diagnostic(e: NonHaltingDiagnostic) { DIAGNOSTICS.with(|diagnostics| diagnostics.borrow_mut().push(e)); } impl<'mir, 'tcx> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {} pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> { + /// Emit all diagnostics that were registed with `register_diagnostics` fn process_diagnostics(&self) { let this = self.eval_context_ref(); DIAGNOSTICS.with(|diagnostics| { diff --git a/src/eval.rs b/src/eval.rs index e9e74db9b7..7a3945220f 100644 --- a/src/eval.rs +++ b/src/eval.rs @@ -206,6 +206,6 @@ pub fn eval_main<'tcx>(tcx: TyCtxt<'tcx>, main_id: DefId, config: MiriConfig) -> } Some(return_code) } - Err(e) => report_err(&ecx, e), + Err(e) => report_diagnostic(&ecx, e), } } diff --git a/src/lib.rs b/src/lib.rs index 4d2411a669..880a14b98c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -43,7 +43,7 @@ pub use crate::shims::tls::{EvalContextExt as TlsEvalContextExt, TlsData}; pub use crate::shims::EvalContextExt as ShimsEvalContextExt; pub use crate::diagnostics::{ - register_diagnostic, report_err, EvalContextExt as DiagnosticsEvalContextExt, NonHaltingDiagnostic, + register_diagnostic, report_diagnostic, EvalContextExt as DiagnosticsEvalContextExt, NonHaltingDiagnostic, }; pub use crate::eval::{create_ecx, eval_main, MiriConfig, TerminationInfo}; pub use crate::helpers::EvalContextExt as HelpersEvalContextExt;