Skip to content

Commit

Permalink
Auto merge of #73513 - oli-obk:const_binop_overflow, r=estebank
Browse files Browse the repository at this point in the history
Show the values and computation that would overflow a const evaluation or propagation

Fixes #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
bors committed Jun 26, 2020
2 parents 9672b5e + 819cde5 commit 7750c3d
Show file tree
Hide file tree
Showing 202 changed files with 1,273 additions and 986 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 @@ -380,7 +380,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
104 changes: 88 additions & 16 deletions src/librustc_middle/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
//!
//! [rustc dev guide]: https://rustc-dev-guide.rust-lang.org/mir/index.html
// ignore-tidy-filelength

use crate::mir::interpret::{GlobalAlloc, Scalar};
use crate::mir::visit::MirVisitable;
use crate::ty::adjustment::PointerCast;
Expand Down Expand Up @@ -1246,10 +1248,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 @@ -1522,17 +1524,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 @@ -1546,12 +1548,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 @@ -1564,6 +1608,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
Loading

0 comments on commit 7750c3d

Please sign in to comment.