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.

Sign up for GitHub

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

cleanup overflow binop code #28

Merged
merged 19 commits into from
Jun 20, 2016
Merged

cleanup overflow binop code #28

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
58b4fac
implement overflowing ops
oli-obk Jun 17, 2016
3ba4f6d
remove code repetition and fix overflowing intrinsics
oli-obk Jun 17, 2016
6376ef4
run the *compiled* run-pass tests on the host machine
oli-obk Jun 17, 2016
4f48bef
cfail test for std::env::args()
oli-obk Jun 17, 2016
e3a2bf8
clippy
oli-obk Jun 17, 2016
00eb198
implement fn -> unsafe fn pointer casts
oli-obk Jun 17, 2016
874d683
improve method names and add documentation
oli-obk Jun 20, 2016
d977642
compiletest 2.0 uses json errors and doesn't depend on the output for…
oli-obk Jun 20, 2016
e90ee16
fix comparing of function pointers
oli-obk Jun 20, 2016
a1082b9
Merge remote-tracking branch 'origin/master' into oflo
oli-obk Jun 20, 2016
ed4af21
fix master
oli-obk Jun 20, 2016
b9ac85d
rustc does overflow checking for us, don't duplicate it.
oli-obk Jun 20, 2016
68469be
rename function cache member
oli-obk Jun 20, 2016
0821a15
no need for EvalContext::eval_binop
oli-obk Jun 20, 2016
3e3aeab
implement bit masks as the compiler would translate them
oli-obk Jun 20, 2016
a088f10
add a comment explaining the magic numbers
oli-obk Jun 20, 2016
001ae69
remove the bad rhs value error and panic instead. the typechecker pre…
oli-obk Jun 20, 2016
c7039db
simplify the masked rhs computation
oli-obk Jun 20, 2016
65de5dd
simplify even more
oli-obk Jun 20, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ use std::fmt;
use rustc::mir::repr as mir;
use rustc::ty::BareFnTy;
use memory::Pointer;
use rustc_const_math::ConstMathErr;
use syntax::codemap::Span;
use primval::PrimVal;

#[derive(Clone, Debug)]
pub enum EvalError<'tcx> {
Expand All @@ -24,6 +27,10 @@ pub enum EvalError<'tcx> {
Unimplemented(String),
DerefFunctionPointer,
ExecuteMemory,
ArrayIndexOutOfBounds(Span, u64, u64),
Math(Span, ConstMathErr),
InvalidBitShiftRhs(PrimVal),
Copy link
Member

@eddyb eddyb Jun 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This is not actually a thing, the semantics of shifting in Rust is that the RHS gets masked with BITS - 1 (assuming bit widths are always a power of 2) and interpreted as unsigned.

EDIT: corrected description of the mask.

(misplaced on another PR)

Overflow(PrimVal, PrimVal, mir::BinOp, PrimVal),
Copy link
Member

Choose a reason for hiding this comment

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

Does this get ignored outside of intrinsic_overflowing? Again, the semantics are 2's complement + wrapping, unless explicit checking is done and reported with Assert.

Copy link
Contributor Author

@oli-obk oli-obk Jun 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Since the current tests are only run in debug mode, the overflow checks prevent the overflow error from ever triggering.

Copy link
Member

@solson solson Jun 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

EvalError::Overflow seems to only be used for checking if eval_binop overflowed, so if eval_binop just did overflowing math internally and also returned a boolean whether something overflowed, could we remove EvalError::Overflow?

I think I want to avoid using Err cases for non-errors (though I currently violate this myself with some pointer reads somewhere that I catch). Unless you think this way is cleaner.

}

pub type EvalResult<'tcx, T> = Result<T, EvalError<'tcx>>;
Expand Down Expand Up @@ -58,6 +65,14 @@ impl<'tcx> Error for EvalError<'tcx> {
"tried to dereference a function pointer",
EvalError::ExecuteMemory =>
"tried to treat a memory pointer as a function pointer",
EvalError::ArrayIndexOutOfBounds(..) =>
"array index out of bounds",
EvalError::Math(..) =>
"mathematical operation failed",
EvalError::InvalidBitShiftRhs(..) =>
"bit shift rhs negative or not an int",
EvalError::Overflow(..) =>
"mathematical operation overflowed",
}
}

Expand All @@ -73,6 +88,12 @@ impl<'tcx> fmt::Display for EvalError<'tcx> {
},
EvalError::FunctionPointerTyMismatch(expected, got) =>
write!(f, "tried to call a function of type {:?} through a function pointer of type {:?}", expected, got),
EvalError::ArrayIndexOutOfBounds(span, len, index) =>
write!(f, "array index {} out of bounds {} at {:?}", index, len, span),
EvalError::Math(span, ref err) =>
write!(f, "mathematical operation at {:?} failed with {:?}", span, err),
EvalError::Overflow(l, r, op, val) =>
write!(f, "mathematical operation overflowed: {:?} {} {:?} => {:?}", l, op.to_hir_binop().as_str(), r, val),
_ => write!(f, "{}", self.description()),
}
}
Expand Down
211 changes: 121 additions & 90 deletions src/interpreter/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,15 +474,23 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
self.frame_mut().block = target;
}

Assert { ref cond, expected, ref msg, target, cleanup } => {
let actual_ptr = self.eval_operand(cond)?;
let actual = self.memory.read_bool(actual_ptr)?;
if actual == expected {
Assert { ref cond, expected, ref msg, target, .. } => {
let cond_ptr = self.eval_operand(cond)?;
if expected == self.memory.read_bool(cond_ptr)? {
self.frame_mut().block = target;
} else {
panic!("unimplemented: jump to {:?} and print {:?}", cleanup, msg);
return match *msg {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: unnecessary return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this isn't part of the return expression. The match could be refactored though

Copy link
Member

Choose a reason for hiding this comment

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

Oops, I should have expanded the code after the diff. It's fine how it is.

mir::AssertMessage::BoundsCheck { ref len, ref index } => {
let len = self.eval_operand(len).expect("can't eval len");
let len = self.memory.read_usize(len).expect("can't read len");
let index = self.eval_operand(index).expect("can't eval index");
let index = self.memory.read_usize(index).expect("can't read index");
Err(EvalError::ArrayIndexOutOfBounds(terminator.source_info.span, len, index))
},
mir::AssertMessage::Math(ref err) => Err(EvalError::Math(terminator.source_info.span, err.clone())),
}
}
}
},

DropAndReplace { .. } => unimplemented!(),
Resume => unimplemented!(),
Expand All @@ -507,9 +515,9 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
let name = self.tcx.item_name(def_id).as_str();
match fn_ty.sig.0.output {
ty::FnConverging(ty) => {
let size = self.type_size(ty);
let layout = self.type_layout(ty);
let ret = return_ptr.unwrap();
self.call_intrinsic(&name, substs, args, ret, size)
self.call_intrinsic(&name, substs, args, ret, layout)
}
ty::FnDiverging => unimplemented!(),
}
Expand Down Expand Up @@ -659,87 +667,126 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
Ok(if not_null { nndiscr } else { 1 - nndiscr })
}

fn intrinsic_with_overflow(
&mut self,
op: mir::BinOp,
left: &mir::Operand<'tcx>,
right: &mir::Operand<'tcx>,
dest: Pointer,
dest_layout: &'tcx Layout,
) -> EvalResult<'tcx, ()> {
use rustc::ty::layout::Layout::*;
let tup_layout = match *dest_layout {
Univariant { ref variant, .. } => variant,
_ => panic!("checked bin op returns something other than a tuple"),
};

let overflowed = self.intrinsic_overflowing(op, left, right, dest)?;
let offset = tup_layout.field_offset(1).bytes() as isize;
self.memory.write_bool(dest.offset(offset), overflowed)
}

fn math(
&mut self,
op: mir::BinOp,
left: &mir::Operand<'tcx>,
right: &mir::Operand<'tcx>,
) -> EvalResult<'tcx, PrimVal> {
let left_ptr = self.eval_operand(left)?;
let left_ty = self.operand_ty(left);
let left_val = self.read_primval(left_ptr, left_ty)?;

let right_ptr = self.eval_operand(right)?;
let right_ty = self.operand_ty(right);
let right_val = self.read_primval(right_ptr, right_ty)?;

primval::binary_op(op, left_val, right_val)
}

fn intrinsic_overflowing(
&mut self,
op: mir::BinOp,
left: &mir::Operand<'tcx>,
right: &mir::Operand<'tcx>,
dest: Pointer,
) -> EvalResult<'tcx, bool> {
match self.math(op, left, right) {
Ok(val) => {
self.memory.write_primval(dest, val)?;
Ok(false)
},
Err(EvalError::Overflow(l, r, op, val)) => {
debug!("operation overflowed: {:?} {} {:?} => {:?}", l, op.to_hir_binop().as_str(), r, val);
self.memory.write_primval(dest, val)?;
Ok(true)
},
Err(other) => Err(other),
}
}

fn call_intrinsic(
&mut self,
name: &str,
substs: &'tcx Substs<'tcx>,
args: &[mir::Operand<'tcx>],
dest: Pointer,
dest_size: usize
dest_layout: &'tcx Layout,
) -> EvalResult<'tcx, ()> {
let args_res: EvalResult<Vec<Pointer>> = args.iter()
.map(|arg| self.eval_operand(arg))
.collect();
let args = args_res?;
let args_ptrs = args_res?;

let pointer_size = self.memory.pointer_size;

match name {
// FIXME(solson): Handle different integer types correctly.
"add_with_overflow" => {
let ty = *substs.types.get(subst::FnSpace, 0);
let size = self.type_size(ty);
let left = self.memory.read_int(args[0], size)?;
let right = self.memory.read_int(args[1], size)?;
let (n, overflowed) = unsafe {
::std::intrinsics::add_with_overflow::<i64>(left, right)
};
self.memory.write_int(dest, n, size)?;
self.memory.write_bool(dest.offset(size as isize), overflowed)?;
}
"add_with_overflow" => self.intrinsic_with_overflow(mir::BinOp::Add, &args[0], &args[1], dest, dest_layout)?,
"sub_with_overflow" => self.intrinsic_with_overflow(mir::BinOp::Sub, &args[0], &args[1], dest, dest_layout)?,
"mul_with_overflow" => self.intrinsic_with_overflow(mir::BinOp::Mul, &args[0], &args[1], dest, dest_layout)?,

// FIXME: turn into an assertion to catch wrong `assume` that would cause UB in llvm
"assume" => {}

"copy_nonoverlapping" => {
let elem_ty = *substs.types.get(subst::FnSpace, 0);
let elem_size = self.type_size(elem_ty);
let src = self.memory.read_ptr(args[0])?;
let dest = self.memory.read_ptr(args[1])?;
let count = self.memory.read_isize(args[2])?;
let src = self.memory.read_ptr(args_ptrs[0])?;
let dest = self.memory.read_ptr(args_ptrs[1])?;
let count = self.memory.read_isize(args_ptrs[2])?;
self.memory.copy(src, dest, count as usize * elem_size)?;
}

"discriminant_value" => {
let ty = *substs.types.get(subst::FnSpace, 0);
let adt_ptr = self.memory.read_ptr(args[0])?;
let adt_ptr = self.memory.read_ptr(args_ptrs[0])?;
let discr_val = self.read_discriminant_value(adt_ptr, ty)?;
self.memory.write_uint(dest, discr_val, dest_size)?;
self.memory.write_uint(dest, discr_val, 8)?;
}

"forget" => {
let arg_ty = *substs.types.get(subst::FnSpace, 0);
let arg_size = self.type_size(arg_ty);
self.memory.drop_fill(args[0], arg_size)?;
self.memory.drop_fill(args_ptrs[0], arg_size)?;
}

"init" => self.memory.write_repeat(dest, 0, dest_size)?,
"init" => self.memory.write_repeat(dest, 0, dest_layout.size(&self.tcx.data_layout).bytes() as usize)?,

"min_align_of" => {
self.memory.write_int(dest, 1, dest_size)?;
// FIXME: use correct value
self.memory.write_int(dest, 1, pointer_size)?;
}

"move_val_init" => {
let ty = *substs.types.get(subst::FnSpace, 0);
let ptr = self.memory.read_ptr(args[0])?;
self.move_(args[1], ptr, ty)?;
}

// FIXME(solson): Handle different integer types correctly.
"mul_with_overflow" => {
let ty = *substs.types.get(subst::FnSpace, 0);
let size = self.type_size(ty);
let left = self.memory.read_int(args[0], size)?;
let right = self.memory.read_int(args[1], size)?;
let (n, overflowed) = unsafe {
::std::intrinsics::mul_with_overflow::<i64>(left, right)
};
self.memory.write_int(dest, n, size)?;
self.memory.write_bool(dest.offset(size as isize), overflowed)?;
let ptr = self.memory.read_ptr(args_ptrs[0])?;
self.move_(args_ptrs[1], ptr, ty)?;
}

"offset" => {
let pointee_ty = *substs.types.get(subst::FnSpace, 0);
let pointee_size = self.type_size(pointee_ty) as isize;
let ptr_arg = args[0];
let offset = self.memory.read_isize(args[1])?;
let ptr_arg = args_ptrs[0];
let offset = self.memory.read_isize(args_ptrs[1])?;

match self.memory.read_ptr(ptr_arg) {
Ok(ptr) => {
Expand All @@ -755,35 +802,35 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
}
}

// FIXME(solson): Handle different integer types correctly. Use primvals?
"overflowing_sub" => {
let ty = *substs.types.get(subst::FnSpace, 0);
let size = self.type_size(ty);
let left = self.memory.read_int(args[0], size)?;
let right = self.memory.read_int(args[1], size)?;
let n = left.wrapping_sub(right);
self.memory.write_int(dest, n, size)?;
self.intrinsic_overflowing(mir::BinOp::Sub, &args[0], &args[1], dest)?;
}
"overflowing_mul" => {
self.intrinsic_overflowing(mir::BinOp::Mul, &args[0], &args[1], dest)?;
}
"overflowing_add" => {
self.intrinsic_overflowing(mir::BinOp::Add, &args[0], &args[1], dest)?;
}

"size_of" => {
let ty = *substs.types.get(subst::FnSpace, 0);
let size = self.type_size(ty) as u64;
self.memory.write_uint(dest, size, dest_size)?;
self.memory.write_uint(dest, size, pointer_size)?;
}

"size_of_val" => {
let ty = *substs.types.get(subst::FnSpace, 0);
if self.type_is_sized(ty) {
let size = self.type_size(ty) as u64;
self.memory.write_uint(dest, size, dest_size)?;
self.memory.write_uint(dest, size, pointer_size)?;
} else {
match ty.sty {
ty::TySlice(_) | ty::TyStr => {
let elem_ty = ty.sequence_element_type(self.tcx);
let elem_size = self.type_size(elem_ty) as u64;
let ptr_size = self.memory.pointer_size as isize;
let n = self.memory.read_usize(args[0].offset(ptr_size))?;
self.memory.write_uint(dest, n * elem_size, dest_size)?;
let n = self.memory.read_usize(args_ptrs[0].offset(ptr_size))?;
self.memory.write_uint(dest, n * elem_size, pointer_size)?;
}

_ => return Err(EvalError::Unimplemented(format!("unimplemented: size_of_val::<{:?}>", ty))),
Expand All @@ -793,9 +840,9 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {

"transmute" => {
let ty = *substs.types.get(subst::FnSpace, 0);
self.move_(args[0], dest, ty)?;
self.move_(args_ptrs[0], dest, ty)?;
}
"uninit" => self.memory.mark_definedness(dest, dest_size, false)?,
"uninit" => self.memory.mark_definedness(dest, dest_layout.size(&self.tcx.data_layout).bytes() as usize, false)?,

name => return Err(EvalError::Unimplemented(format!("unimplemented intrinsic: {}", name))),
}
Expand Down Expand Up @@ -900,35 +947,12 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
}

BinaryOp(bin_op, ref left, ref right) => {
let left_ptr = self.eval_operand(left)?;
let left_ty = self.operand_ty(left);
let left_val = self.read_primval(left_ptr, left_ty)?;

let right_ptr = self.eval_operand(right)?;
let right_ty = self.operand_ty(right);
let right_val = self.read_primval(right_ptr, right_ty)?;

let val = primval::binary_op(bin_op, left_val, right_val)?;
self.memory.write_primval(dest, val)?;
let result = self.math(bin_op, left, right)?;
self.memory.write_primval(dest, result)?;
}

// FIXME(solson): Factor this out with BinaryOp.
CheckedBinaryOp(bin_op, ref left, ref right) => {
let left_ptr = self.eval_operand(left)?;
let left_ty = self.operand_ty(left);
let left_val = self.read_primval(left_ptr, left_ty)?;

let right_ptr = self.eval_operand(right)?;
let right_ty = self.operand_ty(right);
let right_val = self.read_primval(right_ptr, right_ty)?;

let val = primval::binary_op(bin_op, left_val, right_val)?;
self.memory.write_primval(dest, val)?;

// FIXME(solson): Find the result type size properly. Perhaps refactor out
// Projection calculations so we can do the equivalent of `dest.1` here.
let s = self.type_size(left_ty);
self.memory.write_bool(dest.offset(s as isize), false)?;
self.intrinsic_with_overflow(bin_op, left, right, dest, dest_layout)?;
}

UnaryOp(un_op, ref operand) => {
Expand Down Expand Up @@ -1103,9 +1127,7 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {

// Hack to support fat pointer -> thin pointer casts to keep tests for
// other things passing for now.
let is_fat_ptr_cast = pointee_type(src_ty).map(|ty| {
!self.type_is_sized(ty)
}).unwrap_or(false);
let is_fat_ptr_cast = pointee_type(src_ty).map_or(false, |ty| !self.type_is_sized(ty));

if dest_size == src_size || is_fat_ptr_cast {
self.memory.copy(src, dest, dest_size)?;
Expand All @@ -1122,7 +1144,16 @@ impl<'a, 'tcx> EvalContext<'a, 'tcx> {
ref other => panic!("reify fn pointer on {:?}", other),
},

_ => return Err(EvalError::Unimplemented(format!("can't handle cast: {:?}", rvalue))),
UnsafeFnPointer => match dest_ty.sty {
ty::TyFnPtr(unsafe_fn_ty) => {
let src = self.eval_operand(operand)?;
let ptr = self.memory.read_ptr(src)?;
let fn_def = self.memory.get_fn(ptr.alloc_id)?;
let fn_ptr = self.memory.create_fn_ptr(fn_def.def_id, fn_def.substs, unsafe_fn_ty);
self.memory.write_ptr(dest, fn_ptr)?;
},
ref other => panic!("fn to unsafe fn cast on {:?}", other),
},
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
btree_range,
collections,
collections_bound,
core_intrinsics,
filling_drop,
Copy link
Member

Choose a reason for hiding this comment

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

Yay. I hated this hack and the unsafe code I added with it.

question_mark,
rustc_private,
Expand All @@ -14,6 +13,7 @@
extern crate rustc_data_structures;
extern crate rustc_mir;
extern crate rustc_trans;
extern crate rustc_const_math;
extern crate syntax;
#[macro_use] extern crate log;
extern crate log_settings;
Expand Down
Loading