Skip to content

Commit

Permalink
Auto merge of #68969 - RalfJung:dont-panic, r=oli-obk
Browse files Browse the repository at this point in the history 
remove Panic variant from InterpError

The interpreter engine itself does not raise `Panic` errors any more, so remove them from the error enum. Instead, const-prop and const-eval have to do their own handling of panics.

I used the opportunity to refactor the const-eval error handling a bit to use the `MachineStop` variant.

Also, in const-prop I could do some cleanup as now, no more lints are being reported in `use_ecx`. However, I am quite puzzled by why exactly the linting there works the way it does -- the code can certainly be cleaned up more, but I don't know enough of the intent to do that. I left some questions for the most confusing parts, but for now behavior should be unchanged by this PR (so, all that weirdness I am asking about is pre-existing and merely maintained here). Cc @wesleywiser

Fixes #66902

r? @oli-obk
  • Loading branch information
@bors
bors committed Feb 13, 2020
2 parents be493fe + 33ba83c commit d538b80
Show file tree
Hide file tree
Showing 17 changed files with 271 additions and 274 deletions.
146 changes: 51 additions & 95 deletions src/librustc/mir/interpret/error.rs
View file
Expand Up @@ -7,11 +7,9 @@ use crate::ty::query::TyCtxtAt;
use crate::ty::{self, layout, Ty};

use backtrace::Backtrace;
use hir::GeneratorKind;
use rustc_errors::{struct_span_err, DiagnosticBuilder};
use rustc_hir as hir;
use rustc_macros::HashStable;
use rustc_span::symbol::Symbol;
use rustc_span::{Pos, Span};
use rustc_target::spec::abi::Abi;
use std::{any::Any, env, fmt};
Expand Down Expand Up @@ -128,9 +126,15 @@ impl<'tcx> ConstEvalErr<'tcx> {
}
}

/// Sets the message passed in via `message` and adds span labels before handing control back
/// to `emit` to do any final processing. It's the caller's responsibility to call emit(),
/// stash(), etc. within the `emit` function to dispose of the diagnostic properly.
/// Create a diagnostic for this const eval error.
///
/// Sets the message passed in via `message` and adds span labels with detailed error
/// information before handing control back to `emit` to do any final processing.
/// It's the caller's responsibility to call emit(), stash(), etc. within the `emit`
/// function to dispose of the diagnostic properly.
///
/// If `lint_root.is_some()` report it as a lint, else report it as a hard error.
/// (Except that for some errors, we ignore all that -- see `must_error` below.)
fn struct_generic(
&self,
tcx: TyCtxtAt<'tcx>,
Expand All @@ -139,20 +143,30 @@ impl<'tcx> ConstEvalErr<'tcx> {
lint_root: Option<hir::HirId>,
) -> Result<(), ErrorHandled> {
let must_error = match self.error {
InterpError::MachineStop(_) => bug!("CTFE does not stop"),
err_inval!(Layout(LayoutError::Unknown(_))) | err_inval!(TooGeneric) => {
return Err(ErrorHandled::TooGeneric);
}
err_inval!(TypeckError) => return Err(ErrorHandled::Reported),
// We must *always* hard error on these, even if the caller wants just a lint.
err_inval!(Layout(LayoutError::SizeOverflow(_))) => true,
_ => false,
};
trace!("reporting const eval failure at {:?}", self.span);

let add_span_labels = |err: &mut DiagnosticBuilder<'_>| {
if !must_error {
err.span_label(self.span, self.error.to_string());
let err_msg = match &self.error {
InterpError::MachineStop(msg) => {
// A custom error (`ConstEvalErrKind` in `librustc_mir/interp/const_eval/error.rs`).
// Should be turned into a string by now.
msg.downcast_ref::<String>().expect("invalid MachineStop payload").clone()
}
err => err.to_string(),
};

let finish = |mut err: DiagnosticBuilder<'_>, span_msg: Option<String>| {
if let Some(span_msg) = span_msg {
err.span_label(self.span, span_msg);
}
// Add spans for the stacktrace.
// Skip the last, which is just the environment of the constant. The stacktrace
// is sometimes empty because we create "fake" eval contexts in CTFE to do work
// on constant values.
Expand All @@ -161,35 +175,37 @@ impl<'tcx> ConstEvalErr<'tcx> {
err.span_label(frame_info.call_site, frame_info.to_string());
}
}
// Let the caller finish the job.
emit(err)
};

if let (Some(lint_root), false) = (lint_root, must_error) {
let hir_id = self
.stacktrace
.iter()
.rev()
.filter_map(|frame| frame.lint_root)
.next()
.unwrap_or(lint_root);
tcx.struct_span_lint_hir(
rustc_session::lint::builtin::CONST_ERR,
hir_id,
tcx.span,
|lint| {
let mut err = lint.build(message);
add_span_labels(&mut err);
emit(err);
},
);
if must_error {
// The `message` makes little sense here, this is a more serious error than the
// caller thinks anyway.
// See <https://github.com/rust-lang/rust/pull/63152>.
finish(struct_error(tcx, &err_msg), None);
} else {
let mut err = if must_error {
struct_error(tcx, &self.error.to_string())
// Regular case.
if let Some(lint_root) = lint_root {
// Report as lint.
let hir_id = self
.stacktrace
.iter()
.rev()
.filter_map(|frame| frame.lint_root)
.next()
.unwrap_or(lint_root);
tcx.struct_span_lint_hir(
rustc_session::lint::builtin::CONST_ERR,
hir_id,
tcx.span,
|lint| finish(lint.build(message), Some(err_msg)),
);
} else {
struct_error(tcx, message)
};
add_span_labels(&mut err);
emit(err);
};
// Report as hard error.
finish(struct_error(tcx, message), Some(err_msg));
}
}
Ok(())
}
}
Expand Down Expand Up @@ -259,63 +275,6 @@ impl<'tcx> From<InterpError<'tcx>> for InterpErrorInfo<'tcx> {
}
}

#[derive(Clone, RustcEncodable, RustcDecodable, HashStable, PartialEq)]
pub enum PanicInfo<O> {
Panic { msg: Symbol, line: u32, col: u32, file: Symbol },
BoundsCheck { len: O, index: O },
Overflow(mir::BinOp),
OverflowNeg,
DivisionByZero,
RemainderByZero,
ResumedAfterReturn(GeneratorKind),
ResumedAfterPanic(GeneratorKind),
}

/// Type for MIR `Assert` terminator error messages.
pub type AssertMessage<'tcx> = PanicInfo<mir::Operand<'tcx>>;

impl<O> PanicInfo<O> {
/// Getting a description does not require `O` to be printable, and does not
/// require allocation.
/// The caller is expected to handle `Panic` and `BoundsCheck` separately.
pub fn description(&self) -> &'static str {
use PanicInfo::*;
match self {
Overflow(mir::BinOp::Add) => "attempt to add with overflow",
Overflow(mir::BinOp::Sub) => "attempt to subtract with overflow",
Overflow(mir::BinOp::Mul) => "attempt to multiply with overflow",
Overflow(mir::BinOp::Div) => "attempt to divide with overflow",
Overflow(mir::BinOp::Rem) => "attempt to calculate the remainder with overflow",
OverflowNeg => "attempt to negate with overflow",
Overflow(mir::BinOp::Shr) => "attempt to shift right with overflow",
Overflow(mir::BinOp::Shl) => "attempt to shift left with overflow",
Overflow(op) => bug!("{:?} cannot overflow", op),
DivisionByZero => "attempt to divide by zero",
RemainderByZero => "attempt to calculate the remainder with a divisor of zero",
ResumedAfterReturn(GeneratorKind::Gen) => "generator resumed after completion",
ResumedAfterReturn(GeneratorKind::Async(_)) => "`async fn` resumed after completion",
ResumedAfterPanic(GeneratorKind::Gen) => "generator resumed after panicking",
ResumedAfterPanic(GeneratorKind::Async(_)) => "`async fn` resumed after panicking",
Panic { .. } | BoundsCheck { .. } => bug!("Unexpected PanicInfo"),
}
}
}

impl<O: fmt::Debug> fmt::Debug for PanicInfo<O> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
use PanicInfo::*;
match self {
Panic { ref msg, line, col, ref file } => {
write!(f, "the evaluated program panicked at '{}', {}:{}:{}", msg, file, line, col)
}
BoundsCheck { ref len, ref index } => {
write!(f, "index out of bounds: the len is {:?} but the index is {:?}", len, index)
}
_ => write!(f, "{}", self.description()),
}
}
}

/// Error information for when the program we executed turned out not to actually be a valid
/// program. This cannot happen in stand-alone Miri, but it can happen during CTFE/ConstProp
/// where we work on generic code or execution does not have all information available.
Expand Down Expand Up @@ -616,8 +575,6 @@ impl fmt::Debug for ResourceExhaustionInfo {
}

pub enum InterpError<'tcx> {
/// The program panicked.
Panic(PanicInfo<u64>),
/// The program caused undefined behavior.
UndefinedBehavior(UndefinedBehaviorInfo),
/// The program did something the interpreter does not support (some of these *might* be UB
Expand Down Expand Up @@ -650,8 +607,7 @@ impl fmt::Debug for InterpError<'_> {
InvalidProgram(ref msg) => write!(f, "{:?}", msg),
UndefinedBehavior(ref msg) => write!(f, "{:?}", msg),
ResourceExhaustion(ref msg) => write!(f, "{:?}", msg),
Panic(ref msg) => write!(f, "{:?}", msg),
MachineStop(_) => write!(f, "machine caused execution to stop"),
MachineStop(_) => bug!("unhandled MachineStop"),
}
}
}
20 changes: 3 additions & 17 deletions src/librustc/mir/interpret/mod.rs
View file
Expand Up @@ -37,15 +37,6 @@ macro_rules! err_ub_format {
($($tt:tt)*) => { err_ub!(Ub(format!($($tt)*))) };
}

#[macro_export]
macro_rules! err_panic {
($($tt:tt)*) => {
$crate::mir::interpret::InterpError::Panic(
$crate::mir::interpret::PanicInfo::$($tt)*
)
};
}

#[macro_export]
macro_rules! err_exhaust {
($($tt:tt)*) => {
Expand Down Expand Up @@ -80,11 +71,6 @@ macro_rules! throw_ub_format {
($($tt:tt)*) => { throw_ub!(Ub(format!($($tt)*))) };
}

#[macro_export]
macro_rules! throw_panic {
($($tt:tt)*) => { return Err(err_panic!($($tt)*).into()) };
}

#[macro_export]
macro_rules! throw_exhaust {
($($tt:tt)*) => { return Err(err_exhaust!($($tt)*).into()) };
Expand All @@ -104,9 +90,9 @@ mod queries;
mod value;

pub use self::error::{
struct_error, AssertMessage, ConstEvalErr, ConstEvalRawResult, ConstEvalResult, ErrorHandled,
FrameInfo, InterpError, InterpErrorInfo, InterpResult, InvalidProgramInfo, PanicInfo,
ResourceExhaustionInfo, UndefinedBehaviorInfo, UnsupportedOpInfo,
struct_error, ConstEvalErr, ConstEvalRawResult, ConstEvalResult, ErrorHandled, FrameInfo,
InterpError, InterpErrorInfo, InterpResult, InvalidProgramInfo, ResourceExhaustionInfo,
UndefinedBehaviorInfo, UnsupportedOpInfo,
};

pub use self::value::{get_slice_bytes, ConstValue, RawConst, Scalar, ScalarMaybeUndef};
Expand Down
67 changes: 59 additions & 8 deletions src/librustc/mir/mod.rs
View file
Expand Up @@ -2,7 +2,7 @@
//!
//! [rustc guide]: https://rust-lang.github.io/rustc-guide/mir/index.html

use crate::mir::interpret::{GlobalAlloc, PanicInfo, Scalar};
use crate::mir::interpret::{GlobalAlloc, Scalar};
use crate::mir::visit::MirVisitable;
use crate::ty::adjustment::PointerCast;
use crate::ty::fold::{TypeFoldable, TypeFolder, TypeVisitor};
Expand Down Expand Up @@ -36,7 +36,6 @@ pub use syntax::ast::Mutability;
use syntax::ast::Name;

pub use self::cache::{BodyAndCache, ReadOnlyBodyAndCache};
pub use self::interpret::AssertMessage;
pub use self::query::*;
pub use crate::read_only;

Expand Down Expand Up @@ -1154,6 +1153,21 @@ pub enum TerminatorKind<'tcx> {
},
}

/// Information about an assertion failure.
#[derive(Clone, RustcEncodable, RustcDecodable, HashStable, PartialEq)]
pub enum AssertKind<O> {
BoundsCheck { len: O, index: O },
Overflow(BinOp),
OverflowNeg,
DivisionByZero,
RemainderByZero,
ResumedAfterReturn(GeneratorKind),
ResumedAfterPanic(GeneratorKind),
}

/// Type for MIR `Assert` terminator error messages.
pub type AssertMessage<'tcx> = AssertKind<Operand<'tcx>>;

pub type Successors<'a> =
iter::Chain<option::IntoIter<&'a BasicBlock>, slice::Iter<'a, BasicBlock>>;
pub type SuccessorsMut<'a> =
Expand Down Expand Up @@ -1383,6 +1397,45 @@ impl<'tcx> BasicBlockData<'tcx> {
}
}

impl<O> AssertKind<O> {
/// Getting a description does not require `O` to be printable, and does not
/// require allocation.
/// The caller is expected to handle `BoundsCheck` separately.
pub fn description(&self) -> &'static str {
use AssertKind::*;
match self {
Overflow(BinOp::Add) => "attempt to add with overflow",
Overflow(BinOp::Sub) => "attempt to subtract with overflow",
Overflow(BinOp::Mul) => "attempt to multiply with overflow",
Overflow(BinOp::Div) => "attempt to divide with overflow",
Overflow(BinOp::Rem) => "attempt to calculate the remainder with overflow",
OverflowNeg => "attempt to negate with overflow",
Overflow(BinOp::Shr) => "attempt to shift right with overflow",
Overflow(BinOp::Shl) => "attempt to shift left with overflow",
Overflow(op) => bug!("{:?} cannot overflow", op),
DivisionByZero => "attempt to divide by zero",
RemainderByZero => "attempt to calculate the remainder with a divisor of zero",
ResumedAfterReturn(GeneratorKind::Gen) => "generator resumed after completion",
ResumedAfterReturn(GeneratorKind::Async(_)) => "`async fn` resumed after completion",
ResumedAfterPanic(GeneratorKind::Gen) => "generator resumed after panicking",
ResumedAfterPanic(GeneratorKind::Async(_)) => "`async fn` resumed after panicking",
BoundsCheck { .. } => bug!("Unexpected AssertKind"),
}
}
}

impl<O: fmt::Debug> fmt::Debug for AssertKind<O> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
use AssertKind::*;
match self {
BoundsCheck { ref len, ref index } => {
write!(f, "index out of bounds: the len is {:?} but the index is {:?}", len, index)
}
_ => write!(f, "{}", self.description()),
}
}
}

impl<'tcx> Debug for TerminatorKind<'tcx> {
fn fmt(&self, fmt: &mut Formatter<'_>) -> fmt::Result {
self.fmt_head(fmt)?;
Expand Down Expand Up @@ -2666,13 +2719,12 @@ impl<'tcx> TypeFoldable<'tcx> for Terminator<'tcx> {
}
}
Assert { ref cond, expected, ref msg, target, cleanup } => {
use PanicInfo::*;
use AssertKind::*;
let msg = match msg {
BoundsCheck { ref len, ref index } => {
BoundsCheck { len: len.fold_with(folder), index: index.fold_with(folder) }
}
Panic { .. }
| Overflow(_)
Overflow(_)
| OverflowNeg
| DivisionByZero
| RemainderByZero
Expand Down Expand Up @@ -2716,13 +2768,12 @@ impl<'tcx> TypeFoldable<'tcx> for Terminator<'tcx> {
}
Assert { ref cond, ref msg, .. } => {
if cond.visit_with(visitor) {
use PanicInfo::*;
use AssertKind::*;
match msg {
BoundsCheck { ref len, ref index } => {
len.visit_with(visitor) || index.visit_with(visitor)
}
Panic { .. }
| Overflow(_)
Overflow(_)
| OverflowNeg
| DivisionByZero
| RemainderByZero
Expand Down
4 changes: 2 additions & 2 deletions src/librustc/mir/visit.rs
View file
Expand Up @@ -533,13 +533,13 @@ macro_rules! make_mir_visitor {
fn super_assert_message(&mut self,
msg: & $($mutability)? AssertMessage<'tcx>,
location: Location) {
use crate::mir::interpret::PanicInfo::*;
use crate::mir::AssertKind::*;
match msg {
BoundsCheck { len, index } => {
self.visit_operand(len, location);
self.visit_operand(index, location);
}
Panic { .. } | Overflow(_) | OverflowNeg | DivisionByZero | RemainderByZero |
Overflow(_) | OverflowNeg | DivisionByZero | RemainderByZero |
ResumedAfterReturn(_) | ResumedAfterPanic(_) => {
// Nothing to visit
}
Expand Down

0 comments on commit d538b80

Please sign in to comment.