Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MIR] Implement overflow checking #33255

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/librustc/mir/repr.rs
Expand Up @@ -757,6 +757,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 @@ -850,6 +851,16 @@ pub enum BinOp {
Gt,
}

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

@nagisa nagisa Apr 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to me like a potential source of divergence: you could easily forget to add more operators here when implementing overflow elsewhere in the compiler for new ops. Perhaps we should have some central authority on which operators are overflow-checkable?

The central authority could also return false for cases when the overflow checks are disabled by a flag for example, and we could make the odd default branch in LLVM a bug! too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well they're really "can be used with CheckedBinaryOp".

I don't think divergence is a valid concern here though. There aren't any new cases where we're going to check for overflow. I only added this method to make the logic in as_rvalue a bit clearer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The _ could be expanded to all the various options to avoid divergence concerns.

Copy link
Member

@nagisa nagisa Apr 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I totally thought we are using BinOp from some higher level, but apparently MIR has its own enumeration.

Never mind me then.

_ => false
}
}
}

#[derive(Copy, Clone, Debug, PartialEq, Eq, RustcEncodable, RustcDecodable)]
pub enum UnOp {
/// The `!` operator for logical inversion
Expand All @@ -868,6 +879,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 @@ -189,6 +189,13 @@ impl<'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
3 changes: 3 additions & 0 deletions src/librustc/mir/visit.rs
Expand Up @@ -450,6 +450,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/gather_moves.rs
Expand Up @@ -543,7 +543,8 @@ fn gather_moves<'tcx>(mir: &Mir<'tcx>, tcx: &TyCtxt<'tcx>) -> MoveData<'tcx> {
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
19 changes: 19 additions & 0 deletions src/librustc_mir/build/block.rs
Expand Up @@ -12,6 +12,7 @@ use build::{BlockAnd, BlockAndExtension, Builder};
use hair::*;
use rustc::mir::repr::*;
use rustc::hir;
use syntax::codemap::Span;

impl<'a,'tcx> Builder<'a,'tcx> {
pub fn ast_block(&mut self,
Expand Down Expand Up @@ -79,4 +80,22 @@ impl<'a,'tcx> Builder<'a,'tcx> {
block.unit()
})
}

// Helper method for generating a conditional branch
// Returns (TrueBlock, FalseBlock)
pub fn build_cond_br(&mut self, block: BasicBlock, span: Span,
cond: Operand<'tcx>) -> (BasicBlock, BasicBlock) {
let scope_id = self.innermost_scope_id();

let then_block = self.cfg.start_new_block();
let else_block = self.cfg.start_new_block();

self.cfg.terminate(block, scope_id, span,
TerminatorKind::If {
cond: cond,
targets: (then_block, else_block)
});

(then_block, else_block)
}
}
170 changes: 169 additions & 1 deletion src/librustc_mir/build/expr/as_rvalue.rs
Expand Up @@ -10,12 +10,19 @@

//! See docs in build/expr/mod.rs

use std;

use rustc_data_structures::fnv::FnvHashMap;

use build::{BlockAnd, BlockAndExtension, Builder};
use build::expr::category::{Category, RvalueFunc};
use hair::*;
use rustc_const_math::{ConstInt, ConstIsize};
use rustc::middle::const_val::ConstVal;
use rustc::ty;
use rustc::mir::repr::*;
use syntax::ast;
use syntax::codemap::Span;

impl<'a,'tcx> Builder<'a,'tcx> {
/// Compile `expr`, yielding an rvalue.
Expand Down Expand Up @@ -66,10 +73,26 @@ impl<'a,'tcx> Builder<'a,'tcx> {
ExprKind::Binary { op, lhs, rhs } => {
let lhs = unpack!(block = this.as_operand(block, lhs));
let rhs = unpack!(block = this.as_operand(block, rhs));
block.and(Rvalue::BinaryOp(op, lhs, rhs))
this.build_binary_op(block, op, expr_span, expr.ty,
lhs, rhs)
}
ExprKind::Unary { op, arg } => {
let arg = unpack!(block = this.as_operand(block, arg));
// Check for -MIN on signed integers
if op == UnOp::Neg && this.check_overflow() && expr.ty.is_signed() {
let bool_ty = this.hir.bool_ty();

let minval = this.minval_literal(expr_span, expr.ty);
let is_min = this.temp(bool_ty);

this.cfg.push_assign(block, scope_id, expr_span, &is_min,
Rvalue::BinaryOp(BinOp::Eq, arg.clone(), minval));

let (of_block, ok_block) = this.build_cond_br(block, expr_span,
Operand::Consume(is_min));
this.panic(of_block, "attempted to negate with overflow", expr_span);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I imagine we might want some more structured form of terminators here for these special checks (but that needn't derail this PR).

block = ok_block;
}
block.and(Rvalue::UnaryOp(op, arg))
}
ExprKind::Box { value, value_extents } => {
Expand Down Expand Up @@ -221,4 +244,149 @@ impl<'a,'tcx> Builder<'a,'tcx> {
}
}
}

pub fn build_binary_op(&mut self, mut block: BasicBlock,
op: BinOp, span: Span, ty: ty::Ty<'tcx>,
lhs: Operand<'tcx>, rhs: Operand<'tcx>) -> BlockAnd<Rvalue<'tcx>> {
let scope_id = self.innermost_scope_id();
let bool_ty = self.hir.bool_ty();
if self.check_overflow() && op.is_checkable() && ty.is_integral() {
let result_tup = self.hir.tcx().mk_tup(vec![ty, bool_ty]);
let result_value = self.temp(result_tup);

self.cfg.push_assign(block, scope_id, span,
&result_value, Rvalue::CheckedBinaryOp(op,
lhs,
rhs));
let val_fld = Field::new(0);
let of_fld = Field::new(1);

let val = result_value.clone().field(val_fld, ty);
let of = result_value.field(of_fld, bool_ty);

let msg = if op == BinOp::Shl || op == BinOp::Shr {
"shift operation overflowed"
} else {
"arithmetic operation overflowed"
};

let (of_block, ok_block) = self.build_cond_br(block, span, Operand::Consume(of));
self.panic(of_block, msg, span);

ok_block.and(Rvalue::Use(Operand::Consume(val)))
} else {
Copy link
Member

@nagisa nagisa Apr 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn’t this branch ignore the check_overflow? Are we checking for division unconditionally in current trans?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me it seems like it would be cleaner to have something like

if !self.is_overflow_checked() || !op.is_checkable() || !ty.is_integral() { return block.and(Rvalue::BinaryOp(...) }

at the top, and then only do the branching based on the operator elsewhere in this function.

if ty.is_integral() && (op == BinOp::Div || op == BinOp::Rem) {
// Checking division and remainder is more complex, since we 1. always check
// and 2. there are two possible failure cases, divide-by-zero and overflow.

let (zero_msg, overflow_msg) = if op == BinOp::Div {
("attempted to divide by zero",
"attempted to divide with overflow")
} else {
("attempted remainder with a divisor of zero",
"attempted remainder with overflow")
};

// Check for / 0
let is_zero = self.temp(bool_ty);
let zero = self.zero_literal(span, ty);
self.cfg.push_assign(block, scope_id, span, &is_zero,
Rvalue::BinaryOp(BinOp::Eq, rhs.clone(), zero));

let (zero_block, ok_block) = self.build_cond_br(block, span,
Operand::Consume(is_zero));
self.panic(zero_block, zero_msg, span);

block = ok_block;

// We only need to check for the overflow in one case:
// MIN / -1, and only for signed values.
if ty.is_signed() {
let neg_1 = self.neg_1_literal(span, ty);
let min = self.minval_literal(span, ty);

let is_neg_1 = self.temp(bool_ty);
let is_min = self.temp(bool_ty);
let of = self.temp(bool_ty);

// this does (rhs == -1) & (lhs == MIN). It could short-circuit instead

self.cfg.push_assign(block, scope_id, span, &is_neg_1,
Rvalue::BinaryOp(BinOp::Eq, rhs.clone(), neg_1));
self.cfg.push_assign(block, scope_id, span, &is_min,
Rvalue::BinaryOp(BinOp::Eq, lhs.clone(), min));

let is_neg_1 = Operand::Consume(is_neg_1);
let is_min = Operand::Consume(is_min);
self.cfg.push_assign(block, scope_id, span, &of,
Rvalue::BinaryOp(BinOp::BitAnd, is_neg_1, is_min));

let (of_block, ok_block) = self.build_cond_br(block, span,
Operand::Consume(of));
self.panic(of_block, overflow_msg, span);

block = ok_block;
}
}

block.and(Rvalue::BinaryOp(op, lhs, rhs))
}
}

// Helper to get a `-1` value of the appropriate type
fn neg_1_literal(&mut self, span: Span, ty: ty::Ty<'tcx>) -> Operand<'tcx> {
let literal = match ty.sty {
ty::TyInt(ity) => {
let val = match ity {
ast::IntTy::I8 => ConstInt::I8(-1),
ast::IntTy::I16 => ConstInt::I16(-1),
ast::IntTy::I32 => ConstInt::I32(-1),
ast::IntTy::I64 => ConstInt::I64(-1),
ast::IntTy::Is => {
let int_ty = self.hir.tcx().sess.target.int_type;
let val = ConstIsize::new(-1, int_ty).unwrap();
ConstInt::Isize(val)
}
};

Literal::Value { value: ConstVal::Integral(val) }
}
_ => {
span_bug!(span, "Invalid type for neg_1_literal: `{:?}`", ty)
}
};

self.literal_operand(span, ty, literal)
}

// Helper to get the minimum value of the appropriate type
fn minval_literal(&mut self, span: Span, ty: ty::Ty<'tcx>) -> Operand<'tcx> {
let literal = match ty.sty {
ty::TyInt(ity) => {
let val = match ity {
ast::IntTy::I8 => ConstInt::I8(std::i8::MIN),
ast::IntTy::I16 => ConstInt::I16(std::i16::MIN),
ast::IntTy::I32 => ConstInt::I32(std::i32::MIN),
ast::IntTy::I64 => ConstInt::I64(std::i64::MIN),
ast::IntTy::Is => {
let int_ty = self.hir.tcx().sess.target.int_type;
let min = match int_ty {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can generate the correct ConstIsize variant directly, without casting to i64 and going through ConstIsize::new.

let val = match int_ty {
    ast::IntTy::I32 => ConstIsize::Is32(std::i32::MIN),
    ast::IntTy::I64 => ConstIsize::Is64(std::i64::MIN),
    _ => bug!(),
}

ast::IntTy::I32 => std::i32::MIN as i64,
ast::IntTy::I64 => std::i64::MIN,
_ => unreachable!()
};
let val = ConstIsize::new(min, int_ty).unwrap();
ConstInt::Isize(val)
}
};

Literal::Value { value: ConstVal::Integral(val) }
}
_ => {
span_bug!(span, "Invalid type for minval_literal: `{:?}`", ty)
}
};

self.literal_operand(span, ty, literal)
}
}
10 changes: 6 additions & 4 deletions src/librustc_mir/build/expr/stmt.rs
Expand Up @@ -67,17 +67,19 @@ impl<'a,'tcx> Builder<'a,'tcx> {
// only affects weird things like `x += {x += 1; x}`
// -- is that equal to `x + (x + 1)` or `2*(x+1)`?

let lhs = this.hir.mirror(lhs);
let lhs_ty = lhs.ty;

// As above, RTL.
let rhs = unpack!(block = this.as_operand(block, rhs));
let lhs = unpack!(block = this.as_lvalue(block, lhs));

// we don't have to drop prior contents or anything
// because AssignOp is only legal for Copy types
// (overloaded ops should be desugared into a call).
this.cfg.push_assign(block, scope_id, expr_span, &lhs,
Rvalue::BinaryOp(op,
Operand::Consume(lhs.clone()),
rhs));
let result = unpack!(block = this.build_binary_op(block, op, expr_span, lhs_ty,
Operand::Consume(lhs.clone()), rhs));
this.cfg.push_assign(block, scope_id, expr_span, &lhs, result);

block.unit()
}
Expand Down
54 changes: 53 additions & 1 deletion src/librustc_mir/build/misc.rs
Expand Up @@ -12,9 +12,14 @@
//! kind of thing.

use build::Builder;
use rustc::ty::Ty;

use rustc_const_math::{ConstInt, ConstUsize, ConstIsize};
use rustc::middle::const_val::ConstVal;
use rustc::ty::{self, Ty};

use rustc::mir::repr::*;
use std::u32;
use syntax::ast;
use syntax::codemap::Span;

impl<'a,'tcx> Builder<'a,'tcx> {
Expand Down Expand Up @@ -50,6 +55,53 @@ impl<'a,'tcx> Builder<'a,'tcx> {
Rvalue::Aggregate(AggregateKind::Tuple, vec![])
}

// Returns a zero literal operand for the appropriate type, works for
// bool, char, integers and floats.
pub fn zero_literal(&mut self, span: Span, ty: Ty<'tcx>) -> Operand<'tcx> {
let literal = match ty.sty {
ty::TyBool => {
self.hir.false_literal()
}
ty::TyChar => Literal::Value { value: ConstVal::Char('\0') },
ty::TyUint(ity) => {
let val = match ity {
ast::UintTy::U8 => ConstInt::U8(0),
ast::UintTy::U16 => ConstInt::U16(0),
ast::UintTy::U32 => ConstInt::U32(0),
ast::UintTy::U64 => ConstInt::U64(0),
ast::UintTy::Us => {
let uint_ty = self.hir.tcx().sess.target.uint_type;
let val = ConstUsize::new(0, uint_ty).unwrap();
ConstInt::Usize(val)
}
};

Literal::Value { value: ConstVal::Integral(val) }
}
ty::TyInt(ity) => {
let val = match ity {
ast::IntTy::I8 => ConstInt::I8(0),
ast::IntTy::I16 => ConstInt::I16(0),
ast::IntTy::I32 => ConstInt::I32(0),
ast::IntTy::I64 => ConstInt::I64(0),
ast::IntTy::Is => {
let int_ty = self.hir.tcx().sess.target.int_type;
let val = ConstIsize::new(0, int_ty).unwrap();
ConstInt::Isize(val)
}
};

Literal::Value { value: ConstVal::Integral(val) }
}
ty::TyFloat(_) => Literal::Value { value: ConstVal::Float(0.0) },
_ => {
span_bug!(span, "Invalid type for zero_literal: `{:?}`", ty)
}
};

self.literal_operand(span, ty, literal)
}

pub fn push_usize(&mut self,
block: BasicBlock,
scope_id: ScopeId,
Expand Down