Skip to content

Commit

Permalink
Rollup merge of rust-lang#73513 - oli-obk:const_binop_overflow, r=est…
Browse files Browse the repository at this point in the history
…ebank

Show the values and computation that would overflow a const evaluation or propagation

Fixes rust-lang#71134

In contrast to the example in the issue it doesn't use individual spans for each operand. The effort required to implement that is quite high compared to the little (if at all) benefit it would bring to diagnostics.

cc @shepmaster

The way this is implemented it is also fairly easy to do the same for overflow panics at runtime, but that should be done in a separate PR since it may have runtime performance implications.
  • Loading branch information
Manishearth committed Jun 22, 2020
2 parents 3e98225 + 3b47280 commit 9916c09
Show file tree
Hide file tree
Showing 200 changed files with 1,280 additions and 995 deletions.
2 changes: 1 addition & 1 deletion src/librustc_codegen_ssa/mir/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
// checked operation, just a comparison with the minimum
// value, so we have to check for the assert message.
if !bx.check_overflow() {
if let AssertKind::OverflowNeg = *msg {
if let AssertKind::OverflowNeg(_) = *msg {
const_cond = Some(expected);
}
}
Expand Down
102 changes: 86 additions & 16 deletions src/librustc_middle/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1244,10 +1244,10 @@ pub enum TerminatorKind<'tcx> {
#[derive(Clone, RustcEncodable, RustcDecodable, HashStable, PartialEq)]
pub enum AssertKind<O> {
BoundsCheck { len: O, index: O },
Overflow(BinOp),
OverflowNeg,
DivisionByZero,
RemainderByZero,
Overflow(BinOp, O, O),
OverflowNeg(O),
DivisionByZero(O),
RemainderByZero(O),
ResumedAfterReturn(GeneratorKind),
ResumedAfterPanic(GeneratorKind),
}
Expand Down Expand Up @@ -1520,17 +1520,17 @@ impl<O> AssertKind<O> {
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",
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",
Expand All @@ -1544,12 +1544,54 @@ impl<O> AssertKind<O> {
where
O: Debug,
{
use AssertKind::*;
match self {
AssertKind::BoundsCheck { ref len, ref index } => write!(
BoundsCheck { ref len, ref index } => write!(
f,
"\"index out of bounds: the len is {{}} but the index is {{}}\", {:?}, {:?}",
len, index
),

OverflowNeg(op) => {
write!(f, "\"attempt to negate {{}} which would overflow\", {:?}", op)
}
DivisionByZero(op) => write!(f, "\"attempt to divide {{}} by zero\", {:?}", op),
RemainderByZero(op) => write!(
f,
"\"attempt to calculate the remainder of {{}} with a divisor of zero\", {:?}",
op
),
Overflow(BinOp::Add, l, r) => write!(
f,
"\"attempt to compute `{{}} + {{}}` which would overflow\", {:?}, {:?}",
l, r
),
Overflow(BinOp::Sub, l, r) => write!(
f,
"\"attempt to compute `{{}} - {{}}` which would overflow\", {:?}, {:?}",
l, r
),
Overflow(BinOp::Mul, l, r) => write!(
f,
"\"attempt to compute `{{}} * {{}}` which would overflow\", {:?}, {:?}",
l, r
),
Overflow(BinOp::Div, l, r) => write!(
f,
"\"attempt to compute `{{}} / {{}}` which would overflow\", {:?}, {:?}",
l, r
),
Overflow(BinOp::Rem, l, r) => write!(
f,
"\"attempt to compute the remainder of `{{}} % {{}}` which would overflow\", {:?}, {:?}",
l, r
),
Overflow(BinOp::Shr, _, r) => {
write!(f, "\"attempt to shift right by {{}} which would overflow\", {:?}", r)
}
Overflow(BinOp::Shl, _, r) => {
write!(f, "\"attempt to shift left by {{}} which would overflow\", {:?}", r)
}
_ => write!(f, "\"{}\"", self.description()),
}
}
Expand All @@ -1562,6 +1604,34 @@ impl<O: fmt::Debug> fmt::Debug for AssertKind<O> {
BoundsCheck { ref len, ref index } => {
write!(f, "index out of bounds: the len is {:?} but the index is {:?}", len, index)
}
OverflowNeg(op) => write!(f, "attempt to negate {:#?} which would overflow", op),
DivisionByZero(op) => write!(f, "attempt to divide {:#?} by zero", op),
RemainderByZero(op) => {
write!(f, "attempt to calculate the remainder of {:#?} with a divisor of zero", op)
}
Overflow(BinOp::Add, l, r) => {
write!(f, "attempt to compute `{:#?} + {:#?}` which would overflow", l, r)
}
Overflow(BinOp::Sub, l, r) => {
write!(f, "attempt to compute `{:#?} - {:#?}` which would overflow", l, r)
}
Overflow(BinOp::Mul, l, r) => {
write!(f, "attempt to compute `{:#?} * {:#?}` which would overflow", l, r)
}
Overflow(BinOp::Div, l, r) => {
write!(f, "attempt to compute `{:#?} / {:#?}` which would overflow", l, r)
}
Overflow(BinOp::Rem, l, r) => write!(
f,
"attempt to compute the remainder of `{:#?} % {:#?}` which would overflow",
l, r
),
Overflow(BinOp::Shr, _, r) => {
write!(f, "attempt to shift right by {:#?} which would overflow", r)
}
Overflow(BinOp::Shl, _, r) => {
write!(f, "attempt to shift left by {:#?} which would overflow", r)
}
_ => write!(f, "{}", self.description()),
}
}
Expand Down
24 changes: 11 additions & 13 deletions src/librustc_middle/mir/type_foldable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,14 @@ impl<'tcx> TypeFoldable<'tcx> for Terminator<'tcx> {
Assert { ref cond, expected, ref msg, target, cleanup } => {
use AssertKind::*;
let msg = match msg {
BoundsCheck { ref len, ref index } => {
BoundsCheck { len, index } => {
BoundsCheck { len: len.fold_with(folder), index: index.fold_with(folder) }
}
Overflow(_)
| OverflowNeg
| DivisionByZero
| RemainderByZero
| ResumedAfterReturn(_)
| ResumedAfterPanic(_) => msg.clone(),
Overflow(op, l, r) => Overflow(*op, l.fold_with(folder), r.fold_with(folder)),
OverflowNeg(op) => OverflowNeg(op.fold_with(folder)),
DivisionByZero(op) => DivisionByZero(op.fold_with(folder)),
RemainderByZero(op) => RemainderByZero(op.fold_with(folder)),
ResumedAfterReturn(_) | ResumedAfterPanic(_) => msg.clone(),
};
Assert { cond: cond.fold_with(folder), expected, msg, target, cleanup }
}
Expand Down Expand Up @@ -117,12 +116,11 @@ impl<'tcx> TypeFoldable<'tcx> for Terminator<'tcx> {
BoundsCheck { ref len, ref index } => {
len.visit_with(visitor) || index.visit_with(visitor)
}
Overflow(_)
| OverflowNeg
| DivisionByZero
| RemainderByZero
| ResumedAfterReturn(_)
| ResumedAfterPanic(_) => false,
Overflow(_, l, r) => l.visit_with(visitor) || r.visit_with(visitor),
OverflowNeg(op) | DivisionByZero(op) | RemainderByZero(op) => {
op.visit_with(visitor)
}
ResumedAfterReturn(_) | ResumedAfterPanic(_) => false,
}
} else {
false
Expand Down
8 changes: 7 additions & 1 deletion src/librustc_middle/mir/visit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,13 @@ macro_rules! make_mir_visitor {
self.visit_operand(len, location);
self.visit_operand(index, location);
}
Overflow(_) | OverflowNeg | DivisionByZero | RemainderByZero |
Overflow(_, l, r) => {
self.visit_operand(l, location);
self.visit_operand(r, location);
}
OverflowNeg(op) | DivisionByZero(op) | RemainderByZero(op) => {
self.visit_operand(op, location);
}
ResumedAfterReturn(_) | ResumedAfterPanic(_) => {
// Nothing to visit
}
Expand Down
111 changes: 111 additions & 0 deletions src/librustc_middle/ty/consts.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
use crate::mir::interpret::truncate;
use rustc_target::abi::Size;

#[derive(Copy, Clone)]
/// A type for representing any integer. Only used for printing.
// FIXME: Use this for the integer-tree representation needed for type level ints and
// const generics?
pub struct ConstInt {
/// Number of bytes of the integer. Only 1, 2, 4, 8, 16 are legal values.
size: u8,
/// Whether the value is of a signed integer type.
signed: bool,
/// Whether the value is a `usize` or `isize` type.
is_ptr_sized_integral: bool,
/// Raw memory of the integer. All bytes beyond the `size` are unused and must be zero.
raw: u128,
}

impl ConstInt {
pub fn new(raw: u128, size: Size, signed: bool, is_ptr_sized_integral: bool) -> Self {
assert!(raw <= truncate(u128::MAX, size));
Self { raw, size: size.bytes() as u8, signed, is_ptr_sized_integral }
}
}

impl std::fmt::Debug for ConstInt {
fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
let Self { size, signed, raw, is_ptr_sized_integral } = *self;
if signed {
let bit_size = size * 8;
let min = 1u128 << (bit_size - 1);
let max = min - 1;
if raw == min {
match (size, is_ptr_sized_integral) {
(_, true) => write!(fmt, "isize::MIN"),
(1, _) => write!(fmt, "i8::MIN"),
(2, _) => write!(fmt, "i16::MIN"),
(4, _) => write!(fmt, "i32::MIN"),
(8, _) => write!(fmt, "i64::MIN"),
(16, _) => write!(fmt, "i128::MIN"),
_ => bug!("ConstInt 0x{:x} with size = {} and signed = {}", raw, size, signed),
}
} else if raw == max {
match (size, is_ptr_sized_integral) {
(_, true) => write!(fmt, "isize::MAX"),
(1, _) => write!(fmt, "i8::MAX"),
(2, _) => write!(fmt, "i16::MAX"),
(4, _) => write!(fmt, "i32::MAX"),
(8, _) => write!(fmt, "i64::MAX"),
(16, _) => write!(fmt, "i128::MAX"),
_ => bug!("ConstInt 0x{:x} with size = {} and signed = {}", raw, size, signed),
}
} else {
match size {
1 => write!(fmt, "{}", raw as i8)?,
2 => write!(fmt, "{}", raw as i16)?,
4 => write!(fmt, "{}", raw as i32)?,
8 => write!(fmt, "{}", raw as i64)?,
16 => write!(fmt, "{}", raw as i128)?,
_ => bug!("ConstInt 0x{:x} with size = {} and signed = {}", raw, size, signed),
}
if fmt.alternate() {
match (size, is_ptr_sized_integral) {
(_, true) => write!(fmt, "_isize")?,
(1, _) => write!(fmt, "_i8")?,
(2, _) => write!(fmt, "_i16")?,
(4, _) => write!(fmt, "_i32")?,
(8, _) => write!(fmt, "_i64")?,
(16, _) => write!(fmt, "_i128")?,
_ => bug!(),
}
}
Ok(())
}
} else {
let max = truncate(u128::MAX, Size::from_bytes(size));
if raw == max {
match (size, is_ptr_sized_integral) {
(_, true) => write!(fmt, "usize::MAX"),
(1, _) => write!(fmt, "u8::MAX"),
(2, _) => write!(fmt, "u16::MAX"),
(4, _) => write!(fmt, "u32::MAX"),
(8, _) => write!(fmt, "u64::MAX"),
(16, _) => write!(fmt, "u128::MAX"),
_ => bug!("ConstInt 0x{:x} with size = {} and signed = {}", raw, size, signed),
}
} else {
match size {
1 => write!(fmt, "{}", raw as u8)?,
2 => write!(fmt, "{}", raw as u16)?,
4 => write!(fmt, "{}", raw as u32)?,
8 => write!(fmt, "{}", raw as u64)?,
16 => write!(fmt, "{}", raw as u128)?,
_ => bug!("ConstInt 0x{:x} with size = {} and signed = {}", raw, size, signed),
}
if fmt.alternate() {
match (size, is_ptr_sized_integral) {
(_, true) => write!(fmt, "_usize")?,
(1, _) => write!(fmt, "_u8")?,
(2, _) => write!(fmt, "_u16")?,
(4, _) => write!(fmt, "_u32")?,
(8, _) => write!(fmt, "_u64")?,
(16, _) => write!(fmt, "_u128")?,
_ => bug!(),
}
}
Ok(())
}
}
}
}
3 changes: 3 additions & 0 deletions src/librustc_middle/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,8 @@ pub use self::trait_def::TraitDef;

pub use self::query::queries;

pub use self::consts::ConstInt;

pub mod adjustment;
pub mod binding;
pub mod cast;
Expand All @@ -108,6 +110,7 @@ pub mod trait_def;
pub mod util;
pub mod walk;

mod consts;
mod context;
mod diagnostics;
mod instance;
Expand Down
37 changes: 7 additions & 30 deletions src/librustc_middle/ty/print/pretty.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
use crate::middle::cstore::{ExternCrate, ExternCrateSource};
use crate::mir::interpret::{
sign_extend, truncate, AllocId, ConstValue, GlobalAlloc, Pointer, Scalar,
};
use crate::mir::interpret::{AllocId, ConstValue, GlobalAlloc, Pointer, Scalar};
use crate::ty::layout::IntegerExt;
use crate::ty::subst::{GenericArg, GenericArgKind, Subst};
use crate::ty::{self, DefIdTree, ParamConst, Ty, TyCtxt, TypeFoldable};
use crate::ty::{self, ConstInt, DefIdTree, ParamConst, Ty, TyCtxt, TypeFoldable};
use rustc_apfloat::ieee::{Double, Single};
use rustc_apfloat::Float;
use rustc_ast::ast;
Expand Down Expand Up @@ -981,35 +979,14 @@ pub trait PrettyPrinter<'tcx>:
}
// Int
(Scalar::Raw { data, .. }, ty::Uint(ui)) => {
let bit_size = Integer::from_attr(&self.tcx(), UnsignedInt(*ui)).size();
let max = truncate(u128::MAX, bit_size);

let ui_str = ui.name_str();
if data == max {
p!(write("{}::MAX", ui_str))
} else {
if print_ty { p!(write("{}{}", data, ui_str)) } else { p!(write("{}", data)) }
};
let size = Integer::from_attr(&self.tcx(), UnsignedInt(*ui)).size();
let int = ConstInt::new(data, size, false, ty.is_ptr_sized_integral());
if print_ty { p!(write("{:#?}", int)) } else { p!(write("{:?}", int)) }
}
(Scalar::Raw { data, .. }, ty::Int(i)) => {
let size = Integer::from_attr(&self.tcx(), SignedInt(*i)).size();
let bit_size = size.bits() as u128;
let min = 1u128 << (bit_size - 1);
let max = min - 1;

let i_str = i.name_str();
match data {
d if d == min => p!(write("{}::MIN", i_str)),
d if d == max => p!(write("{}::MAX", i_str)),
_ => {
let data = sign_extend(data, size) as i128;
if print_ty {
p!(write("{}{}", data, i_str))
} else {
p!(write("{}", data))
}
}
}
let int = ConstInt::new(data, size, true, ty.is_ptr_sized_integral());
if print_ty { p!(write("{:#?}", int)) } else { p!(write("{:?}", int)) }
}
// Char
(Scalar::Raw { data, .. }, ty::Char) if char::from_u32(data as u32).is_some() => {
Expand Down
3 changes: 2 additions & 1 deletion src/librustc_mir/const_eval/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use std::error::Error;
use std::fmt;

use rustc_middle::mir::AssertKind;
use rustc_middle::ty::ConstInt;
use rustc_span::{Span, Symbol};

use super::InterpCx;
Expand All @@ -13,7 +14,7 @@ pub enum ConstEvalErrKind {
NeedsRfc(String),
ConstAccessesStatic,
ModifiedGlobal,
AssertFailure(AssertKind<u64>),
AssertFailure(AssertKind<ConstInt>),
Panic { msg: Symbol, line: u32, col: u32, file: Symbol },
}

Expand Down
Loading

0 comments on commit 9916c09

Please sign in to comment.