From d060e202e97dc8f535ae709c2d6cd0a6ba5aa87e Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Fri, 15 Feb 2019 02:11:47 +0000 Subject: [PATCH 1/3] [WIP] Make usize overflow always have debug-assertions semantics This is a prototype for https://github.com/rust-lang/rfcs/pull/2635 to enable us to collect some measurements. --- src/librustc_codegen_llvm/context.rs | 14 +++++++++++--- src/librustc_codegen_ssa/mir/block.rs | 2 +- src/librustc_codegen_ssa/mir/rvalue.rs | 2 +- src/librustc_codegen_ssa/traits/misc.rs | 2 +- src/librustc_mir/build/expr/as_rvalue.rs | 4 ++-- src/librustc_mir/hair/cx/mod.rs | 6 +++--- 6 files changed, 19 insertions(+), 11 deletions(-) diff --git a/src/librustc_codegen_llvm/context.rs b/src/librustc_codegen_llvm/context.rs index 8144132832e50..e95711d8795d6 100644 --- a/src/librustc_codegen_llvm/context.rs +++ b/src/librustc_codegen_llvm/context.rs @@ -12,13 +12,15 @@ use type_of::PointeeInfo; use rustc_codegen_ssa::traits::*; use libc::c_uint; +use syntax::ast; + use rustc_data_structures::base_n; use rustc_data_structures::small_c_str::SmallCStr; use rustc::mir::mono::Stats; use rustc::session::config::{self, DebugInfo}; use rustc::session::Session; use rustc::ty::layout::{LayoutError, LayoutOf, Size, TyLayout, VariantIdx}; -use rustc::ty::{self, Ty, TyCtxt}; +use rustc::ty::{self, Ty, TyCtxt, TyKind}; use rustc::util::nodemap::FxHashMap; use rustc_target::spec::{HasTargetSpec, Target}; use rustc_codegen_ssa::callee::resolve_and_get_fn; @@ -409,8 +411,14 @@ impl MiscMethods<'tcx> for CodegenCx<'ll, 'tcx> { &self.tcx.sess } - fn check_overflow(&self) -> bool { - self.check_overflow + fn check_overflow(&self, ty: Option>) -> bool { + let type_specific_overflow = match ty { + Some(ty) => { + ty.sty == TyKind::Uint(ast::UintTy::Usize) + }, + None => false + }; + self.check_overflow || type_specific_overflow } fn stats(&self) -> &RefCell { diff --git a/src/librustc_codegen_ssa/mir/block.rs b/src/librustc_codegen_ssa/mir/block.rs index caca1789fc98c..0b71a45e79f67 100644 --- a/src/librustc_codegen_ssa/mir/block.rs +++ b/src/librustc_codegen_ssa/mir/block.rs @@ -339,7 +339,7 @@ impl<'a, 'tcx: 'a, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { // NOTE: Unlike binops, negation doesn't have its own // checked operation, just a comparison with the minimum // value, so we have to check for the assert message. - if !bx.check_overflow() { + if !bx.check_overflow(None) { if let mir::interpret::EvalErrorKind::OverflowNeg = *msg { const_cond = Some(expected); } diff --git a/src/librustc_codegen_ssa/mir/rvalue.rs b/src/librustc_codegen_ssa/mir/rvalue.rs index 25a7754d118d7..a7cbdb0c227b7 100644 --- a/src/librustc_codegen_ssa/mir/rvalue.rs +++ b/src/librustc_codegen_ssa/mir/rvalue.rs @@ -670,7 +670,7 @@ impl<'a, 'tcx: 'a, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> { // with #[rustc_inherit_overflow_checks] and inlined from // another crate (mostly core::num generic/#[inline] fns), // while the current crate doesn't use overflow checks. - if !bx.cx().check_overflow() { + if !bx.cx().check_overflow(Some(input_ty)) { let val = self.codegen_scalar_binop(bx, op, lhs, rhs, input_ty); return OperandValue::Pair(val, bx.cx().const_bool(false)); } diff --git a/src/librustc_codegen_ssa/traits/misc.rs b/src/librustc_codegen_ssa/traits/misc.rs index b23155563665d..5595f66ed4d8d 100644 --- a/src/librustc_codegen_ssa/traits/misc.rs +++ b/src/librustc_codegen_ssa/traits/misc.rs @@ -12,7 +12,7 @@ pub trait MiscMethods<'tcx>: BackendTypes { fn vtables( &self, ) -> &RefCell, Option>), Self::Value>>; - fn check_overflow(&self) -> bool; + fn check_overflow(&self, ty: Option>) -> bool; fn instances(&self) -> &RefCell, Self::Value>>; fn get_fn(&self, instance: Instance<'tcx>) -> Self::Value; fn get_param(&self, llfn: Self::Value, index: c_uint) -> Self::Value; diff --git a/src/librustc_mir/build/expr/as_rvalue.rs b/src/librustc_mir/build/expr/as_rvalue.rs index 88dbd93939e54..8b0e792db4e1e 100644 --- a/src/librustc_mir/build/expr/as_rvalue.rs +++ b/src/librustc_mir/build/expr/as_rvalue.rs @@ -84,7 +84,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { ExprKind::Unary { op, arg } => { let arg = unpack!(block = this.as_operand(block, scope, arg)); // Check for -MIN on signed integers - if this.hir.check_overflow() && op == UnOp::Neg && expr.ty.is_signed() { + if this.hir.check_overflow(expr.ty) && op == UnOp::Neg && expr.ty.is_signed() { let bool_ty = this.hir.bool_ty(); let minval = this.minval_literal(expr_span, expr.ty); @@ -410,7 +410,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> { ) -> BlockAnd> { let source_info = self.source_info(span); let bool_ty = self.hir.bool_ty(); - if self.hir.check_overflow() && op.is_checkable() && ty.is_integral() { + if self.hir.check_overflow(ty) && op.is_checkable() && ty.is_integral() { let result_tup = self.hir.tcx().intern_tup(&[ty, bool_ty]); let result_value = self.temp(result_tup, span); diff --git a/src/librustc_mir/hair/cx/mod.rs b/src/librustc_mir/hair/cx/mod.rs index cd937d702fd77..f41c78db6b2b4 100644 --- a/src/librustc_mir/hair/cx/mod.rs +++ b/src/librustc_mir/hair/cx/mod.rs @@ -11,7 +11,7 @@ use rustc::hir::Node; use rustc::middle::region; use rustc::infer::InferCtxt; use rustc::ty::subst::Subst; -use rustc::ty::{self, Ty, TyCtxt}; +use rustc::ty::{self, Ty, TyCtxt, TyKind}; use rustc::ty::subst::{Kind, Substs}; use rustc::ty::layout::VariantIdx; use syntax::ast; @@ -218,8 +218,8 @@ impl<'a, 'gcx, 'tcx> Cx<'a, 'gcx, 'tcx> { self.tables } - pub fn check_overflow(&self) -> bool { - self.check_overflow + pub fn check_overflow(&self, ty: Ty<'tcx>) -> bool { + self.check_overflow || ty.sty == TyKind::Uint(ast::UintTy::Usize) } pub fn type_is_copy_modulo_regions(&self, ty: Ty<'tcx>, span: Span) -> bool { From aef540b216742e2c2c85029d95fe1224d8d6cdbe Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Fri, 15 Feb 2019 13:42:45 +0000 Subject: [PATCH 2/3] Remove failing tests so we can do a perf run The failures aren't central to what this patch is about (they're related to linking and panic not being included in the final artifact), so they shouldn't disrupt perf measurements. --- src/test/run-make-fulldeps/issue-14500/Makefile | 17 ----------------- src/test/run-make-fulldeps/issue-14500/bar.rs | 1 - src/test/run-make-fulldeps/issue-14500/foo.c | 7 ------- src/test/run-make-fulldeps/issue-14500/foo.rs | 5 ----- src/test/run-make-fulldeps/lto-smoke-c/Makefile | 11 ----------- src/test/run-make-fulldeps/lto-smoke-c/bar.c | 7 ------- src/test/run-make-fulldeps/lto-smoke-c/foo.rs | 4 ---- 7 files changed, 52 deletions(-) delete mode 100644 src/test/run-make-fulldeps/issue-14500/Makefile delete mode 100644 src/test/run-make-fulldeps/issue-14500/bar.rs delete mode 100644 src/test/run-make-fulldeps/issue-14500/foo.c delete mode 100644 src/test/run-make-fulldeps/issue-14500/foo.rs delete mode 100644 src/test/run-make-fulldeps/lto-smoke-c/Makefile delete mode 100644 src/test/run-make-fulldeps/lto-smoke-c/bar.c delete mode 100644 src/test/run-make-fulldeps/lto-smoke-c/foo.rs diff --git a/src/test/run-make-fulldeps/issue-14500/Makefile b/src/test/run-make-fulldeps/issue-14500/Makefile deleted file mode 100644 index bd94db0952028..0000000000000 --- a/src/test/run-make-fulldeps/issue-14500/Makefile +++ /dev/null @@ -1,17 +0,0 @@ --include ../tools.mk - -# Test to make sure that reachable extern fns are always available in final -# productcs, including when LTO is used. In this test, the `foo` crate has a -# reahable symbol, and is a dependency of the `bar` crate. When the `bar` crate -# is compiled with LTO, it shouldn't strip the symbol from `foo`, and that's the -# only way that `foo.c` will successfully compile. - -ifeq ($(UNAME),Bitrig) - EXTRACFLAGS := -lc $(EXTRACFLAGS) $(EXTRACXXFLAGS) -endif - -all: - $(RUSTC) foo.rs --crate-type=rlib - $(RUSTC) bar.rs --crate-type=staticlib -C lto -L. -o $(TMPDIR)/libbar.a - $(CC) foo.c $(TMPDIR)/libbar.a $(EXTRACFLAGS) $(call OUT_EXE,foo) - $(call RUN,foo) diff --git a/src/test/run-make-fulldeps/issue-14500/bar.rs b/src/test/run-make-fulldeps/issue-14500/bar.rs deleted file mode 100644 index 49af74e1b7489..0000000000000 --- a/src/test/run-make-fulldeps/issue-14500/bar.rs +++ /dev/null @@ -1 +0,0 @@ -extern crate foo; diff --git a/src/test/run-make-fulldeps/issue-14500/foo.c b/src/test/run-make-fulldeps/issue-14500/foo.c deleted file mode 100644 index 2353d400df303..0000000000000 --- a/src/test/run-make-fulldeps/issue-14500/foo.c +++ /dev/null @@ -1,7 +0,0 @@ -extern void foo(); -extern char FOO_STATIC; - -int main() { - foo(); - return (int)FOO_STATIC; -} diff --git a/src/test/run-make-fulldeps/issue-14500/foo.rs b/src/test/run-make-fulldeps/issue-14500/foo.rs deleted file mode 100644 index feebef1a6d079..0000000000000 --- a/src/test/run-make-fulldeps/issue-14500/foo.rs +++ /dev/null @@ -1,5 +0,0 @@ -#[no_mangle] -pub extern fn foo() {} - -#[no_mangle] -pub static FOO_STATIC: u8 = 0; diff --git a/src/test/run-make-fulldeps/lto-smoke-c/Makefile b/src/test/run-make-fulldeps/lto-smoke-c/Makefile deleted file mode 100644 index 0f61f5de938bc..0000000000000 --- a/src/test/run-make-fulldeps/lto-smoke-c/Makefile +++ /dev/null @@ -1,11 +0,0 @@ --include ../tools.mk - -# Apparently older versions of GCC segfault if -g is passed... -CC := $(CC:-g=) - -all: - $(RUSTC) foo.rs -C lto - $(CC) bar.c $(call STATICLIB,foo) \ - $(call OUT_EXE,bar) \ - $(EXTRACFLAGS) $(EXTRACXXFLAGS) - $(call RUN,bar) diff --git a/src/test/run-make-fulldeps/lto-smoke-c/bar.c b/src/test/run-make-fulldeps/lto-smoke-c/bar.c deleted file mode 100644 index 5729d411c5bcd..0000000000000 --- a/src/test/run-make-fulldeps/lto-smoke-c/bar.c +++ /dev/null @@ -1,7 +0,0 @@ -// ignore-license -void foo(); - -int main() { - foo(); - return 0; -} diff --git a/src/test/run-make-fulldeps/lto-smoke-c/foo.rs b/src/test/run-make-fulldeps/lto-smoke-c/foo.rs deleted file mode 100644 index 2e59432cdb16f..0000000000000 --- a/src/test/run-make-fulldeps/lto-smoke-c/foo.rs +++ /dev/null @@ -1,4 +0,0 @@ -#![crate_type = "staticlib"] - -#[no_mangle] -pub extern "C" fn foo() {} From 864dd3cd62b561a6ae592adebb9c56f8f1ccb071 Mon Sep 17 00:00:00 2001 From: Alex Gaynor Date: Fri, 22 Feb 2019 12:34:07 -0500 Subject: [PATCH 3/3] Refactor how opaque::Decoder represents its contents Instead of (&[u8], position) simply store the &[u8] and reslice. This was originally written for #58475, to see if removing the arithmetic helped with avoiding integer overflow checks, however I think the result is slightly more readable in general -- specifically the removal of set_position is a nice win. I think this might be a hair faster even without the changes in #58475, but I haven't measured that. --- src/librustc/ty/query/on_disk_cache.rs | 15 ++++---- src/librustc_incremental/persist/load.rs | 2 +- src/librustc_metadata/decoder.rs | 4 +- src/libserialize/leb128.rs | 8 ++-- src/libserialize/lib.rs | 1 + src/libserialize/opaque.rs | 48 +++++++++++------------- 6 files changed, 37 insertions(+), 41 deletions(-) diff --git a/src/librustc/ty/query/on_disk_cache.rs b/src/librustc/ty/query/on_disk_cache.rs index c16f861dedb50..832cf0986c9ab 100644 --- a/src/librustc/ty/query/on_disk_cache.rs +++ b/src/librustc/ty/query/on_disk_cache.rs @@ -105,22 +105,21 @@ impl AbsoluteBytePos { impl<'sess> OnDiskCache<'sess> { /// Creates a new OnDiskCache instance from the serialized data in `data`. - pub fn new(sess: &'sess Session, data: Vec, start_pos: usize) -> OnDiskCache<'sess> { + pub fn new(sess: &'sess Session, data: Vec) -> OnDiskCache<'sess> { debug_assert!(sess.opts.incremental.is_some()); // Wrapping in a scope so we can borrow `data` let footer: Footer = { - let mut decoder = opaque::Decoder::new(&data[..], start_pos); - // Decode the *position* of the footer which can be found in the // last 8 bytes of the file. - decoder.set_position(data.len() - IntEncodedWithFixedSize::ENCODED_SIZE); + let mut decoder = opaque::Decoder::new( + &data, data.len() - IntEncodedWithFixedSize::ENCODED_SIZE); let query_result_index_pos = IntEncodedWithFixedSize::decode(&mut decoder) .expect("Error while trying to decode query result index position.") .0 as usize; // Decoder the file footer which contains all the lookup tables, etc. - decoder.set_position(query_result_index_pos); + decoder = opaque::Decoder::new(&data, query_result_index_pos); decode_tagged(&mut decoder, TAG_FILE_FOOTER) .expect("Error while trying to decode query result index position.") }; @@ -540,7 +539,7 @@ impl<'a, 'tcx: 'a, 'x> ty_codec::TyDecoder<'a, 'tcx> for CacheDecoder<'a, 'tcx, #[inline] fn peek_byte(&self) -> u8 { - self.opaque.data[self.opaque.position()] + self.opaque.data()[0] } fn cached_ty_for_shorthand(&mut self, @@ -569,9 +568,9 @@ impl<'a, 'tcx: 'a, 'x> ty_codec::TyDecoder<'a, 'tcx> for CacheDecoder<'a, 'tcx, fn with_position(&mut self, pos: usize, f: F) -> R where F: FnOnce(&mut Self) -> R { - debug_assert!(pos < self.opaque.data.len()); + debug_assert!(pos < self.opaque.original_data.len()); - let new_opaque = opaque::Decoder::new(self.opaque.data, pos); + let new_opaque = opaque::Decoder::new(&self.opaque.original_data, pos); let old_opaque = mem::replace(&mut self.opaque, new_opaque); let r = f(self); self.opaque = old_opaque; diff --git a/src/librustc_incremental/persist/load.rs b/src/librustc_incremental/persist/load.rs index ecf8bc4a88084..178b179721d3b 100644 --- a/src/librustc_incremental/persist/load.rs +++ b/src/librustc_incremental/persist/load.rs @@ -200,7 +200,7 @@ pub fn load_query_result_cache<'sess>(sess: &'sess Session) -> OnDiskCache<'sess } match load_data(sess.opts.debugging_opts.incremental_info, &query_cache_path(sess)) { - LoadResult::Ok{ data: (bytes, start_pos) } => OnDiskCache::new(sess, bytes, start_pos), + LoadResult::Ok{ data: (bytes, _) } => OnDiskCache::new(sess, bytes), _ => OnDiskCache::new_empty(sess.source_map()) } } diff --git a/src/librustc_metadata/decoder.rs b/src/librustc_metadata/decoder.rs index 1c4e3bc6a50e7..5278723bdcaca 100644 --- a/src/librustc_metadata/decoder.rs +++ b/src/librustc_metadata/decoder.rs @@ -179,7 +179,7 @@ impl<'a, 'tcx: 'a> TyDecoder<'a, 'tcx> for DecodeContext<'a, 'tcx> { #[inline] fn peek_byte(&self) -> u8 { - self.opaque.data[self.opaque.position()] + self.opaque.data()[0] } #[inline] @@ -212,7 +212,7 @@ impl<'a, 'tcx: 'a> TyDecoder<'a, 'tcx> for DecodeContext<'a, 'tcx> { fn with_position(&mut self, pos: usize, f: F) -> R where F: FnOnce(&mut Self) -> R { - let new_opaque = opaque::Decoder::new(self.opaque.data, pos); + let new_opaque = opaque::Decoder::new(self.opaque.original_data, pos); let old_opaque = mem::replace(&mut self.opaque, new_opaque); let old_state = mem::replace(&mut self.lazy_state, LazyState::NoNode); let r = f(self); diff --git a/src/libserialize/leb128.rs b/src/libserialize/leb128.rs index 16ff59489e718..a94e29b3abbb4 100644 --- a/src/libserialize/leb128.rs +++ b/src/libserialize/leb128.rs @@ -114,10 +114,10 @@ pub fn write_signed_leb128(out: &mut Vec, value: i128) { } #[inline] -pub fn read_signed_leb128(data: &[u8], start_position: usize) -> (i128, usize) { +pub fn read_signed_leb128(data: &[u8]) -> (i128, usize) { let mut result = 0; let mut shift = 0; - let mut position = start_position; + let mut position = 0; let mut byte; loop { @@ -136,7 +136,7 @@ pub fn read_signed_leb128(data: &[u8], start_position: usize) -> (i128, usize) { result |= -(1 << shift); } - (result, position - start_position) + (result, position) } macro_rules! impl_test_unsigned_leb128 { @@ -176,7 +176,7 @@ fn test_signed_leb128() { } let mut pos = 0; for &x in &values { - let (value, bytes_read) = read_signed_leb128(&mut stream, pos); + let (value, bytes_read) = read_signed_leb128(&mut stream[pos..]); pos += bytes_read; assert_eq!(x, value); } diff --git a/src/libserialize/lib.rs b/src/libserialize/lib.rs index b8eeb4d2b34af..553a5bdb499de 100644 --- a/src/libserialize/lib.rs +++ b/src/libserialize/lib.rs @@ -15,6 +15,7 @@ Core encoding and decoding interfaces. #![feature(specialization)] #![feature(never_type)] #![feature(nll)] +#![feature(ptr_wrapping_offset_from)] #![cfg_attr(test, feature(test))] pub use self::serialize::{Decoder, Encoder, Decodable, Encodable}; diff --git a/src/libserialize/opaque.rs b/src/libserialize/opaque.rs index a6a5c318079f1..37306c1f82547 100644 --- a/src/libserialize/opaque.rs +++ b/src/libserialize/opaque.rs @@ -157,42 +157,38 @@ impl Encoder { // ----------------------------------------------------------------------------- pub struct Decoder<'a> { - pub data: &'a [u8], - position: usize, + pub original_data: &'a [u8], + data: &'a [u8], } impl<'a> Decoder<'a> { #[inline] - pub fn new(data: &'a [u8], position: usize) -> Decoder<'a> { + pub fn new(data: &'a [u8], pos: usize) -> Decoder<'a> { Decoder { - data, - position, + original_data: data, + data: &data[pos..], } } #[inline] - pub fn position(&self) -> usize { - self.position + pub fn data(&self) -> &[u8] { + self.data } #[inline] - pub fn set_position(&mut self, pos: usize) { - self.position = pos + pub fn position(&self) -> usize { + self.data.as_ptr().wrapping_offset_from(self.original_data.as_ptr()) as usize } #[inline] pub fn advance(&mut self, bytes: usize) { - self.position += bytes; + self.data = &self.data[bytes..]; } #[inline] pub fn read_raw_bytes(&mut self, s: &mut [u8]) -> Result<(), String> { - let start = self.position; - let end = start + s.len(); - - s.copy_from_slice(&self.data[start..end]); - - self.position = end; + s.copy_from_slice(&self.data[..s.len()]); + self.advance(s.len()); Ok(()) } @@ -200,16 +196,16 @@ impl<'a> Decoder<'a> { macro_rules! read_uleb128 { ($dec:expr, $t:ty, $fun:ident) => ({ - let (value, bytes_read) = leb128::$fun(&$dec.data[$dec.position ..]); - $dec.position += bytes_read; + let (value, bytes_read) = leb128::$fun(&$dec.data); + $dec.advance(bytes_read); Ok(value) }) } macro_rules! read_sleb128 { ($dec:expr, $t:ty) => ({ - let (value, bytes_read) = read_signed_leb128($dec.data, $dec.position); - $dec.position += bytes_read; + let (value, bytes_read) = read_signed_leb128($dec.data); + $dec.advance(bytes_read); Ok(value as $t) }) } @@ -245,8 +241,8 @@ impl<'a> serialize::Decoder for Decoder<'a> { #[inline] fn read_u8(&mut self) -> Result { - let value = self.data[self.position]; - self.position += 1; + let value = self.data[0]; + self.advance(1); Ok(value) } @@ -277,8 +273,8 @@ impl<'a> serialize::Decoder for Decoder<'a> { #[inline] fn read_i8(&mut self) -> Result { - let as_u8 = self.data[self.position]; - self.position += 1; + let as_u8 = self.data[0]; + self.advance(1); unsafe { Ok(::std::mem::transmute(as_u8)) } } @@ -314,8 +310,8 @@ impl<'a> serialize::Decoder for Decoder<'a> { #[inline] fn read_str(&mut self) -> Result, Self::Error> { let len = self.read_usize()?; - let s = ::std::str::from_utf8(&self.data[self.position..self.position + len]).unwrap(); - self.position += len; + let s = ::std::str::from_utf8(&self.data[..len]).unwrap(); + self.advance(len); Ok(Cow::Borrowed(s)) }