Skip to content

Commit

Permalink
Auto merge of #162 - thomcc:maybe-uninit, r=emilio
Browse files Browse the repository at this point in the history 
Use MaybeUninit for storage of inline items

Fixes #126

I didn't see a PR open for this, and was poking around so I figured I'd do it. Sorry if I stepped on anyone's toes.

This should remove the use of mem::uninitialized, and ensure that all values used directly or via references are initialized. (e.g. T and &T are always initialized, but `*const T` and `*mut T` may not be, which is fine).

With these changes, `miri` passes. I also changed the CI to run `miri`, hopefully.

That said, this includes three breaking changes, so it will require a min version bump.

1. The functions on the `Array` trait `ptr` and `ptr_mut` have been removed. Because these took a `&self`/`&mut self` argument, there's no way for us to call them when we only have a `MaybeUninit<A>`. Now, we just use the memory of the object directly.

    This limits the flexibility of custom implementations of `Array`, as they can no longer return pointers to values other than themselves, but hopefully nobody does that (IMO there's a pretty good case for sealing `Array`...)

2. `SmallVec::from_buf_and_len_unchecked` now takes a MaybeUninit<A> and not an A. This means that callers have the option of only partially initializing said array without risking nasal demons. I can undo this change if desired, as it's

3. Rust 1.36.0 is required to use MaybeUninit, so this bumps the MSRV from 1.20.0 to 1.36.0. It seems tricky to make the use of MaybeUninit optional here, at least without a bunch or work (ignoring the fact that doing so would require adding a build.rs which parses `rustc -Vv` to tell us the version).

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-smallvec/162)
<!-- Reviewable:end -->
  • Loading branch information
bors-servo committed Oct 19, 2019
2 parents ae79432 + 28c09eb commit 690d65e
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 54 deletions.
5 changes: 3 additions & 2 deletions .travis.yml
View file
@@ -1,6 +1,6 @@
language: rust
rust:
- 1.20.0
- 1.36.0
- nightly
- beta
- stable
Expand All @@ -11,4 +11,5 @@ script: |
([ $TRAVIS_RUST_VERSION != nightly ] || cargo check --verbose --no-default-features) &&
([ $TRAVIS_RUST_VERSION != nightly ] || cargo test --verbose --features union) &&
([ $TRAVIS_RUST_VERSION != nightly ] || cargo test --verbose --all-features) &&
([ $TRAVIS_RUST_VERSION != nightly ] || cargo bench --verbose bench)
([ $TRAVIS_RUST_VERSION != nightly ] || cargo bench --verbose bench) &&
([ $TRAVIS_RUST_VERSION != nightly ] || bash ./scripts/run_miri.sh)
101 changes: 49 additions & 52 deletions lib.rs
View file
Expand Up @@ -63,7 +63,7 @@ use std::iter::{repeat, FromIterator, IntoIterator};
#[cfg(feature = "serde")]
use std::marker::PhantomData;
use std::mem;
use std::mem::ManuallyDrop;
use std::mem::MaybeUninit;
use std::ops;
use std::ptr;
use std::slice;
Expand Down Expand Up @@ -280,29 +280,27 @@ impl<'a, T: 'a> Drop for Drain<'a, T> {

#[cfg(feature = "union")]
union SmallVecData<A: Array> {
inline: ManuallyDrop<A>,
inline: MaybeUninit<A>,
heap: (*mut A::Item, usize),
}

#[cfg(feature = "union")]
impl<A: Array> SmallVecData<A> {
#[inline]
unsafe fn inline(&self) -> &A {
&self.inline
unsafe fn inline(&self) -> *const A::Item {
self.inline.as_ptr() as *const A::Item
}
#[inline]
unsafe fn inline_mut(&mut self) -> &mut A {
&mut self.inline
unsafe fn inline_mut(&mut self) -> *mut A::Item {
self.inline.as_mut_ptr() as *mut A::Item
}
#[inline]
fn from_inline(inline: A) -> SmallVecData<A> {
SmallVecData {
inline: ManuallyDrop::new(inline),
}
fn from_inline(inline: MaybeUninit<A>) -> SmallVecData<A> {
SmallVecData { inline }
}
#[inline]
unsafe fn into_inline(self) -> A {
ManuallyDrop::into_inner(self.inline)
unsafe fn into_inline(self) -> MaybeUninit<A> {
self.inline
}
#[inline]
unsafe fn heap(&self) -> (*mut A::Item, usize) {
Expand All @@ -320,34 +318,34 @@ impl<A: Array> SmallVecData<A> {

#[cfg(not(feature = "union"))]
enum SmallVecData<A: Array> {
Inline(ManuallyDrop<A>),
Inline(MaybeUninit<A>),
Heap((*mut A::Item, usize)),
}

#[cfg(not(feature = "union"))]
impl<A: Array> SmallVecData<A> {
#[inline]
unsafe fn inline(&self) -> &A {
unsafe fn inline(&self) -> *const A::Item {
match *self {
SmallVecData::Inline(ref a) => a,
SmallVecData::Inline(ref a) => a.as_ptr() as *const A::Item,
_ => debug_unreachable!(),
}
}
#[inline]
unsafe fn inline_mut(&mut self) -> &mut A {
unsafe fn inline_mut(&mut self) -> *mut A::Item {
match *self {
SmallVecData::Inline(ref mut a) => a,
SmallVecData::Inline(ref mut a) => a.as_mut_ptr() as *mut A::Item,
_ => debug_unreachable!(),
}
}
#[inline]
fn from_inline(inline: A) -> SmallVecData<A> {
SmallVecData::Inline(ManuallyDrop::new(inline))
fn from_inline(inline: MaybeUninit<A>) -> SmallVecData<A> {
SmallVecData::Inline(inline)
}
#[inline]
unsafe fn into_inline(self) -> A {
unsafe fn into_inline(self) -> MaybeUninit<A> {
match self {
SmallVecData::Inline(a) => ManuallyDrop::into_inner(a),
SmallVecData::Inline(a) => a,
_ => debug_unreachable!(),
}
}
Expand Down Expand Up @@ -412,11 +410,15 @@ impl<A: Array> SmallVec<A> {
/// Construct an empty vector
#[inline]
pub fn new() -> SmallVec<A> {
unsafe {
SmallVec {
capacity: 0,
data: SmallVecData::from_inline(mem::uninitialized()),
}
// Try to detect invalid custom implementations of `Array`. Hopefuly,
// this check should be optimized away entirely for valid ones.
assert!(
mem::size_of::<A>() == A::size() * mem::size_of::<A::Item>()
&& mem::align_of::<A>() >= mem::align_of::<A::Item>()
);
SmallVec {
capacity: 0,
data: SmallVecData::from_inline(MaybeUninit::uninit()),
}
}

Expand Down Expand Up @@ -456,10 +458,10 @@ impl<A: Array> SmallVec<A> {
pub fn from_vec(mut vec: Vec<A::Item>) -> SmallVec<A> {
if vec.capacity() <= A::size() {
unsafe {
let mut data = SmallVecData::<A>::from_inline(mem::uninitialized());
let mut data = SmallVecData::<A>::from_inline(MaybeUninit::uninit());
let len = vec.len();
vec.set_len(0);
ptr::copy_nonoverlapping(vec.as_ptr(), data.inline_mut().ptr_mut(), len);
ptr::copy_nonoverlapping(vec.as_ptr(), data.inline_mut(), len);

SmallVec {
capacity: len,
Expand Down Expand Up @@ -492,7 +494,7 @@ impl<A: Array> SmallVec<A> {
pub fn from_buf(buf: A) -> SmallVec<A> {
SmallVec {
capacity: A::size(),
data: SmallVecData::from_inline(buf),
data: SmallVecData::from_inline(MaybeUninit::new(buf)),
}
}

Expand All @@ -511,7 +513,7 @@ impl<A: Array> SmallVec<A> {
#[inline]
pub fn from_buf_and_len(buf: A, len: usize) -> SmallVec<A> {
assert!(len <= A::size());
unsafe { SmallVec::from_buf_and_len_unchecked(buf, len) }
unsafe { SmallVec::from_buf_and_len_unchecked(MaybeUninit::new(buf), len) }
}

/// Constructs a new `SmallVec` on the stack from an `A` without
Expand All @@ -520,16 +522,17 @@ impl<A: Array> SmallVec<A> {
///
/// ```rust
/// use smallvec::SmallVec;
/// use std::mem::MaybeUninit;
///
/// let buf = [1, 2, 3, 4, 5, 0, 0, 0];
/// let small_vec: SmallVec<_> = unsafe {
/// SmallVec::from_buf_and_len_unchecked(buf, 5)
/// SmallVec::from_buf_and_len_unchecked(MaybeUninit::new(buf), 5)
/// };
///
/// assert_eq!(&*small_vec, &[1, 2, 3, 4, 5]);
/// ```
#[inline]
pub unsafe fn from_buf_and_len_unchecked(buf: A, len: usize) -> SmallVec<A> {
pub unsafe fn from_buf_and_len_unchecked(buf: MaybeUninit<A>, len: usize) -> SmallVec<A> {
SmallVec {
capacity: len,
data: SmallVecData::from_inline(buf),
Expand Down Expand Up @@ -579,7 +582,7 @@ impl<A: Array> SmallVec<A> {
let (ptr, len) = self.data.heap();
(ptr, len, self.capacity)
} else {
(self.data.inline().ptr(), self.capacity, A::size())
(self.data.inline(), self.capacity, A::size())
}
}
}
Expand All @@ -592,11 +595,7 @@ impl<A: Array> SmallVec<A> {
let &mut (ptr, ref mut len_ptr) = self.data.heap_mut();
(ptr, len_ptr, self.capacity)
} else {
(
self.data.inline_mut().ptr_mut(),
&mut self.capacity,
A::size(),
)
(self.data.inline_mut(), &mut self.capacity, A::size())
}
}
}
Expand Down Expand Up @@ -663,8 +662,8 @@ impl<A: Array> SmallVec<A> {
if unspilled {
return;
}
self.data = SmallVecData::from_inline(mem::uninitialized());
ptr::copy_nonoverlapping(ptr, self.data.inline_mut().ptr_mut(), len);
self.data = SmallVecData::from_inline(MaybeUninit::uninit());
ptr::copy_nonoverlapping(ptr, self.data.inline_mut(), len);
self.capacity = len;
} else if new_cap != cap {
let mut vec = Vec::with_capacity(new_cap);
Expand Down Expand Up @@ -730,8 +729,8 @@ impl<A: Array> SmallVec<A> {
if self.inline_size() >= len {
unsafe {
let (ptr, len) = self.data.heap();
self.data = SmallVecData::from_inline(mem::uninitialized());
ptr::copy_nonoverlapping(ptr, self.data.inline_mut().ptr_mut(), len);
self.data = SmallVecData::from_inline(MaybeUninit::uninit());
ptr::copy_nonoverlapping(ptr, self.data.inline_mut(), len);
deallocate(ptr, self.capacity);
self.capacity = len;
}
Expand Down Expand Up @@ -900,7 +899,7 @@ impl<A: Array> SmallVec<A> {
unsafe {
let data = ptr::read(&self.data);
mem::forget(self);
Ok(data.into_inline())
Ok(data.into_inline().assume_init())
}
}
}
Expand Down Expand Up @@ -1062,8 +1061,12 @@ where
SmallVec {
capacity: len,
data: SmallVecData::from_inline(unsafe {
let mut data: A = mem::uninitialized();
ptr::copy_nonoverlapping(slice.as_ptr(), data.ptr_mut(), len);
let mut data: MaybeUninit<A> = MaybeUninit::uninit();
ptr::copy_nonoverlapping(
slice.as_ptr(),
data.as_mut_ptr() as *mut A::Item,
len,
);
data
}),
}
Expand Down Expand Up @@ -1603,10 +1606,6 @@ pub unsafe trait Array {
type Item;
/// Returns the number of items the array can hold.
fn size() -> usize;
/// Returns a pointer to the first element of the array.
fn ptr(&self) -> *const Self::Item;
/// Returns a mutable pointer to the first element of the array.
fn ptr_mut(&mut self) -> *mut Self::Item;
}

/// Set the length of the vec when the `SetLenOnDrop` value goes out of scope.
Expand Down Expand Up @@ -1650,8 +1649,6 @@ macro_rules! impl_array(
unsafe impl<T> Array for [T; $size] {
type Item = T;
fn size() -> usize { $size }
fn ptr(&self) -> *const T { self.as_ptr() }
fn ptr_mut(&mut self) -> *mut T { self.as_mut_ptr() }
}
)+
}
Expand Down Expand Up @@ -1985,7 +1982,7 @@ mod tests {
);
}

#[cfg(feature = "std")]
#[cfg(all(feature = "std", not(miri)))] // Miri currently does not support unwinding
#[test]
// https://github.com/servo/rust-smallvec/issues/96
fn test_insert_many_panic() {
Expand Down
21 changes: 21 additions & 0 deletions scripts/run_miri.sh
View file
@@ -0,0 +1,21 @@
#!/usr/bin/bash

set -ex

# Clean out our target dir, which may have artifacts compiled by a version of
# rust different from the one we're about to download.
cargo clean

# Install and run the latest version of nightly where miri built successfully.
# Taken from: https://github.com/rust-lang/miri#running-miri-on-ci

MIRI_NIGHTLY=nightly-$(curl -s https://rust-lang.github.io/rustup-components-history/x86_64-unknown-linux-gnu/miri)
echo "Installing latest nightly with Miri: $MIRI_NIGHTLY"
rustup default "$MIRI_NIGHTLY"

rustup component add miri
cargo miri setup

cargo miri test --verbose -- -- -Zunstable-options --exclude-should-panic
cargo miri test --verbose --features union -- -- -Zunstable-options --exclude-should-panic
cargo miri test --verbose --all-features -- -- -Zunstable-options --exclude-should-panic

0 comments on commit 690d65e

Please sign in to comment.