From 8f4890d2f6f94765b656380ffde81432ad128dcc Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 9 Jul 2018 13:07:00 +0200 Subject: [PATCH 01/15] add debug assertion to from_raw_parts testing the alignment --- src/libcore/slice/mod.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/libcore/slice/mod.rs b/src/libcore/slice/mod.rs index b766140ffe99a..135290217c969 100644 --- a/src/libcore/slice/mod.rs +++ b/src/libcore/slice/mod.rs @@ -3927,18 +3927,18 @@ unsafe impl<'a, T> TrustedRandomAccess for ExactChunksMut<'a, T> { /// ``` /// use std::slice; /// -/// // manifest a slice out of thin air! -/// let ptr = 0x1234 as *const usize; -/// let amt = 10; -/// unsafe { -/// let slice = slice::from_raw_parts(ptr, amt); -/// } +/// // manifest a slice for a single element +/// let x = 42; +/// let ptr = &x as *const _; +/// let slice = unsafe { slice::from_raw_parts(ptr, 1) }; +/// assert_eq!(slice[0], 42); /// ``` /// /// [`NonNull::dangling()`]: ../../std/ptr/struct.NonNull.html#method.dangling #[inline] #[stable(feature = "rust1", since = "1.0.0")] pub unsafe fn from_raw_parts<'a, T>(data: *const T, len: usize) -> &'a [T] { + debug_assert!(data as usize % mem::align_of::() == 0, "attempt to create unaligned slice"); Repr { raw: FatPtr { data, len } }.rust } @@ -3952,6 +3952,7 @@ pub unsafe fn from_raw_parts<'a, T>(data: *const T, len: usize) -> &'a [T] { #[inline] #[stable(feature = "rust1", since = "1.0.0")] pub unsafe fn from_raw_parts_mut<'a, T>(data: *mut T, len: usize) -> &'a mut [T] { + debug_assert!(data as usize % mem::align_of::() == 0, "attempt to create unaligned slice"); Repr { raw: FatPtr { data, len} }.rust_mut } From ed0b5b3b8a2076ab4cf79a1893124a1b62f50a41 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 10 Jul 2018 09:01:10 +0200 Subject: [PATCH 02/15] add some tests for ZST slices, and fix failures due to unaligned references --- src/libcore/slice/mod.rs | 74 +++++++++++++++++++------------------- src/libcore/tests/slice.rs | 69 +++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 36 deletions(-) diff --git a/src/libcore/slice/mod.rs b/src/libcore/slice/mod.rs index 135290217c969..57f6f4a3b641c 100644 --- a/src/libcore/slice/mod.rs +++ b/src/libcore/slice/mod.rs @@ -45,7 +45,7 @@ use option::Option; use option::Option::{None, Some}; use result::Result; use result::Result::{Ok, Err}; -use ptr; +use ptr::{self, NonNull}; use mem; use marker::{Copy, Send, Sync, Sized, self}; use iter_private::TrustedRandomAccess; @@ -80,7 +80,7 @@ macro_rules! slice_offset { ($ptr:expr, $by:expr) => {{ let ptr = $ptr; if size_from_ptr(ptr) == 0 { - (ptr as *mut i8).wrapping_offset($by) as _ + (ptr as *mut i8).wrapping_offset($by.wrapping_mul(align_from_ptr(ptr) as isize)) as _ } else { ptr.offset($by) } @@ -93,7 +93,7 @@ macro_rules! make_ref { let ptr = $ptr; if size_from_ptr(ptr) == 0 { // Use a non-null pointer value - &*(1 as *mut _) + &*(NonNull::dangling().as_ptr() as *const _) } else { &*ptr } @@ -106,13 +106,39 @@ macro_rules! make_ref_mut { let ptr = $ptr; if size_from_ptr(ptr) == 0 { // Use a non-null pointer value - &mut *(1 as *mut _) + &mut *(NonNull::dangling().as_ptr()) } else { &mut *ptr } }}; } +macro_rules! make_slice { + ($start: expr, $end: expr) => {{ + let start = $start; + let len = unsafe { ptrdistance($start, $end) }; + if size_from_ptr(start) == 0 { + // use a non-null pointer value + unsafe { from_raw_parts(NonNull::dangling().as_ptr() as *const _, len) } + } else { + unsafe { from_raw_parts(start, len) } + } + }} +} + +macro_rules! make_mut_slice { + ($start: expr, $end: expr) => {{ + let start = $start; + let len = unsafe { ptrdistance($start, $end) }; + if size_from_ptr(start) == 0 { + // use a non-null pointer value + unsafe { from_raw_parts_mut(NonNull::dangling().as_ptr(), len) } + } else { + unsafe { from_raw_parts_mut(start, len) } + } + }} +} + #[lang = "slice"] #[cfg(not(test))] impl [T] { @@ -581,7 +607,7 @@ impl [T] { pub fn iter(&self) -> Iter { unsafe { let p = if mem::size_of::() == 0 { - 1 as *const _ + NonNull::dangling().as_ptr() as *const _ } else { let p = self.as_ptr(); assume(!p.is_null()); @@ -612,7 +638,7 @@ impl [T] { pub fn iter_mut(&mut self) -> IterMut { unsafe { let p = if mem::size_of::() == 0 { - 1 as *mut _ + NonNull::dangling().as_ptr() } else { let p = self.as_mut_ptr(); assume(!p.is_null()); @@ -2378,6 +2404,11 @@ fn size_from_ptr(_: *const T) -> usize { mem::size_of::() } +#[inline] +fn align_from_ptr(_: *const T) -> usize { + mem::align_of::() +} + // The shared definition of the `Iter` and `IterMut` iterators macro_rules! iterator { (struct $name:ident -> $ptr:ty, $elem:ty, $mkref:ident) => { @@ -2556,34 +2587,6 @@ macro_rules! iterator { } } -macro_rules! make_slice { - ($start: expr, $end: expr) => {{ - let start = $start; - let diff = ($end as usize).wrapping_sub(start as usize); - if size_from_ptr(start) == 0 { - // use a non-null pointer value - unsafe { from_raw_parts(1 as *const _, diff) } - } else { - let len = diff / size_from_ptr(start); - unsafe { from_raw_parts(start, len) } - } - }} -} - -macro_rules! make_mut_slice { - ($start: expr, $end: expr) => {{ - let start = $start; - let diff = ($end as usize).wrapping_sub(start as usize); - if size_from_ptr(start) == 0 { - // use a non-null pointer value - unsafe { from_raw_parts_mut(1 as *mut _, diff) } - } else { - let len = diff / size_from_ptr(start); - unsafe { from_raw_parts_mut(start, len) } - } - }} -} - /// Immutable slice iterator /// /// This struct is created by the [`iter`] method on [slices]. @@ -2800,11 +2803,10 @@ impl<'a, T> ExactSizeIterator for IterMut<'a, T> { } // Return the number of elements of `T` from `start` to `end`. -// Return the arithmetic difference if `T` is zero size. #[inline(always)] unsafe fn ptrdistance(start: *const T, end: *const T) -> usize { if mem::size_of::() == 0 { - (end as usize).wrapping_sub(start as usize) + (end as usize).wrapping_sub(start as usize) / mem::align_of::() } else { end.offset_from(start) as usize } diff --git a/src/libcore/tests/slice.rs b/src/libcore/tests/slice.rs index 2b37acdfe3e81..a69805513e8c7 100644 --- a/src/libcore/tests/slice.rs +++ b/src/libcore/tests/slice.rs @@ -390,6 +390,75 @@ fn test_windows_zip() { assert_eq!(res, [14, 18, 22, 26]); } +#[test] +#[allow(const_err)] +fn test_iter_consistency() { + use std::fmt::Debug; + use std::mem; + + fn helper(x : T) { + let v : &[T] = &[x, x, x]; + let len = v.len(); + + // TODO: Once #42789 is resolved, also compare the locations with each other + // and with slice patterns. + + for i in 0..len { + let nth = v.iter().nth(i).unwrap(); + assert!(nth as *const _ as usize % mem::align_of::() == 0); + assert_eq!(nth, &x); + } + assert_eq!(v.iter().nth(len), None, "nth(len) should return None"); + + let mut it = v.iter(); + for i in 0..len{ + let remaining = len - i; + assert_eq!(it.size_hint(), (remaining, Some(remaining))); + + let next = it.next().unwrap(); + assert!(next as *const _ as usize % mem::align_of::() == 0); + assert_eq!(next, &x); + } + assert_eq!(it.size_hint(), (0, Some(0))); + assert_eq!(it.next(), None, "The final call to next() should return None"); + } + + fn helper_mut(mut x : T) { + let v : &mut [T] = &mut [x, x, x]; + let len = v.len(); + + // TODO: Once #42789 is resolved, also compare the locations with each other + // and with slice patterns. + + for i in 0..len { + let nth = v.iter_mut().nth(i).unwrap(); + assert!(nth as *mut _ as usize % mem::align_of::() == 0); + assert_eq!(nth, &mut x); + } + assert_eq!(v.iter().nth(len), None, "nth(len) should return None"); + + let mut it = v.iter_mut(); + for i in 0..len { + let remaining = len - i; + assert_eq!(it.size_hint(), (remaining, Some(remaining))); + + let next = it.next().unwrap(); + assert!(next as *mut _ as usize % mem::align_of::() == 0); + assert_eq!(next, &mut x); + } + assert_eq!(it.size_hint(), (0, Some(0))); + assert_eq!(it.next(), None, "The final call to next() should return None"); + } + + // Make sure this works consistently for various types, including ZSTs. + helper(0u32); + helper(()); + helper([0u32; 0]); // ZST with alignment > 0 + helper_mut(0u32); + helper_mut(()); + helper_mut([0u32; 0]); // ZST with alignment > 0 +} + // The current implementation of SliceIndex fails to handle methods // orthogonally from range types; therefore, it is worth testing // all of the indexing operations on each input. From 08962299fd34bbf4ac768510578e7acbd3b92999 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Tue, 10 Jul 2018 09:49:37 +0200 Subject: [PATCH 03/15] pacify tidy --- src/libcore/tests/slice.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libcore/tests/slice.rs b/src/libcore/tests/slice.rs index a69805513e8c7..9f0ebdcefd85f 100644 --- a/src/libcore/tests/slice.rs +++ b/src/libcore/tests/slice.rs @@ -400,7 +400,7 @@ fn test_iter_consistency() { let v : &[T] = &[x, x, x]; let len = v.len(); - // TODO: Once #42789 is resolved, also compare the locations with each other + // FIXME: Once #42789 is resolved, also compare the locations with each other // and with slice patterns. for i in 0..len { @@ -427,7 +427,7 @@ fn test_iter_consistency() { let v : &mut [T] = &mut [x, x, x]; let len = v.len(); - // TODO: Once #42789 is resolved, also compare the locations with each other + // FIXME: Once #42789 is resolved, also compare the locations with each other // and with slice patterns. for i in 0..len { From 3aa38b158952c70254bf642700dd8610d8ee0261 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 11 Jul 2018 13:04:55 +0200 Subject: [PATCH 04/15] slice iterators: ZST iterators no longer just "make up" addresses --- src/libcore/slice/mod.rs | 332 ++++++++++++++----------------------- src/libcore/tests/slice.rs | 95 +++++++---- 2 files changed, 187 insertions(+), 240 deletions(-) diff --git a/src/libcore/slice/mod.rs b/src/libcore/slice/mod.rs index 57f6f4a3b641c..982dc6310e04a 100644 --- a/src/libcore/slice/mod.rs +++ b/src/libcore/slice/mod.rs @@ -45,7 +45,7 @@ use option::Option; use option::Option::{None, Some}; use result::Result; use result::Result::{Ok, Err}; -use ptr::{self, NonNull}; +use ptr; use mem; use marker::{Copy, Send, Sync, Sized, self}; use iter_private::TrustedRandomAccess; @@ -75,70 +75,6 @@ struct FatPtr { // Extension traits // -// Use macros to be generic over const/mut -macro_rules! slice_offset { - ($ptr:expr, $by:expr) => {{ - let ptr = $ptr; - if size_from_ptr(ptr) == 0 { - (ptr as *mut i8).wrapping_offset($by.wrapping_mul(align_from_ptr(ptr) as isize)) as _ - } else { - ptr.offset($by) - } - }}; -} - -// make a &T from a *const T -macro_rules! make_ref { - ($ptr:expr) => {{ - let ptr = $ptr; - if size_from_ptr(ptr) == 0 { - // Use a non-null pointer value - &*(NonNull::dangling().as_ptr() as *const _) - } else { - &*ptr - } - }}; -} - -// make a &mut T from a *mut T -macro_rules! make_ref_mut { - ($ptr:expr) => {{ - let ptr = $ptr; - if size_from_ptr(ptr) == 0 { - // Use a non-null pointer value - &mut *(NonNull::dangling().as_ptr()) - } else { - &mut *ptr - } - }}; -} - -macro_rules! make_slice { - ($start: expr, $end: expr) => {{ - let start = $start; - let len = unsafe { ptrdistance($start, $end) }; - if size_from_ptr(start) == 0 { - // use a non-null pointer value - unsafe { from_raw_parts(NonNull::dangling().as_ptr() as *const _, len) } - } else { - unsafe { from_raw_parts(start, len) } - } - }} -} - -macro_rules! make_mut_slice { - ($start: expr, $end: expr) => {{ - let start = $start; - let len = unsafe { ptrdistance($start, $end) }; - if size_from_ptr(start) == 0 { - // use a non-null pointer value - unsafe { from_raw_parts_mut(NonNull::dangling().as_ptr(), len) } - } else { - unsafe { from_raw_parts_mut(start, len) } - } - }} -} - #[lang = "slice"] #[cfg(not(test))] impl [T] { @@ -606,17 +542,18 @@ impl [T] { #[inline] pub fn iter(&self) -> Iter { unsafe { - let p = if mem::size_of::() == 0 { - NonNull::dangling().as_ptr() as *const _ + let ptr = self.as_ptr(); + assume(!ptr.is_null()); + + let end = if mem::size_of::() == 0 { + (ptr as usize).wrapping_add(self.len()) as *const _ } else { - let p = self.as_ptr(); - assume(!p.is_null()); - p + ptr.offset(self.len() as isize) }; Iter { - ptr: p, - end: slice_offset!(p, self.len() as isize), + ptr, + end, _marker: marker::PhantomData } } @@ -637,17 +574,18 @@ impl [T] { #[inline] pub fn iter_mut(&mut self) -> IterMut { unsafe { - let p = if mem::size_of::() == 0 { - NonNull::dangling().as_ptr() + let ptr = self.as_mut_ptr(); + assume(!ptr.is_null()); + + let end = if mem::size_of::() == 0 { + (ptr as usize).wrapping_add(self.len()) as *mut _ } else { - let p = self.as_mut_ptr(); - assume(!p.is_null()); - p + ptr.offset(self.len() as isize) }; IterMut { - ptr: p, - end: slice_offset!(p, self.len() as isize), + ptr, + end, _marker: marker::PhantomData } } @@ -2399,19 +2337,66 @@ impl<'a, T> IntoIterator for &'a mut [T] { } } -#[inline] -fn size_from_ptr(_: *const T) -> usize { - mem::size_of::() -} - -#[inline] -fn align_from_ptr(_: *const T) -> usize { - mem::align_of::() -} - // The shared definition of the `Iter` and `IterMut` iterators macro_rules! iterator { - (struct $name:ident -> $ptr:ty, $elem:ty, $mkref:ident) => { + (struct $name:ident -> $ptr:ty, $elem:ty, $raw_mut:tt, $( $mut_:tt )*) => { + impl<'a, T> $name<'a, T> { + // Helper function for creating a slice from the iterator. + #[inline(always)] + fn make_slice(&self) -> &'a [T] { + unsafe { from_raw_parts(self.ptr, self.len()) } + } + + // Helper function for moving the start of the iterator forwards by `offset` elements, + // returning the old start. + // Unsafe because the offset must be in-bounds or one-past-the-end. + #[inline(always)] + unsafe fn post_inc_start(&mut self, offset: isize) -> * $raw_mut T { + if mem::size_of::() == 0 { + self.end = (self.end as isize).wrapping_sub(offset) as * $raw_mut T; + self.ptr + } else { + let old = self.ptr; + self.ptr = self.ptr.offset(offset); + old + } + } + + // Helper function for moving the end of the iterator backwards by `offset` elements, + // returning the new end. + // Unsafe because the offset must be in-bounds or one-past-the-end. + #[inline(always)] + unsafe fn pre_dec_end(&mut self, offset: isize) -> * $raw_mut T { + if mem::size_of::() == 0 { + self.end = (self.end as isize).wrapping_sub(offset) as * $raw_mut T; + self.ptr + } else { + self.end = self.end.offset(-offset); + self.end + } + } + } + + #[stable(feature = "rust1", since = "1.0.0")] + impl<'a, T> ExactSizeIterator for $name<'a, T> { + #[inline(always)] + fn len(&self) -> usize { + if mem::size_of::() == 0 { + // end is really ptr+len + (self.end as usize).wrapping_sub(self.ptr as usize) + } else { + unsafe { self.end.offset_from(self.ptr) as usize } + } + } + + #[inline(always)] + fn is_empty(&self) -> bool { + // The way we encode the length of a ZST iterator, this works both for ZST + // and non-ZST. + self.ptr == self.end + } + } + #[stable(feature = "rust1", since = "1.0.0")] impl<'a, T> Iterator for $name<'a, T> { type Item = $elem; @@ -2420,21 +2405,21 @@ macro_rules! iterator { fn next(&mut self) -> Option<$elem> { // could be implemented with slices, but this avoids bounds checks unsafe { + assume(!self.ptr.is_null()); if mem::size_of::() != 0 { - assume(!self.ptr.is_null()); assume(!self.end.is_null()); } - if self.ptr == self.end { + if self.is_empty() { None } else { - Some($mkref!(self.ptr.post_inc())) + Some(& $( $mut_ )* *self.post_inc_start(1)) } } } #[inline] fn size_hint(&self) -> (usize, Option) { - let exact = unsafe { ptrdistance(self.ptr, self.end) }; + let exact = self.len(); (exact, Some(exact)) } @@ -2445,8 +2430,21 @@ macro_rules! iterator { #[inline] fn nth(&mut self, n: usize) -> Option<$elem> { - // Call helper method. Can't put the definition here because mut versus const. - self.iter_nth(n) + if n >= self.len() { + // This iterator is now empty. The way we encode the length of a non-ZST + // iterator, this works for both ZST and non-ZST. + // For a ZST we would usually do `self.end = self.ptr`, but since + // we will not give out an address any more after this there is no + // way to observe the difference. + self.ptr = self.end; + return None; + } + // We are in bounds. `offset` does the right thing even for ZSTs. + unsafe { + let elem = Some(& $( $mut_ )* *self.ptr.offset(n as isize)); + self.post_inc_start((n as isize).wrapping_add(1)); + elem + } } #[inline] @@ -2461,14 +2459,14 @@ macro_rules! iterator { // manual unrolling is needed when there are conditional exits from the loop let mut accum = init; unsafe { - while ptrdistance(self.ptr, self.end) >= 4 { - accum = f(accum, $mkref!(self.ptr.post_inc()))?; - accum = f(accum, $mkref!(self.ptr.post_inc()))?; - accum = f(accum, $mkref!(self.ptr.post_inc()))?; - accum = f(accum, $mkref!(self.ptr.post_inc()))?; + while self.len() >= 4 { + accum = f(accum, & $( $mut_ )* *self.post_inc_start(1))?; + accum = f(accum, & $( $mut_ )* *self.post_inc_start(1))?; + accum = f(accum, & $( $mut_ )* *self.post_inc_start(1))?; + accum = f(accum, & $( $mut_ )* *self.post_inc_start(1))?; } - while self.ptr != self.end { - accum = f(accum, $mkref!(self.ptr.post_inc()))?; + while !self.is_empty() { + accum = f(accum, & $( $mut_ )* *self.post_inc_start(1))?; } } Try::from_ok(accum) @@ -2495,7 +2493,7 @@ macro_rules! iterator { { // The addition might panic on overflow // Use the len of the slice to hint optimizer to remove result index bounds check. - let n = make_slice!(self.ptr, self.end).len(); + let n = self.make_slice().len(); self.try_fold(0, move |i, x| { if predicate(x) { Err(i) } else { Ok(i + 1) } @@ -2514,7 +2512,7 @@ macro_rules! iterator { // No need for an overflow check here, because `ExactSizeIterator` // implies that the number of elements fits into a `usize`. // Use the len of the slice to hint optimizer to remove result index bounds check. - let n = make_slice!(self.ptr, self.end).len(); + let n = self.make_slice().len(); self.try_rfold(n, move |i, x| { let i = i - 1; if predicate(x) { Err(i) } @@ -2533,14 +2531,14 @@ macro_rules! iterator { fn next_back(&mut self) -> Option<$elem> { // could be implemented with slices, but this avoids bounds checks unsafe { + assume(!self.ptr.is_null()); if mem::size_of::() != 0 { - assume(!self.ptr.is_null()); assume(!self.end.is_null()); } - if self.end == self.ptr { + if self.is_empty() { None } else { - Some($mkref!(self.end.pre_dec())) + Some(& $( $mut_ )* *self.pre_dec_end(1)) } } } @@ -2552,14 +2550,14 @@ macro_rules! iterator { // manual unrolling is needed when there are conditional exits from the loop let mut accum = init; unsafe { - while ptrdistance(self.ptr, self.end) >= 4 { - accum = f(accum, $mkref!(self.end.pre_dec()))?; - accum = f(accum, $mkref!(self.end.pre_dec()))?; - accum = f(accum, $mkref!(self.end.pre_dec()))?; - accum = f(accum, $mkref!(self.end.pre_dec()))?; + while self.len() >= 4 { + accum = f(accum, & $( $mut_ )* *self.pre_dec_end(1))?; + accum = f(accum, & $( $mut_ )* *self.pre_dec_end(1))?; + accum = f(accum, & $( $mut_ )* *self.pre_dec_end(1))?; + accum = f(accum, & $( $mut_ )* *self.pre_dec_end(1))?; } - while self.ptr != self.end { - accum = f(accum, $mkref!(self.end.pre_dec()))?; + while !self.is_empty() { + accum = f(accum, & $( $mut_ )* *self.pre_dec_end(1))?; } } Try::from_ok(accum) @@ -2610,7 +2608,9 @@ macro_rules! iterator { #[stable(feature = "rust1", since = "1.0.0")] pub struct Iter<'a, T: 'a> { ptr: *const T, - end: *const T, + end: *const T, // If T is a ZST, this is actually ptr+len. This encoding is picked so that + // ptr == end is a quick test for the Iterator being empty, that works + // for both ZST and non-ZST. _marker: marker::PhantomData<&'a T>, } @@ -2655,32 +2655,11 @@ impl<'a, T> Iter<'a, T> { /// ``` #[stable(feature = "iter_to_slice", since = "1.4.0")] pub fn as_slice(&self) -> &'a [T] { - make_slice!(self.ptr, self.end) - } - - // Helper function for Iter::nth - fn iter_nth(&mut self, n: usize) -> Option<&'a T> { - match self.as_slice().get(n) { - Some(elem_ref) => unsafe { - self.ptr = slice_offset!(self.ptr, (n as isize).wrapping_add(1)); - Some(elem_ref) - }, - None => { - self.ptr = self.end; - None - } - } + self.make_slice() } } -iterator!{struct Iter -> *const T, &'a T, make_ref} - -#[stable(feature = "rust1", since = "1.0.0")] -impl<'a, T> ExactSizeIterator for Iter<'a, T> { - fn is_empty(&self) -> bool { - self.ptr == self.end - } -} +iterator!{struct Iter -> *const T, &'a T, const, /* no mut */} #[stable(feature = "rust1", since = "1.0.0")] impl<'a, T> Clone for Iter<'a, T> { @@ -2721,7 +2700,9 @@ impl<'a, T> AsRef<[T]> for Iter<'a, T> { #[stable(feature = "rust1", since = "1.0.0")] pub struct IterMut<'a, T: 'a> { ptr: *mut T, - end: *mut T, + end: *mut T, // If T is a ZST, this is actually ptr+len. This encoding is picked so that + // ptr == end is a quick test for the Iterator being empty, that works + // for both ZST and non-ZST. _marker: marker::PhantomData<&'a mut T>, } @@ -2729,7 +2710,7 @@ pub struct IterMut<'a, T: 'a> { impl<'a, T: 'a + fmt::Debug> fmt::Debug for IterMut<'a, T> { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { f.debug_tuple("IterMut") - .field(&make_slice!(self.ptr, self.end)) + .field(&self.make_slice()) .finish() } } @@ -2775,76 +2756,11 @@ impl<'a, T> IterMut<'a, T> { /// ``` #[stable(feature = "iter_to_slice", since = "1.4.0")] pub fn into_slice(self) -> &'a mut [T] { - make_mut_slice!(self.ptr, self.end) - } - - // Helper function for IterMut::nth - fn iter_nth(&mut self, n: usize) -> Option<&'a mut T> { - match make_mut_slice!(self.ptr, self.end).get_mut(n) { - Some(elem_ref) => unsafe { - self.ptr = slice_offset!(self.ptr, (n as isize).wrapping_add(1)); - Some(elem_ref) - }, - None => { - self.ptr = self.end; - None - } - } - } -} - -iterator!{struct IterMut -> *mut T, &'a mut T, make_ref_mut} - -#[stable(feature = "rust1", since = "1.0.0")] -impl<'a, T> ExactSizeIterator for IterMut<'a, T> { - fn is_empty(&self) -> bool { - self.ptr == self.end + unsafe { from_raw_parts_mut(self.ptr, self.len()) } } } -// Return the number of elements of `T` from `start` to `end`. -#[inline(always)] -unsafe fn ptrdistance(start: *const T, end: *const T) -> usize { - if mem::size_of::() == 0 { - (end as usize).wrapping_sub(start as usize) / mem::align_of::() - } else { - end.offset_from(start) as usize - } -} - -// Extension methods for raw pointers, used by the iterators -trait PointerExt : Copy { - unsafe fn slice_offset(self, i: isize) -> Self; - - /// Increments `self` by 1, but returns the old value. - #[inline(always)] - unsafe fn post_inc(&mut self) -> Self { - let current = *self; - *self = self.slice_offset(1); - current - } - - /// Decrements `self` by 1, and returns the new value. - #[inline(always)] - unsafe fn pre_dec(&mut self) -> Self { - *self = self.slice_offset(-1); - *self - } -} - -impl PointerExt for *const T { - #[inline(always)] - unsafe fn slice_offset(self, i: isize) -> Self { - slice_offset!(self, i) - } -} - -impl PointerExt for *mut T { - #[inline(always)] - unsafe fn slice_offset(self, i: isize) -> Self { - slice_offset!(self, i) - } -} +iterator!{struct IterMut -> *mut T, &'a mut T, mut, mut} /// An internal abstraction over the splitting iterators, so that /// splitn, splitn_mut etc can be implemented once. diff --git a/src/libcore/tests/slice.rs b/src/libcore/tests/slice.rs index 9f0ebdcefd85f..e4230b0849706 100644 --- a/src/libcore/tests/slice.rs +++ b/src/libcore/tests/slice.rs @@ -392,65 +392,96 @@ fn test_windows_zip() { #[test] #[allow(const_err)] -fn test_iter_consistency() { +fn test_iter_ref_consistency() { use std::fmt::Debug; - use std::mem; fn helper(x : T) { let v : &[T] = &[x, x, x]; + let v_ptrs : [*const T; 3] = match v { + [ref v1, ref v2, ref v3] => [v1 as *const _, v2 as *const _, v3 as *const _], + _ => unreachable!() + }; let len = v.len(); - // FIXME: Once #42789 is resolved, also compare the locations with each other - // and with slice patterns. - for i in 0..len { + assert_eq!(&v[i] as *const _, v_ptrs[i]); // check the v_ptrs array, just to be sure let nth = v.iter().nth(i).unwrap(); - assert!(nth as *const _ as usize % mem::align_of::() == 0); - assert_eq!(nth, &x); + assert_eq!(nth as *const _, v_ptrs[i]); } assert_eq!(v.iter().nth(len), None, "nth(len) should return None"); - let mut it = v.iter(); - for i in 0..len{ - let remaining = len - i; - assert_eq!(it.size_hint(), (remaining, Some(remaining))); + { + let mut it = v.iter(); + for i in 0..len{ + let remaining = len - i; + assert_eq!(it.size_hint(), (remaining, Some(remaining))); - let next = it.next().unwrap(); - assert!(next as *const _ as usize % mem::align_of::() == 0); - assert_eq!(next, &x); + let next = it.next().unwrap(); + assert_eq!(next as *const _, v_ptrs[i]); + } + assert_eq!(it.size_hint(), (0, Some(0))); + assert_eq!(it.next(), None, "The final call to next() should return None"); + } + + { + let mut it = v.iter(); + for i in 0..len{ + let remaining = len - i; + assert_eq!(it.size_hint(), (remaining, Some(remaining))); + + let prev = it.next_back().unwrap(); + assert_eq!(prev as *const _, v_ptrs[remaining-1]); + } + assert_eq!(it.size_hint(), (0, Some(0))); + assert_eq!(it.next_back(), None, "The final call to next_back() should return None"); } - assert_eq!(it.size_hint(), (0, Some(0))); - assert_eq!(it.next(), None, "The final call to next() should return None"); } - fn helper_mut(mut x : T) { + fn helper_mut(x : T) { let v : &mut [T] = &mut [x, x, x]; + let v_ptrs : [*mut T; 3] = match v { + [ref v1, ref v2, ref v3] => + [v1 as *const _ as *mut _, v2 as *const _ as *mut _, v3 as *const _ as *mut _], + _ => unreachable!() + }; let len = v.len(); - // FIXME: Once #42789 is resolved, also compare the locations with each other - // and with slice patterns. - for i in 0..len { + assert_eq!(&mut v[i] as *mut _, v_ptrs[i]); // check the v_ptrs array, just to be sure let nth = v.iter_mut().nth(i).unwrap(); - assert!(nth as *mut _ as usize % mem::align_of::() == 0); - assert_eq!(nth, &mut x); + assert_eq!(nth as *mut _, v_ptrs[i]); } assert_eq!(v.iter().nth(len), None, "nth(len) should return None"); - let mut it = v.iter_mut(); - for i in 0..len { - let remaining = len - i; - assert_eq!(it.size_hint(), (remaining, Some(remaining))); + { + let mut it = v.iter_mut(); + for i in 0..len { + let remaining = len - i; + assert_eq!(it.size_hint(), (remaining, Some(remaining))); + + let next = it.next().unwrap(); + assert_eq!(next as *mut _, v_ptrs[i]); + } + assert_eq!(it.size_hint(), (0, Some(0))); + assert_eq!(it.next(), None, "The final call to next() should return None"); + } + + { + let mut it = v.iter_mut(); + for i in 0..len{ + let remaining = len - i; + assert_eq!(it.size_hint(), (remaining, Some(remaining))); - let next = it.next().unwrap(); - assert!(next as *mut _ as usize % mem::align_of::() == 0); - assert_eq!(next, &mut x); + let prev = it.next_back().unwrap(); + assert_eq!(prev as *mut _, v_ptrs[remaining-1]); + } + assert_eq!(it.size_hint(), (0, Some(0))); + assert_eq!(it.next_back(), None, "The final call to next_back() should return None"); } - assert_eq!(it.size_hint(), (0, Some(0))); - assert_eq!(it.next(), None, "The final call to next() should return None"); } - // Make sure this works consistently for various types, including ZSTs. + // Make sure iterators and slice patterns yield consistent addresses for various types, + // including ZSTs. helper(0u32); helper(()); helper([0u32; 0]); // ZST with alignment > 0 From 8c57009a8787f9be27780541489c2764c7c1f81f Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 11 Jul 2018 21:36:50 +0200 Subject: [PATCH 05/15] comments --- src/libcore/slice/mod.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/libcore/slice/mod.rs b/src/libcore/slice/mod.rs index 982dc6310e04a..c7beec4d5ef20 100644 --- a/src/libcore/slice/mod.rs +++ b/src/libcore/slice/mod.rs @@ -2353,6 +2353,7 @@ macro_rules! iterator { #[inline(always)] unsafe fn post_inc_start(&mut self, offset: isize) -> * $raw_mut T { if mem::size_of::() == 0 { + // This is *reducing* the length. `ptr` never changes with ZST. self.end = (self.end as isize).wrapping_sub(offset) as * $raw_mut T; self.ptr } else { @@ -2434,8 +2435,8 @@ macro_rules! iterator { // This iterator is now empty. The way we encode the length of a non-ZST // iterator, this works for both ZST and non-ZST. // For a ZST we would usually do `self.end = self.ptr`, but since - // we will not give out an address any more after this there is no - // way to observe the difference. + // we will not give out an reference any more after this there is no + // way to observe the difference except for raw pointers. self.ptr = self.end; return None; } From 02ec2886e0453cff71fff3ab1379f00054423049 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 18 Jul 2018 18:29:44 +0200 Subject: [PATCH 06/15] use wrapping_offset; fix logic error in nth --- src/libcore/slice/mod.rs | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/src/libcore/slice/mod.rs b/src/libcore/slice/mod.rs index c7beec4d5ef20..10bf21749a411 100644 --- a/src/libcore/slice/mod.rs +++ b/src/libcore/slice/mod.rs @@ -546,7 +546,7 @@ impl [T] { assume(!ptr.is_null()); let end = if mem::size_of::() == 0 { - (ptr as usize).wrapping_add(self.len()) as *const _ + (ptr as *const u8).wrapping_offset(self.len() as isize) as *const T } else { ptr.offset(self.len() as isize) }; @@ -578,7 +578,7 @@ impl [T] { assume(!ptr.is_null()); let end = if mem::size_of::() == 0 { - (ptr as usize).wrapping_add(self.len()) as *mut _ + (ptr as *mut u8).wrapping_offset(self.len() as isize) as *mut T } else { ptr.offset(self.len() as isize) }; @@ -2354,7 +2354,7 @@ macro_rules! iterator { unsafe fn post_inc_start(&mut self, offset: isize) -> * $raw_mut T { if mem::size_of::() == 0 { // This is *reducing* the length. `ptr` never changes with ZST. - self.end = (self.end as isize).wrapping_sub(offset) as * $raw_mut T; + self.end = (self.end as * $raw_mut u8).wrapping_offset(-offset) as * $raw_mut T; self.ptr } else { let old = self.ptr; @@ -2369,7 +2369,7 @@ macro_rules! iterator { #[inline(always)] unsafe fn pre_dec_end(&mut self, offset: isize) -> * $raw_mut T { if mem::size_of::() == 0 { - self.end = (self.end as isize).wrapping_sub(offset) as * $raw_mut T; + self.end = (self.end as * $raw_mut u8).wrapping_offset(-offset) as * $raw_mut T; self.ptr } else { self.end = self.end.offset(-offset); @@ -2432,12 +2432,14 @@ macro_rules! iterator { #[inline] fn nth(&mut self, n: usize) -> Option<$elem> { if n >= self.len() { - // This iterator is now empty. The way we encode the length of a non-ZST - // iterator, this works for both ZST and non-ZST. - // For a ZST we would usually do `self.end = self.ptr`, but since - // we will not give out an reference any more after this there is no - // way to observe the difference except for raw pointers. - self.ptr = self.end; + // This iterator is now empty. + if mem::size_of::() == 0 { + // We have to do it this way as `ptr` may never be 0, but `end` + // could be (due to wrapping). + self.end = self.ptr; + } else { + self.ptr = self.end; + } return None; } // We are in bounds. `offset` does the right thing even for ZSTs. From 66609fc92ddddb0d0a5d7e3309dc8afe5de0a7cf Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 19 Jul 2018 12:28:43 +0200 Subject: [PATCH 07/15] test nth better --- src/libcore/tests/slice.rs | 48 +++++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 11 deletions(-) diff --git a/src/libcore/tests/slice.rs b/src/libcore/tests/slice.rs index e4230b0849706..7968521f7b461 100644 --- a/src/libcore/tests/slice.rs +++ b/src/libcore/tests/slice.rs @@ -395,7 +395,7 @@ fn test_windows_zip() { fn test_iter_ref_consistency() { use std::fmt::Debug; - fn helper(x : T) { + fn test(x : T) { let v : &[T] = &[x, x, x]; let v_ptrs : [*const T; 3] = match v { [ref v1, ref v2, ref v3] => [v1 as *const _, v2 as *const _, v3 as *const _], @@ -403,6 +403,7 @@ fn test_iter_ref_consistency() { }; let len = v.len(); + // nth(i) for i in 0..len { assert_eq!(&v[i] as *const _, v_ptrs[i]); // check the v_ptrs array, just to be sure let nth = v.iter().nth(i).unwrap(); @@ -410,9 +411,20 @@ fn test_iter_ref_consistency() { } assert_eq!(v.iter().nth(len), None, "nth(len) should return None"); + // stepping through with nth(0) { let mut it = v.iter(); - for i in 0..len{ + for i in 0..len { + let next = it.nth(0).unwrap(); + assert_eq!(next as *const _, v_ptrs[i]); + } + assert_eq!(it.nth(0), None); + } + + // next() + { + let mut it = v.iter(); + for i in 0..len { let remaining = len - i; assert_eq!(it.size_hint(), (remaining, Some(remaining))); @@ -423,9 +435,10 @@ fn test_iter_ref_consistency() { assert_eq!(it.next(), None, "The final call to next() should return None"); } + // next_back() { let mut it = v.iter(); - for i in 0..len{ + for i in 0..len { let remaining = len - i; assert_eq!(it.size_hint(), (remaining, Some(remaining))); @@ -437,7 +450,7 @@ fn test_iter_ref_consistency() { } } - fn helper_mut(x : T) { + fn test_mut(x : T) { let v : &mut [T] = &mut [x, x, x]; let v_ptrs : [*mut T; 3] = match v { [ref v1, ref v2, ref v3] => @@ -446,6 +459,7 @@ fn test_iter_ref_consistency() { }; let len = v.len(); + // nth(i) for i in 0..len { assert_eq!(&mut v[i] as *mut _, v_ptrs[i]); // check the v_ptrs array, just to be sure let nth = v.iter_mut().nth(i).unwrap(); @@ -453,6 +467,17 @@ fn test_iter_ref_consistency() { } assert_eq!(v.iter().nth(len), None, "nth(len) should return None"); + // stepping through with nth(0) + { + let mut it = v.iter(); + for i in 0..len { + let next = it.nth(0).unwrap(); + assert_eq!(next as *const _, v_ptrs[i]); + } + assert_eq!(it.nth(0), None); + } + + // next() { let mut it = v.iter_mut(); for i in 0..len { @@ -466,9 +491,10 @@ fn test_iter_ref_consistency() { assert_eq!(it.next(), None, "The final call to next() should return None"); } + // next_back() { let mut it = v.iter_mut(); - for i in 0..len{ + for i in 0..len { let remaining = len - i; assert_eq!(it.size_hint(), (remaining, Some(remaining))); @@ -482,12 +508,12 @@ fn test_iter_ref_consistency() { // Make sure iterators and slice patterns yield consistent addresses for various types, // including ZSTs. - helper(0u32); - helper(()); - helper([0u32; 0]); // ZST with alignment > 0 - helper_mut(0u32); - helper_mut(()); - helper_mut([0u32; 0]); // ZST with alignment > 0 + test(0u32); + test(()); + test([0u32; 0]); // ZST with alignment > 0 + test_mut(0u32); + test_mut(()); + test_mut([0u32; 0]); // ZST with alignment > 0 } // The current implementation of SliceIndex fails to handle methods From 96d184683ff9fc17b637434af284e7cfcfdb376c Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 19 Jul 2018 12:41:10 +0200 Subject: [PATCH 08/15] make the code for nth closer to what it used to be --- src/libcore/slice/mod.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libcore/slice/mod.rs b/src/libcore/slice/mod.rs index 10bf21749a411..8ae92eb599c27 100644 --- a/src/libcore/slice/mod.rs +++ b/src/libcore/slice/mod.rs @@ -2382,11 +2382,12 @@ macro_rules! iterator { impl<'a, T> ExactSizeIterator for $name<'a, T> { #[inline(always)] fn len(&self) -> usize { + let diff = (self.end as usize).wrapping_sub(self.ptr as usize); if mem::size_of::() == 0 { - // end is really ptr+len - (self.end as usize).wrapping_sub(self.ptr as usize) + // end is really ptr+len, so we are already done + diff } else { - unsafe { self.end.offset_from(self.ptr) as usize } + diff / mem::size_of::() } } From 10079be3cd85d4db0f16ca571e74e0cb51d9e5cf Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 28 Jul 2018 13:20:07 +0200 Subject: [PATCH 09/15] macro-inline len() and is_empty() to fix performance regressions This also changes the IR for nth(), but the new IR actually looks nicer that the old (and it is one instruction shorter). --- src/libcore/slice/mod.rs | 49 ++++++++++++++++++++++++---------------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/src/libcore/slice/mod.rs b/src/libcore/slice/mod.rs index 8ae92eb599c27..be1b27278e250 100644 --- a/src/libcore/slice/mod.rs +++ b/src/libcore/slice/mod.rs @@ -2337,6 +2337,22 @@ impl<'a, T> IntoIterator for &'a mut [T] { } } +// Inlining is_empty and len makes a huge performance difference +macro_rules! is_empty { + // The way we encode the length of a ZST iterator, this works both for ZST + // and non-ZST. + ($self: expr) => {$self.ptr == $self.end} +} +macro_rules! len { + ($T: ty, $self: expr) => {{ + if mem::size_of::<$T>() == 0 { + ($self.end as usize).wrapping_sub($self.ptr as usize) + } else { + $self.end.offset_from($self.ptr) as usize + } + }} +} + // The shared definition of the `Iter` and `IterMut` iterators macro_rules! iterator { (struct $name:ident -> $ptr:ty, $elem:ty, $raw_mut:tt, $( $mut_:tt )*) => { @@ -2344,7 +2360,7 @@ macro_rules! iterator { // Helper function for creating a slice from the iterator. #[inline(always)] fn make_slice(&self) -> &'a [T] { - unsafe { from_raw_parts(self.ptr, self.len()) } + unsafe { from_raw_parts(self.ptr, len!(T, self)) } } // Helper function for moving the start of the iterator forwards by `offset` elements, @@ -2382,20 +2398,12 @@ macro_rules! iterator { impl<'a, T> ExactSizeIterator for $name<'a, T> { #[inline(always)] fn len(&self) -> usize { - let diff = (self.end as usize).wrapping_sub(self.ptr as usize); - if mem::size_of::() == 0 { - // end is really ptr+len, so we are already done - diff - } else { - diff / mem::size_of::() - } + unsafe { len!(T, self) } } #[inline(always)] fn is_empty(&self) -> bool { - // The way we encode the length of a ZST iterator, this works both for ZST - // and non-ZST. - self.ptr == self.end + is_empty!(self) } } @@ -2411,7 +2419,7 @@ macro_rules! iterator { if mem::size_of::() != 0 { assume(!self.end.is_null()); } - if self.is_empty() { + if is_empty!(self) { None } else { Some(& $( $mut_ )* *self.post_inc_start(1)) @@ -2421,7 +2429,7 @@ macro_rules! iterator { #[inline] fn size_hint(&self) -> (usize, Option) { - let exact = self.len(); + let exact = unsafe { len!(T, self) }; (exact, Some(exact)) } @@ -2432,7 +2440,7 @@ macro_rules! iterator { #[inline] fn nth(&mut self, n: usize) -> Option<$elem> { - if n >= self.len() { + if n >= unsafe { len!(T, self) } { // This iterator is now empty. if mem::size_of::() == 0 { // We have to do it this way as `ptr` may never be 0, but `end` @@ -2463,13 +2471,13 @@ macro_rules! iterator { // manual unrolling is needed when there are conditional exits from the loop let mut accum = init; unsafe { - while self.len() >= 4 { + while len!(T, self) >= 4 { accum = f(accum, & $( $mut_ )* *self.post_inc_start(1))?; accum = f(accum, & $( $mut_ )* *self.post_inc_start(1))?; accum = f(accum, & $( $mut_ )* *self.post_inc_start(1))?; accum = f(accum, & $( $mut_ )* *self.post_inc_start(1))?; } - while !self.is_empty() { + while !is_empty!(self) { accum = f(accum, & $( $mut_ )* *self.post_inc_start(1))?; } } @@ -2539,7 +2547,7 @@ macro_rules! iterator { if mem::size_of::() != 0 { assume(!self.end.is_null()); } - if self.is_empty() { + if is_empty!(self) { None } else { Some(& $( $mut_ )* *self.pre_dec_end(1)) @@ -2554,13 +2562,14 @@ macro_rules! iterator { // manual unrolling is needed when there are conditional exits from the loop let mut accum = init; unsafe { - while self.len() >= 4 { + while len!(T, self) >= 4 { accum = f(accum, & $( $mut_ )* *self.pre_dec_end(1))?; accum = f(accum, & $( $mut_ )* *self.pre_dec_end(1))?; accum = f(accum, & $( $mut_ )* *self.pre_dec_end(1))?; accum = f(accum, & $( $mut_ )* *self.pre_dec_end(1))?; } - while !self.is_empty() { + // inlining is_empty everywhere makes a huge performance difference + while !is_empty!(self) { accum = f(accum, & $( $mut_ )* *self.pre_dec_end(1))?; } } @@ -2760,7 +2769,7 @@ impl<'a, T> IterMut<'a, T> { /// ``` #[stable(feature = "iter_to_slice", since = "1.4.0")] pub fn into_slice(self) -> &'a mut [T] { - unsafe { from_raw_parts_mut(self.ptr, self.len()) } + unsafe { from_raw_parts_mut(self.ptr, len!(T, self)) } } } From a4a6b670c88498b22462a6c3561046de34dadecd Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sat, 28 Jul 2018 13:55:17 +0200 Subject: [PATCH 10/15] Use ident, not expr, to avoid accidental side-effects --- src/libcore/slice/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libcore/slice/mod.rs b/src/libcore/slice/mod.rs index be1b27278e250..62ec6281d7b91 100644 --- a/src/libcore/slice/mod.rs +++ b/src/libcore/slice/mod.rs @@ -2341,10 +2341,10 @@ impl<'a, T> IntoIterator for &'a mut [T] { macro_rules! is_empty { // The way we encode the length of a ZST iterator, this works both for ZST // and non-ZST. - ($self: expr) => {$self.ptr == $self.end} + ($self: ident) => {$self.ptr == $self.end} } macro_rules! len { - ($T: ty, $self: expr) => {{ + ($T: ty, $self: ident) => {{ if mem::size_of::<$T>() == 0 { ($self.end as usize).wrapping_sub($self.ptr as usize) } else { From 26e1cd9ed44f25eb23d950cc2822cd1ea30d11bf Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 29 Jul 2018 23:48:18 +0200 Subject: [PATCH 11/15] simplify len macro: No longer require the type --- src/libcore/slice/mod.rs | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/src/libcore/slice/mod.rs b/src/libcore/slice/mod.rs index 62ec6281d7b91..e260bd364a368 100644 --- a/src/libcore/slice/mod.rs +++ b/src/libcore/slice/mod.rs @@ -2337,6 +2337,12 @@ impl<'a, T> IntoIterator for &'a mut [T] { } } +// Macro helper functions +#[inline(always)] +fn size_from_ptr(_: *const T) -> usize { + mem::size_of::() +} + // Inlining is_empty and len makes a huge performance difference macro_rules! is_empty { // The way we encode the length of a ZST iterator, this works both for ZST @@ -2344,11 +2350,12 @@ macro_rules! is_empty { ($self: ident) => {$self.ptr == $self.end} } macro_rules! len { - ($T: ty, $self: ident) => {{ - if mem::size_of::<$T>() == 0 { - ($self.end as usize).wrapping_sub($self.ptr as usize) + ($self: ident) => {{ + let start = $self.ptr; + if size_from_ptr(start) == 0 { + ($self.end as usize).wrapping_sub(start as usize) } else { - $self.end.offset_from($self.ptr) as usize + $self.end.offset_from(start) as usize } }} } @@ -2360,7 +2367,7 @@ macro_rules! iterator { // Helper function for creating a slice from the iterator. #[inline(always)] fn make_slice(&self) -> &'a [T] { - unsafe { from_raw_parts(self.ptr, len!(T, self)) } + unsafe { from_raw_parts(self.ptr, len!(self)) } } // Helper function for moving the start of the iterator forwards by `offset` elements, @@ -2398,7 +2405,7 @@ macro_rules! iterator { impl<'a, T> ExactSizeIterator for $name<'a, T> { #[inline(always)] fn len(&self) -> usize { - unsafe { len!(T, self) } + unsafe { len!(self) } } #[inline(always)] @@ -2429,7 +2436,7 @@ macro_rules! iterator { #[inline] fn size_hint(&self) -> (usize, Option) { - let exact = unsafe { len!(T, self) }; + let exact = unsafe { len!(self) }; (exact, Some(exact)) } @@ -2440,7 +2447,7 @@ macro_rules! iterator { #[inline] fn nth(&mut self, n: usize) -> Option<$elem> { - if n >= unsafe { len!(T, self) } { + if n >= unsafe { len!(self) } { // This iterator is now empty. if mem::size_of::() == 0 { // We have to do it this way as `ptr` may never be 0, but `end` @@ -2471,7 +2478,7 @@ macro_rules! iterator { // manual unrolling is needed when there are conditional exits from the loop let mut accum = init; unsafe { - while len!(T, self) >= 4 { + while len!(self) >= 4 { accum = f(accum, & $( $mut_ )* *self.post_inc_start(1))?; accum = f(accum, & $( $mut_ )* *self.post_inc_start(1))?; accum = f(accum, & $( $mut_ )* *self.post_inc_start(1))?; @@ -2562,7 +2569,7 @@ macro_rules! iterator { // manual unrolling is needed when there are conditional exits from the loop let mut accum = init; unsafe { - while len!(T, self) >= 4 { + while len!(self) >= 4 { accum = f(accum, & $( $mut_ )* *self.pre_dec_end(1))?; accum = f(accum, & $( $mut_ )* *self.pre_dec_end(1))?; accum = f(accum, & $( $mut_ )* *self.pre_dec_end(1))?; @@ -2769,7 +2776,7 @@ impl<'a, T> IterMut<'a, T> { /// ``` #[stable(feature = "iter_to_slice", since = "1.4.0")] pub fn into_slice(self) -> &'a mut [T] { - unsafe { from_raw_parts_mut(self.ptr, len!(T, self)) } + unsafe { from_raw_parts_mut(self.ptr, len!(self)) } } } From f34650ab9fd458b11b3f7e2925c75b048ebd47d0 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 29 Jul 2018 13:41:43 +0200 Subject: [PATCH 12/15] Make sure #47772 does not regress --- .../codegen/slice-position-bounds-check.rs | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 src/test/codegen/slice-position-bounds-check.rs diff --git a/src/test/codegen/slice-position-bounds-check.rs b/src/test/codegen/slice-position-bounds-check.rs new file mode 100644 index 0000000000000..bcbf7fd83cf00 --- /dev/null +++ b/src/test/codegen/slice-position-bounds-check.rs @@ -0,0 +1,34 @@ +// Copyright 2017 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// no-system-llvm +// compile-flags: -O +#![crate_type = "lib"] + +fn search(arr: &mut [T], a: &T) -> Result { + match arr.iter().position(|x| x == a) { + Some(p) => { + Ok(p) + }, + None => Err(()), + } +} + +// CHECK-LABEL: @position_no_bounds_check +#[no_mangle] +pub fn position_no_bounds_check(y: &mut [u32], x: &u32, z: &u32) -> bool { + // This contains "call assume" so we cannot just rule out all calls + // CHECK-NOT: panic + if let Ok(p) = search(y, x) { + y[p] == *z + } else { + false + } +} From c3b0794e2ee5039d60e255ef9921e0a88771b042 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 29 Jul 2018 22:29:49 +0200 Subject: [PATCH 13/15] make sure that the no-panic test tests what it is supposed to test --- src/test/codegen/slice-position-bounds-check.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/test/codegen/slice-position-bounds-check.rs b/src/test/codegen/slice-position-bounds-check.rs index bcbf7fd83cf00..aae81ae84922b 100644 --- a/src/test/codegen/slice-position-bounds-check.rs +++ b/src/test/codegen/slice-position-bounds-check.rs @@ -32,3 +32,11 @@ pub fn position_no_bounds_check(y: &mut [u32], x: &u32, z: &u32) -> bool { false } } + +// just to make sure that panicking really emits "panic" somewhere in the IR +// CHECK-LABEL: @test_check +#[no_mangle] +pub fn test_check() { + // CHECK: panic + unreachable!() +} From b0500c1c4446c178a4bb7ea8ef89c03c8adc9092 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Sun, 29 Jul 2018 23:52:36 +0200 Subject: [PATCH 14/15] Introduce another way to compute the length, to fix position codegen regression --- src/libcore/slice/mod.rs | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/src/libcore/slice/mod.rs b/src/libcore/slice/mod.rs index e260bd364a368..836e067bcdd34 100644 --- a/src/libcore/slice/mod.rs +++ b/src/libcore/slice/mod.rs @@ -2359,6 +2359,22 @@ macro_rules! len { } }} } +// To get rid of some bounds checks (see `position`), for some reason it +// makes a difference to compute the length in this way. +// (Tested by `codegen/slice-position-bounds-check`.) +macro_rules! len2 { + ($self: ident) => {{ + let start = $self.ptr; + let diff = ($self.end as usize).wrapping_sub(start as usize); + let size = size_from_ptr(start); + if size == 0 { + diff + } else { + // Using division instead of `offset_from` helps LLVM remove bounds checks + diff / size + } + }} +} // The shared definition of the `Iter` and `IterMut` iterators macro_rules! iterator { @@ -2367,7 +2383,7 @@ macro_rules! iterator { // Helper function for creating a slice from the iterator. #[inline(always)] fn make_slice(&self) -> &'a [T] { - unsafe { from_raw_parts(self.ptr, len!(self)) } + unsafe { from_raw_parts(self.ptr, len2!(self)) } } // Helper function for moving the start of the iterator forwards by `offset` elements, @@ -2510,9 +2526,8 @@ macro_rules! iterator { Self: Sized, P: FnMut(Self::Item) -> bool, { - // The addition might panic on overflow - // Use the len of the slice to hint optimizer to remove result index bounds check. - let n = self.make_slice().len(); + // The addition might panic on overflow. + let n = len2!(self); self.try_fold(0, move |i, x| { if predicate(x) { Err(i) } else { Ok(i + 1) } @@ -2529,9 +2544,7 @@ macro_rules! iterator { Self: Sized + ExactSizeIterator + DoubleEndedIterator { // No need for an overflow check here, because `ExactSizeIterator` - // implies that the number of elements fits into a `usize`. - // Use the len of the slice to hint optimizer to remove result index bounds check. - let n = self.make_slice().len(); + let n = len2!(self); self.try_rfold(n, move |i, x| { let i = i - 1; if predicate(x) { Err(i) } @@ -2776,7 +2789,7 @@ impl<'a, T> IterMut<'a, T> { /// ``` #[stable(feature = "iter_to_slice", since = "1.4.0")] pub fn into_slice(self) -> &'a mut [T] { - unsafe { from_raw_parts_mut(self.ptr, len!(self)) } + unsafe { from_raw_parts_mut(self.ptr, len2!(self)) } } } From 8dac6e3fdec137094eb52893f37cef62d31deae8 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 30 Jul 2018 09:04:03 +0200 Subject: [PATCH 15/15] use the same length computation everywhere --- src/libcore/slice/mod.rs | 31 ++++++++++--------------------- 1 file changed, 10 insertions(+), 21 deletions(-) diff --git a/src/libcore/slice/mod.rs b/src/libcore/slice/mod.rs index 836e067bcdd34..eeb171ebb9b46 100644 --- a/src/libcore/slice/mod.rs +++ b/src/libcore/slice/mod.rs @@ -2349,20 +2349,9 @@ macro_rules! is_empty { // and non-ZST. ($self: ident) => {$self.ptr == $self.end} } +// To get rid of some bounds checks (see `position`), we compute the length in a somewhat +// unexpected way. (Tested by `codegen/slice-position-bounds-check`.) macro_rules! len { - ($self: ident) => {{ - let start = $self.ptr; - if size_from_ptr(start) == 0 { - ($self.end as usize).wrapping_sub(start as usize) - } else { - $self.end.offset_from(start) as usize - } - }} -} -// To get rid of some bounds checks (see `position`), for some reason it -// makes a difference to compute the length in this way. -// (Tested by `codegen/slice-position-bounds-check`.) -macro_rules! len2 { ($self: ident) => {{ let start = $self.ptr; let diff = ($self.end as usize).wrapping_sub(start as usize); @@ -2383,7 +2372,7 @@ macro_rules! iterator { // Helper function for creating a slice from the iterator. #[inline(always)] fn make_slice(&self) -> &'a [T] { - unsafe { from_raw_parts(self.ptr, len2!(self)) } + unsafe { from_raw_parts(self.ptr, len!(self)) } } // Helper function for moving the start of the iterator forwards by `offset` elements, @@ -2421,7 +2410,7 @@ macro_rules! iterator { impl<'a, T> ExactSizeIterator for $name<'a, T> { #[inline(always)] fn len(&self) -> usize { - unsafe { len!(self) } + len!(self) } #[inline(always)] @@ -2452,18 +2441,18 @@ macro_rules! iterator { #[inline] fn size_hint(&self) -> (usize, Option) { - let exact = unsafe { len!(self) }; + let exact = len!(self); (exact, Some(exact)) } #[inline] fn count(self) -> usize { - self.len() + len!(self) } #[inline] fn nth(&mut self, n: usize) -> Option<$elem> { - if n >= unsafe { len!(self) } { + if n >= len!(self) { // This iterator is now empty. if mem::size_of::() == 0 { // We have to do it this way as `ptr` may never be 0, but `end` @@ -2527,7 +2516,7 @@ macro_rules! iterator { P: FnMut(Self::Item) -> bool, { // The addition might panic on overflow. - let n = len2!(self); + let n = len!(self); self.try_fold(0, move |i, x| { if predicate(x) { Err(i) } else { Ok(i + 1) } @@ -2544,7 +2533,7 @@ macro_rules! iterator { Self: Sized + ExactSizeIterator + DoubleEndedIterator { // No need for an overflow check here, because `ExactSizeIterator` - let n = len2!(self); + let n = len!(self); self.try_rfold(n, move |i, x| { let i = i - 1; if predicate(x) { Err(i) } @@ -2789,7 +2778,7 @@ impl<'a, T> IterMut<'a, T> { /// ``` #[stable(feature = "iter_to_slice", since = "1.4.0")] pub fn into_slice(self) -> &'a mut [T] { - unsafe { from_raw_parts_mut(self.ptr, len2!(self)) } + unsafe { from_raw_parts_mut(self.ptr, len!(self)) } } }