Skip to content

Commit

Permalink
Auto merge of #33905 - eddyb:mir-overflow, r=nikomatsakis
Browse files Browse the repository at this point in the history
[MIR] Implement overflow checking

The initial set of changes is from @Aatch's #33255 PR, rebased on master, plus:

Added an `Assert` terminator to MIR, to simplify working with overflow and bounds checks.
With this terminator, error cases can be accounted for directly, instead of looking for lang item calls.
It also keeps the MIR slimmer, with no extra explicit blocks for the actual panic calls.

Warnings can be produced when the `Assert` is known to always panic at runtime, e.g.:
```rust
warning: index out of bounds: the len is 1 but the index is 3
 --> <anon>:1:14
1 |> fn main() { &[std::io::stdout()][3]; }
  |>              ^^^^^^^^^^^^^^^^^^^^^^
```

Generalized the `OperandValue::FatPtr` optimization to any aggregate pair of immediates.
This allows us to generate the same IR for overflow checks as old trans, not something worse.
For example, addition on `i16` calls `llvm.sadd.with.overflow.i16`, which returns `{i16, i1}`.
However, the Rust type `(i16, bool)`, has to be `{i16, i8}`, only an immediate `bool` is `i1`.
But if we split the pair into an `i16` and an `i1`, we can pass them around as such for free.

The latest addition is a rebase of #34054, updated to work for pairs too. Closes #34054, fixes #33873.

Last but not least, the `#[rustc_inherit_overflow_checks]` attribute was introduced to control the
overflow checking behavior of generic or `#[inline]` functions, when translated in another crate.

It is **not** intended to be used by crates other than `libcore`, which is in the unusual position of
being distributed as only an optimized build with no checks, even when used from debug mode.
Before MIR-based translation, this worked out fine, as the decision for overflow was made at
translation time, in the crate being compiled, but MIR stored in `rlib` has to contain the checks.

To avoid always generating the checks and slowing everything down, a decision was made to
use an attribute in the few spots of `libcore` that need it (see #33255 for previous discussion):
* `core::ops::{Add, Sub, Mul, Neg, Shl, Shr}` implementations for integers, which have `#[inline]` methods and can be used in generic abstractions from other crates
* `core::ops::{Add, Sub, Mul, Neg, Shl, Shr}Assign` same as above, for augmented assignment
* `pow` and `abs` methods on integers, which intentionally piggy-back on built-in multiplication and negation, respectively, to get overflow checks
* `core::iter::{Iterator, Chain, Peek}::count` and `core::iter::Enumerate::{next, nth}`, also documented as panicking on overflow, from addition, counting elements of an iterator in an `usize`
  • Loading branch information
bors committed Jun 5, 2016
2 parents 22b36c7 + cee244d commit 8cbffc5
Show file tree
Hide file tree
Showing 62 changed files with 1,363 additions and 431 deletions.
1 change: 1 addition & 0 deletions src/libcore/iter/iterator.rs
Expand Up @@ -172,6 +172,7 @@ pub trait Iterator {
/// assert_eq!(a.iter().count(), 5);
/// ```
#[inline]
#[rustc_inherit_overflow_checks]
#[stable(feature = "rust1", since = "1.0.0")]
fn count(self) -> usize where Self: Sized {
// Might overflow.
Expand Down
4 changes: 4 additions & 0 deletions src/libcore/iter/mod.rs
Expand Up @@ -510,6 +510,7 @@ impl<A, B> Iterator for Chain<A, B> where
}

#[inline]
#[rustc_inherit_overflow_checks]
fn count(self) -> usize {
match self.state {
ChainState::Both => self.a.count() + self.b.count(),
Expand Down Expand Up @@ -932,6 +933,7 @@ impl<I> Iterator for Enumerate<I> where I: Iterator {
///
/// Might panic if the index of the element overflows a `usize`.
#[inline]
#[rustc_inherit_overflow_checks]
fn next(&mut self) -> Option<(usize, <I as Iterator>::Item)> {
self.iter.next().map(|a| {
let ret = (self.count, a);
Expand All @@ -947,6 +949,7 @@ impl<I> Iterator for Enumerate<I> where I: Iterator {
}

#[inline]
#[rustc_inherit_overflow_checks]
fn nth(&mut self, n: usize) -> Option<(usize, I::Item)> {
self.iter.nth(n).map(|a| {
let i = self.count + n;
Expand Down Expand Up @@ -1008,6 +1011,7 @@ impl<I: Iterator> Iterator for Peekable<I> {
}

#[inline]
#[rustc_inherit_overflow_checks]
fn count(self) -> usize {
(if self.peeked.is_some() { 1 } else { 0 }) + self.iter.count()
}
Expand Down
6 changes: 3 additions & 3 deletions src/libcore/num/mod.rs
Expand Up @@ -1033,7 +1033,7 @@ macro_rules! int_impl {
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
#[inline]
#[rustc_no_mir] // FIXME #29769 MIR overflow checking is TBD.
#[rustc_inherit_overflow_checks]
pub fn pow(self, mut exp: u32) -> Self {
let mut base = self;
let mut acc = Self::one();
Expand Down Expand Up @@ -1075,7 +1075,7 @@ macro_rules! int_impl {
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
#[inline]
#[rustc_no_mir] // FIXME #29769 MIR overflow checking is TBD.
#[rustc_inherit_overflow_checks]
pub fn abs(self) -> Self {
if self.is_negative() {
// Note that the #[inline] above means that the overflow
Expand Down Expand Up @@ -2061,7 +2061,7 @@ macro_rules! uint_impl {
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
#[inline]
#[rustc_no_mir] // FIXME #29769 MIR overflow checking is TBD.
#[rustc_inherit_overflow_checks]
pub fn pow(self, mut exp: u32) -> Self {
let mut base = self;
let mut acc = Self::one();
Expand Down
11 changes: 11 additions & 0 deletions src/libcore/ops.rs
Expand Up @@ -208,6 +208,7 @@ macro_rules! add_impl {
type Output = $t;

#[inline]
#[rustc_inherit_overflow_checks]
fn add(self, other: $t) -> $t { self + other }
}

Expand Down Expand Up @@ -261,6 +262,7 @@ macro_rules! sub_impl {
type Output = $t;

#[inline]
#[rustc_inherit_overflow_checks]
fn sub(self, other: $t) -> $t { self - other }
}

Expand Down Expand Up @@ -314,6 +316,7 @@ macro_rules! mul_impl {
type Output = $t;

#[inline]
#[rustc_inherit_overflow_checks]
fn mul(self, other: $t) -> $t { self * other }
}

Expand Down Expand Up @@ -511,6 +514,7 @@ macro_rules! neg_impl_core {
type Output = $t;

#[inline]
#[rustc_inherit_overflow_checks]
fn neg(self) -> $t { let $id = self; $body }
}

Expand Down Expand Up @@ -788,6 +792,7 @@ macro_rules! shl_impl {
type Output = $t;

#[inline]
#[rustc_inherit_overflow_checks]
fn shl(self, other: $f) -> $t {
self << other
}
Expand Down Expand Up @@ -859,6 +864,7 @@ macro_rules! shr_impl {
type Output = $t;

#[inline]
#[rustc_inherit_overflow_checks]
fn shr(self, other: $f) -> $t {
self >> other
}
Expand Down Expand Up @@ -923,6 +929,7 @@ macro_rules! add_assign_impl {
#[stable(feature = "op_assign_traits", since = "1.8.0")]
impl AddAssign for $t {
#[inline]
#[rustc_inherit_overflow_checks]
fn add_assign(&mut self, other: $t) { *self += other }
}
)+)
Expand Down Expand Up @@ -967,6 +974,7 @@ macro_rules! sub_assign_impl {
#[stable(feature = "op_assign_traits", since = "1.8.0")]
impl SubAssign for $t {
#[inline]
#[rustc_inherit_overflow_checks]
fn sub_assign(&mut self, other: $t) { *self -= other }
}
)+)
Expand Down Expand Up @@ -1011,6 +1019,7 @@ macro_rules! mul_assign_impl {
#[stable(feature = "op_assign_traits", since = "1.8.0")]
impl MulAssign for $t {
#[inline]
#[rustc_inherit_overflow_checks]
fn mul_assign(&mut self, other: $t) { *self *= other }
}
)+)
Expand Down Expand Up @@ -1275,6 +1284,7 @@ macro_rules! shl_assign_impl {
#[stable(feature = "op_assign_traits", since = "1.8.0")]
impl ShlAssign<$f> for $t {
#[inline]
#[rustc_inherit_overflow_checks]
fn shl_assign(&mut self, other: $f) {
*self <<= other
}
Expand Down Expand Up @@ -1337,6 +1347,7 @@ macro_rules! shr_assign_impl {
#[stable(feature = "op_assign_traits", since = "1.8.0")]
impl ShrAssign<$f> for $t {
#[inline]
#[rustc_inherit_overflow_checks]
fn shr_assign(&mut self, other: $f) {
*self >>= other
}
Expand Down
61 changes: 60 additions & 1 deletion src/librustc/mir/repr.rs
Expand Up @@ -10,7 +10,7 @@

use graphviz::IntoCow;
use middle::const_val::ConstVal;
use rustc_const_math::{ConstUsize, ConstInt};
use rustc_const_math::{ConstUsize, ConstInt, ConstMathErr};
use hir::def_id::DefId;
use ty::subst::Substs;
use ty::{self, AdtDef, ClosureSubsts, FnOutput, Region, Ty};
Expand Down Expand Up @@ -354,6 +354,16 @@ pub enum TerminatorKind<'tcx> {
/// Cleanups to be done if the call unwinds.
cleanup: Option<BasicBlock>
},

/// Jump to the target if the condition has the expected value,
/// otherwise panic with a message and a cleanup target.
Assert {
cond: Operand<'tcx>,
expected: bool,
msg: AssertMessage<'tcx>,
target: BasicBlock,
cleanup: Option<BasicBlock>
}
}

impl<'tcx> Terminator<'tcx> {
Expand Down Expand Up @@ -389,6 +399,8 @@ impl<'tcx> TerminatorKind<'tcx> {
Drop { ref target, unwind: None, .. } => {
slice::ref_slice(target).into_cow()
}
Assert { target, cleanup: Some(unwind), .. } => vec![target, unwind].into_cow(),
Assert { ref target, .. } => slice::ref_slice(target).into_cow(),
}
}

Expand All @@ -413,6 +425,8 @@ impl<'tcx> TerminatorKind<'tcx> {
Drop { ref mut target, unwind: None, .. } => {
vec![target]
}
Assert { ref mut target, cleanup: Some(ref mut unwind), .. } => vec![target, unwind],
Assert { ref mut target, .. } => vec![target]
}
}
}
Expand Down Expand Up @@ -495,6 +509,26 @@ impl<'tcx> TerminatorKind<'tcx> {
}
write!(fmt, ")")
}
Assert { ref cond, expected, ref msg, .. } => {
write!(fmt, "assert(")?;
if !expected {
write!(fmt, "!")?;
}
write!(fmt, "{:?}, ", cond)?;

match *msg {
AssertMessage::BoundsCheck { ref len, ref index } => {
write!(fmt, "{:?}, {:?}, {:?}",
"index out of bounds: the len is {} but the index is {}",
len, index)?;
}
AssertMessage::Math(ref err) => {
write!(fmt, "{:?}", err.description())?;
}
}

write!(fmt, ")")
}
}
}

Expand Down Expand Up @@ -532,10 +566,21 @@ impl<'tcx> TerminatorKind<'tcx> {
Drop { unwind: Some(_), .. } => {
vec!["return".into_cow(), "unwind".into_cow()]
}
Assert { cleanup: None, .. } => vec!["".into()],
Assert { .. } =>
vec!["success".into_cow(), "unwind".into_cow()]
}
}
}

#[derive(Clone, Debug, RustcEncodable, RustcDecodable)]
pub enum AssertMessage<'tcx> {
BoundsCheck {
len: Operand<'tcx>,
index: Operand<'tcx>
},
Math(ConstMathErr)
}

///////////////////////////////////////////////////////////////////////////
// Statements
Expand Down Expand Up @@ -787,6 +832,7 @@ pub enum Rvalue<'tcx> {
Cast(CastKind, Operand<'tcx>, Ty<'tcx>),

BinaryOp(BinOp, Operand<'tcx>, Operand<'tcx>),
CheckedBinaryOp(BinOp, Operand<'tcx>, Operand<'tcx>),

UnaryOp(UnOp, Operand<'tcx>),

Expand Down Expand Up @@ -880,6 +926,16 @@ pub enum BinOp {
Gt,
}

impl BinOp {
pub fn is_checkable(self) -> bool {
use self::BinOp::*;
match self {
Add | Sub | Mul | Shl | Shr => true,
_ => false
}
}
}

#[derive(Copy, Clone, Debug, PartialEq, Eq, RustcEncodable, RustcDecodable)]
pub enum UnOp {
/// The `!` operator for logical inversion
Expand All @@ -898,6 +954,9 @@ impl<'tcx> Debug for Rvalue<'tcx> {
Len(ref a) => write!(fmt, "Len({:?})", a),
Cast(ref kind, ref lv, ref ty) => write!(fmt, "{:?} as {:?} ({:?})", lv, ty, kind),
BinaryOp(ref op, ref a, ref b) => write!(fmt, "{:?}({:?}, {:?})", op, a, b),
CheckedBinaryOp(ref op, ref a, ref b) => {
write!(fmt, "Checked{:?}({:?}, {:?})", op, a, b)
}
UnaryOp(ref op, ref a) => write!(fmt, "{:?}({:?})", op, a),
Box(ref t) => write!(fmt, "Box({:?})", t),
InlineAsm { ref asm, ref outputs, ref inputs } => {
Expand Down
7 changes: 7 additions & 0 deletions src/librustc/mir/tcx.rs
Expand Up @@ -183,6 +183,13 @@ impl<'a, 'gcx, 'tcx> Mir<'tcx> {
let rhs_ty = self.operand_ty(tcx, rhs);
Some(self.binop_ty(tcx, op, lhs_ty, rhs_ty))
}
Rvalue::CheckedBinaryOp(op, ref lhs, ref rhs) => {
let lhs_ty = self.operand_ty(tcx, lhs);
let rhs_ty = self.operand_ty(tcx, rhs);
let ty = self.binop_ty(tcx, op, lhs_ty, rhs_ty);
let ty = tcx.mk_tup(vec![ty, tcx.types.bool]);
Some(ty)
}
Rvalue::UnaryOp(_, ref operand) => {
Some(self.operand_ty(tcx, operand))
}
Expand Down
33 changes: 33 additions & 0 deletions src/librustc/mir/visit.rs
Expand Up @@ -127,6 +127,11 @@ macro_rules! make_mir_visitor {
self.super_terminator_kind(block, kind);
}

fn visit_assert_message(&mut self,
msg: & $($mutability)* AssertMessage<'tcx>) {
self.super_assert_message(msg);
}

fn visit_rvalue(&mut self,
rvalue: & $($mutability)* Rvalue<'tcx>) {
self.super_rvalue(rvalue);
Expand Down Expand Up @@ -426,6 +431,31 @@ macro_rules! make_mir_visitor {
}
cleanup.map(|t| self.visit_branch(block, t));
}

TerminatorKind::Assert { ref $($mutability)* cond,
expected: _,
ref $($mutability)* msg,
target,
cleanup } => {
self.visit_operand(cond);
self.visit_assert_message(msg);
self.visit_branch(block, target);
cleanup.map(|t| self.visit_branch(block, t));
}
}
}

fn super_assert_message(&mut self,
msg: & $($mutability)* AssertMessage<'tcx>) {
match *msg {
AssertMessage::BoundsCheck {
ref $($mutability)* len,
ref $($mutability)* index
} => {
self.visit_operand(len);
self.visit_operand(index);
}
AssertMessage::Math(_) => {}
}
}

Expand Down Expand Up @@ -461,6 +491,9 @@ macro_rules! make_mir_visitor {
}

Rvalue::BinaryOp(_bin_op,
ref $($mutability)* lhs,
ref $($mutability)* rhs) |
Rvalue::CheckedBinaryOp(_bin_op,
ref $($mutability)* lhs,
ref $($mutability)* rhs) => {
self.visit_operand(lhs);
Expand Down
3 changes: 2 additions & 1 deletion src/librustc_borrowck/borrowck/mir/dataflow/mod.rs
Expand Up @@ -450,13 +450,14 @@ impl<'a, 'tcx: 'a, D> DataflowAnalysis<'a, 'tcx, D>
repr::TerminatorKind::Return |
repr::TerminatorKind::Resume => {}
repr::TerminatorKind::Goto { ref target } |
repr::TerminatorKind::Assert { ref target, cleanup: None, .. } |
repr::TerminatorKind::Drop { ref target, location: _, unwind: None } |

repr::TerminatorKind::DropAndReplace {
ref target, value: _, location: _, unwind: None
} => {
self.propagate_bits_into_entry_set_for(in_out, changed, target);
}
repr::TerminatorKind::Assert { ref target, cleanup: Some(ref unwind), .. } |
repr::TerminatorKind::Drop { ref target, location: _, unwind: Some(ref unwind) } |
repr::TerminatorKind::DropAndReplace {
ref target, value: _, location: _, unwind: Some(ref unwind)
Expand Down
19 changes: 18 additions & 1 deletion src/librustc_borrowck/borrowck/mir/gather_moves.rs
Expand Up @@ -595,7 +595,8 @@ fn gather_moves<'a, 'tcx>(mir: &Mir<'tcx>, tcx: TyCtxt<'a, 'tcx, 'tcx>) -> MoveD
bb_ctxt.on_operand(SK::Repeat, operand, source),
Rvalue::Cast(ref _kind, ref operand, ref _ty) =>
bb_ctxt.on_operand(SK::Cast, operand, source),
Rvalue::BinaryOp(ref _binop, ref operand1, ref operand2) => {
Rvalue::BinaryOp(ref _binop, ref operand1, ref operand2) |
Rvalue::CheckedBinaryOp(ref _binop, ref operand1, ref operand2) => {
bb_ctxt.on_operand(SK::BinaryOp, operand1, source);
bb_ctxt.on_operand(SK::BinaryOp, operand2, source);
}
Expand Down Expand Up @@ -662,6 +663,22 @@ fn gather_moves<'a, 'tcx>(mir: &Mir<'tcx>, tcx: TyCtxt<'a, 'tcx, 'tcx>) -> MoveD
bb_ctxt.on_operand(SK::If, cond, source);
}

TerminatorKind::Assert {
ref cond, expected: _,
ref msg, target: _, cleanup: _
} => {
// The `cond` is always of (copyable) type `bool`,
// so there will never be anything to move.
let _ = cond;
match *msg {
AssertMessage:: BoundsCheck { ref len, ref index } => {
// Same for the usize length and index in bounds-checking.
let _ = (len, index);
}
AssertMessage::Math(_) => {}
}
}

TerminatorKind::SwitchInt { switch_ty: _, values: _, targets: _, ref discr } |
TerminatorKind::Switch { adt_def: _, targets: _, ref discr } => {
// The `discr` is not consumed; that is instead
Expand Down

0 comments on commit 8cbffc5

Please sign in to comment.