Skip to content

Use zeroed allocations in the mir interpreter instead eagerly touching the memory #87777

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

Merged
merged 6 commits into from
Aug 6, 2021
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: 2 additions & 0 deletions compiler/rustc_middle/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
//! This API is completely unstable and subject to change.

#![doc(html_root_url = "https://doc.rust-lang.org/nightly/nightly-rustc/")]
#![feature(allocator_api)]
#![feature(array_windows)]
#![feature(assert_matches)]
#![feature(backtrace)]
Expand All @@ -33,6 +34,7 @@
#![feature(discriminant_kind)]
#![feature(never_type)]
#![feature(extern_types)]
#![feature(new_uninit)]
#![feature(nll)]
#![feature(once_cell)]
#![feature(min_specialization)]
Expand Down
10 changes: 5 additions & 5 deletions compiler/rustc_middle/src/mir/interpret/allocation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ use crate::ty;
pub struct Allocation<Tag = AllocId, Extra = ()> {
/// The actual bytes of the allocation.
/// Note that the bytes of a pointer represent the offset of the pointer.
bytes: Vec<u8>,
bytes: Box<[u8]>,
Copy link
Member

Choose a reason for hiding this comment

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

Turns out changing this from Vec to Box introduced UB into the interpreter itself... also see Zulip

/// Maps from byte addresses to extra data for each pointer.
/// Only the first byte of a pointer is inserted into the map; i.e.,
/// every entry in this map applies to `pointer_size` consecutive bytes starting
Expand Down Expand Up @@ -112,7 +112,7 @@ impl<Tag> Allocation<Tag> {
align: Align,
mutability: Mutability,
) -> Self {
let bytes = slice.into().into_owned();
let bytes = Box::<[u8]>::from(slice.into());
let size = Size::from_bytes(bytes.len());
Self {
bytes,
Expand All @@ -131,8 +131,7 @@ impl<Tag> Allocation<Tag> {
/// Try to create an Allocation of `size` bytes, failing if there is not enough memory
/// available to the compiler to do so.
pub fn uninit(size: Size, align: Align, panic_on_fail: bool) -> InterpResult<'static, Self> {
let mut bytes = Vec::new();
bytes.try_reserve(size.bytes_usize()).map_err(|_| {
let bytes = Box::<[u8]>::try_new_zeroed_slice(size.bytes_usize()).map_err(|_| {
// This results in an error that can happen non-deterministically, since the memory
// available to the compiler can change between runs. Normally queries are always
// deterministic. However, we can be non-determinstic here because all uses of const
Expand All @@ -146,7 +145,8 @@ impl<Tag> Allocation<Tag> {
});
InterpError::ResourceExhaustion(ResourceExhaustionInfo::MemoryExhausted)
})?;
bytes.resize(size.bytes_usize(), 0);
// SAFETY: the box was zero-allocated, which is a valid initial value for Box<[u8]>
let bytes = unsafe { bytes.assume_init() };
Ok(Allocation {
bytes,
relocations: Relocations::new(),
Expand Down
66 changes: 65 additions & 1 deletion library/alloc/src/boxed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,6 @@ use crate::alloc::{handle_alloc_error, WriteCloneIntoRaw};
use crate::alloc::{AllocError, Allocator, Global, Layout};
#[cfg(not(no_global_oom_handling))]
use crate::borrow::Cow;
#[cfg(not(no_global_oom_handling))]
use crate::raw_vec::RawVec;
#[cfg(not(no_global_oom_handling))]
use crate::str::from_boxed_utf8_unchecked;
Expand Down Expand Up @@ -589,6 +588,71 @@ impl<T> Box<[T]> {
pub fn new_zeroed_slice(len: usize) -> Box<[mem::MaybeUninit<T>]> {
unsafe { RawVec::with_capacity_zeroed(len).into_box(len) }
}

/// Constructs a new boxed slice with uninitialized contents. Returns an error if
/// the allocation fails
///
/// # Examples
///
/// ```
/// #![feature(allocator_api, new_uninit)]
///
/// let mut values = Box::<[u32]>::try_new_uninit_slice(3)?;
/// let values = unsafe {
/// // Deferred initialization:
/// values[0].as_mut_ptr().write(1);
/// values[1].as_mut_ptr().write(2);
/// values[2].as_mut_ptr().write(3);
/// values.assume_init()
/// };
///
/// assert_eq!(*values, [1, 2, 3]);
/// # Ok::<(), std::alloc::AllocError>(())
/// ```
#[unstable(feature = "allocator_api", issue = "32838")]
#[inline]
pub fn try_new_uninit_slice(len: usize) -> Result<Box<[mem::MaybeUninit<T>]>, AllocError> {
unsafe {
let layout = match Layout::array::<mem::MaybeUninit<T>>(len) {
Ok(l) => l,
Err(_) => return Err(AllocError),
};
let ptr = Global.allocate(layout)?;
Ok(RawVec::from_raw_parts_in(ptr.as_mut_ptr() as *mut _, len, Global).into_box(len))
}
}

/// Constructs a new boxed slice with uninitialized contents, with the memory
Copy link
Member

Choose a reason for hiding this comment

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

Saying 'uninitialized contents' sounds confusing when we also state that the memory is zeroed. I think it would be better to write out MaybeUninit<T>, to make it clear that the allocated memory is initialized.

Copy link
Member Author

Choose a reason for hiding this comment

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

I copied that from another _zeroed function, should I fix those too?

/// being filled with `0` bytes. Returns an error if the allocation fails
///
/// See [`MaybeUninit::zeroed`][zeroed] for examples of correct and incorrect usage
/// of this method.
///
/// # Examples
///
/// ```
/// #![feature(allocator_api, new_uninit)]
///
/// let values = Box::<[u32]>::try_new_zeroed_slice(3)?;
/// let values = unsafe { values.assume_init() };
///
/// assert_eq!(*values, [0, 0, 0]);
/// # Ok::<(), std::alloc::AllocError>(())
/// ```
///
/// [zeroed]: mem::MaybeUninit::zeroed
#[unstable(feature = "allocator_api", issue = "32838")]
#[inline]
pub fn try_new_zeroed_slice(len: usize) -> Result<Box<[mem::MaybeUninit<T>]>, AllocError> {
unsafe {
let layout = match Layout::array::<mem::MaybeUninit<T>>(len) {
Ok(l) => l,
Err(_) => return Err(AllocError),
};
let ptr = Global.allocate_zeroed(layout)?;
Ok(RawVec::from_raw_parts_in(ptr.as_mut_ptr() as *mut _, len, Global).into_box(len))
}
}
}

impl<T, A: Allocator> Box<[T], A> {
Expand Down