Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

use ManuallyDrop instead of forget inside collections #70766

Merged
merged 1 commit into from
Apr 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion src/liballoc/boxed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -469,8 +469,8 @@ impl<T: ?Sized> Box<T> {
#[inline]
#[doc(hidden)]
pub fn into_unique(b: Box<T>) -> Unique<T> {
let b = mem::ManuallyDrop::new(b);
let mut unique = b.0;
mem::forget(b);
// Box is kind-of a library type, but recognized as a "unique pointer" by
// Stacked Borrows. This function here corresponds to "reborrowing to
// a raw pointer", but there is no actual reborrow here -- so
Expand Down
16 changes: 8 additions & 8 deletions src/liballoc/collections/btree/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ use core::fmt::Debug;
use core::hash::{Hash, Hasher};
use core::iter::{FromIterator, FusedIterator, Peekable};
use core::marker::PhantomData;
use core::mem::{self, ManuallyDrop};
Copy link
Member

Choose a reason for hiding this comment

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

Cc @Mark-Simulacrum @ssomers for BTreeMap changes

Copy link
Contributor

Choose a reason for hiding this comment

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

I hardly understand ManuallyDrop, but isn't it a good fit for btree::node::Root? I don't really understand Root's comment "Note that this does not have a destructor," to begin with. Doesn't Rust generate a destructor if you don't implement Drop? So the comment should say: "Note that we (hopefully) do not invoke the generated destructor, because we need to clean up the entire tree and we do that in a fancy and dangerous way while iterating".

Copy link
Member

Choose a reason for hiding this comment

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

I suppose that comment just means that there is no Drop impl that would clean up the data behind the raw pointers in Root -- usually one would expect such an impl to exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay. But is that something more formally expressed by making Root.node ManuallyDrop? I should just try it and learn.

Copy link
Member

Choose a reason for hiding this comment

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

No, putting a raw pointer (or Unique) behind manually drop is pointless.

use core::ops::Bound::{Excluded, Included, Unbounded};
use core::ops::{Index, RangeBounds};
use core::{fmt, mem, ptr};
use core::{fmt, ptr};

use super::node::{self, marker, ForceResult::*, Handle, InsertResult::*, NodeRef};
use super::search::{self, SearchResult::*};
Expand Down Expand Up @@ -190,9 +191,9 @@ impl<K: Clone, V: Clone> Clone for BTreeMap<K, V> {
// We can't destructure subtree directly
// because BTreeMap implements Drop
let (subroot, sublength) = unsafe {
let subtree = ManuallyDrop::new(subtree);
let root = ptr::read(&subtree.root);
let length = subtree.length;
mem::forget(subtree);
(root, length)
};

Expand Down Expand Up @@ -1515,15 +1516,14 @@ impl<K, V> IntoIterator for BTreeMap<K, V> {
type IntoIter = IntoIter<K, V>;

fn into_iter(self) -> IntoIter<K, V> {
if self.root.is_none() {
mem::forget(self);
let me = ManuallyDrop::new(self);
if me.root.is_none() {
return IntoIter { front: None, back: None, length: 0 };
}

let root1 = unsafe { unwrap_unchecked(ptr::read(&self.root)).into_ref() };
let root2 = unsafe { unwrap_unchecked(ptr::read(&self.root)).into_ref() };
let len = self.length;
mem::forget(self);
let root1 = unsafe { unwrap_unchecked(ptr::read(&me.root)).into_ref() };
let root2 = unsafe { unwrap_unchecked(ptr::read(&me.root)).into_ref() };
let len = me.length;

IntoIter {
front: Some(root1.first_leaf_edge()),
Expand Down
11 changes: 5 additions & 6 deletions src/liballoc/collections/vec_deque.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use core::cmp::{self, Ordering};
use core::fmt;
use core::hash::{Hash, Hasher};
use core::iter::{once, repeat_with, FromIterator, FusedIterator};
use core::mem::{self, replace};
use core::mem::{self, replace, ManuallyDrop};
use core::ops::Bound::{Excluded, Included, Unbounded};
use core::ops::{Index, IndexMut, RangeBounds, Try};
use core::ptr::{self, NonNull};
Expand Down Expand Up @@ -2898,12 +2898,12 @@ impl<T> From<Vec<T>> for VecDeque<T> {
/// This avoids reallocating where possible, but the conditions for that are
/// strict, and subject to change, and so shouldn't be relied upon unless the
/// `Vec<T>` came from `From<VecDeque<T>>` and hasn't been reallocated.
fn from(mut other: Vec<T>) -> Self {
fn from(other: Vec<T>) -> Self {
unsafe {
let mut other = ManuallyDrop::new(other);
let other_buf = other.as_mut_ptr();
let mut buf = RawVec::from_raw_parts(other_buf, other.capacity());
let len = other.len();
mem::forget(other);

// We need to extend the buf if it's not a power of two, too small
// or doesn't have at least one free space
Expand Down Expand Up @@ -2955,16 +2955,15 @@ impl<T> From<VecDeque<T>> for Vec<T> {
other.make_contiguous();

unsafe {
let other = ManuallyDrop::new(other);
let buf = other.buf.ptr();
let len = other.len();
let cap = other.cap();

if other.head != 0 {
ptr::copy(buf.add(other.tail), buf, len);
}
let out = Vec::from_raw_parts(buf, len, cap);
mem::forget(other);
out
Vec::from_raw_parts(buf, len, cap)
}
}
}
16 changes: 7 additions & 9 deletions src/liballoc/raw_vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

use core::alloc::MemoryBlock;
use core::cmp;
use core::mem::{self, MaybeUninit};
use core::mem::{self, ManuallyDrop, MaybeUninit};
use core::ops::Drop;
use core::ptr::{NonNull, Unique};
use core::slice;
Expand Down Expand Up @@ -112,11 +112,10 @@ impl<T> RawVec<T, Global> {
}

/// Converts a `Box<[T]>` into a `RawVec<T>`.
pub fn from_box(mut slice: Box<[T]>) -> Self {
pub fn from_box(slice: Box<[T]>) -> Self {
unsafe {
let result = RawVec::from_raw_parts(slice.as_mut_ptr(), slice.len());
mem::forget(slice);
result
let mut slice = ManuallyDrop::new(slice);
RawVec::from_raw_parts(slice.as_mut_ptr(), slice.len())
}
}
}
Expand Down Expand Up @@ -579,11 +578,10 @@ impl<T> RawVec<T, Global> {
"`len` must be smaller than or equal to `self.capacity()`"
);

let me = ManuallyDrop::new(self);
// NOTE: not calling `capacity()` here; actually using the real `cap` field!
Copy link
Member

Choose a reason for hiding this comment

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

Any idea what is up with that comment? cap is not used here, before not after your change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems that this happened recently in #70362, specifically in commit 2526acc.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like just an outdated comment then. Do you want to remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR is already r+-and-rollup-ed, so I'll remove the comment once the PR is merged to avoid merge issues.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I'll just add it to #70776 then

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the comment is outdated and I forgot to remove it.

let slice = slice::from_raw_parts_mut(self.ptr() as *mut MaybeUninit<T>, len);
let output = Box::from_raw(slice);
mem::forget(self);
output
let slice = slice::from_raw_parts_mut(me.ptr() as *mut MaybeUninit<T>, len);
Box::from_raw(slice)
}
}

Expand Down
27 changes: 13 additions & 14 deletions src/liballoc/vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ use core::hash::{self, Hash};
use core::intrinsics::{arith_offset, assume};
use core::iter::{FromIterator, FusedIterator, TrustedLen};
use core::marker::PhantomData;
use core::mem;
use core::mem::{self, ManuallyDrop};
use core::ops::Bound::{Excluded, Included, Unbounded};
use core::ops::{self, Index, IndexMut, RangeBounds};
use core::ptr::{self, NonNull};
Expand Down Expand Up @@ -392,7 +392,7 @@ impl<T> Vec<T> {
/// ```
#[unstable(feature = "vec_into_raw_parts", reason = "new API", issue = "65816")]
pub fn into_raw_parts(self) -> (*mut T, usize, usize) {
let mut me = mem::ManuallyDrop::new(self);
let mut me = ManuallyDrop::new(self);
(me.as_mut_ptr(), me.len(), me.capacity())
}

Expand Down Expand Up @@ -678,9 +678,9 @@ impl<T> Vec<T> {
pub fn into_boxed_slice(mut self) -> Box<[T]> {
unsafe {
self.shrink_to_fit();
let buf = ptr::read(&self.buf);
let len = self.len();
mem::forget(self);
let me = ManuallyDrop::new(self);
let buf = ptr::read(&me.buf);
let len = me.len();
buf.into_box(len).assume_init()
}
}
Expand Down Expand Up @@ -1949,16 +1949,16 @@ impl<T> IntoIterator for Vec<T> {
/// }
/// ```
#[inline]
fn into_iter(mut self) -> IntoIter<T> {
fn into_iter(self) -> IntoIter<T> {
unsafe {
let begin = self.as_mut_ptr();
let mut me = ManuallyDrop::new(self);
let begin = me.as_mut_ptr();
let end = if mem::size_of::<T>() == 0 {
arith_offset(begin as *const i8, self.len() as isize) as *const T
arith_offset(begin as *const i8, me.len() as isize) as *const T
} else {
begin.add(self.len()) as *const T
begin.add(me.len()) as *const T
};
let cap = self.buf.capacity();
mem::forget(self);
let cap = me.buf.capacity();
IntoIter {
buf: NonNull::new_unchecked(begin),
phantom: PhantomData,
Expand Down Expand Up @@ -2081,9 +2081,8 @@ impl<T> SpecExtend<T, IntoIter<T>> for Vec<T> {
// has not been advanced at all.
if iterator.buf.as_ptr() as *const _ == iterator.ptr {
unsafe {
let vec = Vec::from_raw_parts(iterator.buf.as_ptr(), iterator.len(), iterator.cap);
mem::forget(iterator);
vec
let it = ManuallyDrop::new(iterator);
Vec::from_raw_parts(it.buf.as_ptr(), it.len(), it.cap)
}
} else {
let mut vector = Vec::new();
Expand Down