Skip to content

Clean up AllocRef implementation and documentation #75657

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.

Sign up for GitHub

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 3 commits into from
Aug 18, 2020
Merged

Clean up AllocRef implementation and documentation #75657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a9fe0ca
Clean up AllocRef implementation and documentation
TimDiekmann Aug 18, 2020
66a6512
Add comment, which was removed by accident (again)
TimDiekmann Aug 18, 2020
63d241a
Make `grow_impl` unsafe
TimDiekmann Aug 18, 2020
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
148 changes: 74 additions & 74 deletions library/alloc/src/alloc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,30 +161,69 @@ pub unsafe fn alloc_zeroed(layout: Layout) -> *mut u8 {
unsafe { __rust_alloc_zeroed(layout.size(), layout.align()) }
}

impl Global {
#[inline]
fn alloc_impl(&mut self, layout: Layout, zeroed: bool) -> Result<NonNull<[u8]>, AllocErr> {
match layout.size() {
0 => Ok(NonNull::slice_from_raw_parts(layout.dangling(), 0)),
// SAFETY: `layout` is non-zero in size,
size => unsafe {
let raw_ptr = if zeroed { alloc_zeroed(layout) } else { alloc(layout) };
let ptr = NonNull::new(raw_ptr).ok_or(AllocErr)?;
Ok(NonNull::slice_from_raw_parts(ptr, size))
},
}
}

// Safety: Same as `AllocRef::grow`
#[inline]
unsafe fn grow_impl(
&mut self,
ptr: NonNull<u8>,
layout: Layout,
new_size: usize,
zeroed: bool,
) -> Result<NonNull<[u8]>, AllocErr> {
debug_assert!(
new_size >= layout.size(),
"`new_size` must be greater than or equal to `layout.size()`"
);

match layout.size() {
// SAFETY: the caller must ensure that the `new_size` does not overflow.
// `layout.align()` comes from a `Layout` and is thus guaranteed to be valid for a Layout.
0 => unsafe {
let new_layout = Layout::from_size_align_unchecked(new_size, layout.align());
self.alloc_impl(new_layout, zeroed)
},

// SAFETY: `new_size` is non-zero as `old_size` is greater than or equal to `new_size`
// as required by safety conditions. Other conditions must be upheld by the caller
old_size => unsafe {
// `realloc` probably checks for `new_size >= size` or something similar.
intrinsics::assume(new_size >= layout.size());

let raw_ptr = realloc(ptr.as_ptr(), layout, new_size);
let ptr = NonNull::new(raw_ptr).ok_or(AllocErr)?;
if zeroed {
raw_ptr.add(old_size).write_bytes(0, new_size - old_size);
}
Ok(NonNull::slice_from_raw_parts(ptr, new_size))
},
}
}
}

#[unstable(feature = "allocator_api", issue = "32838")]
unsafe impl AllocRef for Global {
#[inline]
fn alloc(&mut self, layout: Layout) -> Result<NonNull<[u8]>, AllocErr> {
let size = layout.size();
let ptr = if size == 0 {
layout.dangling()
} else {
// SAFETY: `layout` is non-zero in size,
unsafe { NonNull::new(alloc(layout)).ok_or(AllocErr)? }
};
Ok(NonNull::slice_from_raw_parts(ptr, size))
self.alloc_impl(layout, false)
}

#[inline]
fn alloc_zeroed(&mut self, layout: Layout) -> Result<NonNull<[u8]>, AllocErr> {
let size = layout.size();
let ptr = if size == 0 {
layout.dangling()
} else {
// SAFETY: `layout` is non-zero in size,
unsafe { NonNull::new(alloc_zeroed(layout)).ok_or(AllocErr)? }
};
Ok(NonNull::slice_from_raw_parts(ptr, size))
self.alloc_impl(layout, true)
}

#[inline]
Expand All @@ -203,26 +242,8 @@ unsafe impl AllocRef for Global {
layout: Layout,
new_size: usize,
) -> Result<NonNull<[u8]>, AllocErr> {
debug_assert!(
new_size >= layout.size(),
"`new_size` must be greater than or equal to `layout.size()`"
);

// SAFETY: `new_size` must be non-zero, which is checked in the match expression.
// If `new_size` is zero, then `old_size` has to be zero as well.
// Other conditions must be upheld by the caller
unsafe {
match layout.size() {
0 => self.alloc(Layout::from_size_align_unchecked(new_size, layout.align())),
old_size => {
// `realloc` probably checks for `new_size >= size` or something similar.
intrinsics::assume(new_size >= old_size);
let raw_ptr = realloc(ptr.as_ptr(), layout, new_size);
let ptr = NonNull::new(raw_ptr).ok_or(AllocErr)?;
Ok(NonNull::slice_from_raw_parts(ptr, new_size))
}
}
}
// SAFETY: all conditions must be upheld by the caller
unsafe { self.grow_impl(ptr, layout, new_size, false) }
}

#[inline]
Expand All @@ -232,27 +253,8 @@ unsafe impl AllocRef for Global {
layout: Layout,
new_size: usize,
) -> Result<NonNull<[u8]>, AllocErr> {
debug_assert!(
new_size >= layout.size(),
"`new_size` must be greater than or equal to `layout.size()`"
);

// SAFETY: `new_size` must be non-zero, which is checked in the match expression.
// If `new_size` is zero, then `old_size` has to be zero as well.
// Other conditions must be upheld by the caller
unsafe {
match layout.size() {
0 => self.alloc_zeroed(Layout::from_size_align_unchecked(new_size, layout.align())),
old_size => {
// `realloc` probably checks for `new_size >= size` or something similar.
intrinsics::assume(new_size >= old_size);
let raw_ptr = realloc(ptr.as_ptr(), layout, new_size);
raw_ptr.add(old_size).write_bytes(0, new_size - old_size);
let ptr = NonNull::new(raw_ptr).ok_or(AllocErr)?;
Ok(NonNull::slice_from_raw_parts(ptr, new_size))
}
}
}
// SAFETY: all conditions must be upheld by the caller
unsafe { self.grow_impl(ptr, layout, new_size, true) }
}

#[inline]
Expand All @@ -262,30 +264,28 @@ unsafe impl AllocRef for Global {
layout: Layout,
new_size: usize,
) -> Result<NonNull<[u8]>, AllocErr> {
let old_size = layout.size();
debug_assert!(
new_size <= old_size,
new_size <= layout.size(),
"`new_size` must be smaller than or equal to `layout.size()`"
);

let ptr = if new_size == 0 {
match new_size {
// SAFETY: conditions must be upheld by the caller
unsafe {
0 => unsafe {
self.dealloc(ptr, layout);
}
layout.dangling()
} else {
// SAFETY: new_size is not zero,
// Other conditions must be upheld by the caller
let raw_ptr = unsafe {
// `realloc` probably checks for `new_size <= old_size` or something similar.
intrinsics::assume(new_size <= old_size);
realloc(ptr.as_ptr(), layout, new_size)
};
NonNull::new(raw_ptr).ok_or(AllocErr)?
};
Ok(NonNull::slice_from_raw_parts(layout.dangling(), 0))
},

Ok(NonNull::slice_from_raw_parts(ptr, new_size))
// SAFETY: `new_size` is non-zero. Other conditions must be upheld by the caller
new_size => unsafe {
// `realloc` probably checks for `new_size <= size` or something similar.
intrinsics::assume(new_size <= layout.size());

let raw_ptr = realloc(ptr.as_ptr(), layout, new_size);
let ptr = NonNull::new(raw_ptr).ok_or(AllocErr)?;
Ok(NonNull::slice_from_raw_parts(ptr, new_size))
},
}
}
}

Expand Down
58 changes: 29 additions & 29 deletions library/core/src/alloc/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,11 @@ pub unsafe trait AllocRef {
/// alignment and a size given by `new_size`. To accomplish this, the allocator may extend the
/// allocation referenced by `ptr` to fit the new layout.
///
/// If this returns `Ok`, then ownership of the memory block referenced by `ptr` has been
/// transferred to this allocator. The memory may or may not have been freed, and should be
/// considered unusable unless it was transferred back to the caller again via the return value
/// of this method.
///
/// If this method returns `Err`, then ownership of the memory block has not been transferred to
/// this allocator, and the contents of the memory block are unaltered.
///
Expand Down Expand Up @@ -192,12 +197,9 @@ pub unsafe trait AllocRef {
"`new_size` must be greater than or equal to `layout.size()`"
);

let new_layout =
// SAFETY: the caller must ensure that the `new_size` does not overflow.
// `layout.align()` comes from a `Layout` and is thus guaranteed to be valid for a Layout.
// The caller must ensure that `new_size` is greater than or equal to zero. If it's equal
// to zero, it's catched beforehand.
Copy link
Member Author

@TimDiekmann TimDiekmann Aug 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

new_size is always greater than or equal to zero, it's unsigned. Also, it was neither checked, if it's zero before, nor it is required, as self.alloc is well defined for zero-sized layouts. Thus I removed the second part.

unsafe { Layout::from_size_align_unchecked(new_size, layout.align()) };
// SAFETY: the caller must ensure that the `new_size` does not overflow.
// `layout.align()` comes from a `Layout` and is thus guaranteed to be valid for a Layout.
let new_layout = unsafe { Layout::from_size_align_unchecked(new_size, layout.align()) };
let new_ptr = self.alloc(new_layout)?;

// SAFETY: because `new_size` must be greater than or equal to `size`, both the old and new
Expand All @@ -206,10 +208,11 @@ pub unsafe trait AllocRef {
// `copy_nonoverlapping` is safe.
// The safety contract for `dealloc` must be upheld by the caller.
unsafe {
ptr::copy_nonoverlapping(ptr.as_ptr(), new_ptr.as_non_null_ptr().as_ptr(), size);
ptr::copy_nonoverlapping(ptr.as_ptr(), new_ptr.as_mut_ptr(), size);
self.dealloc(ptr, layout);
Ok(new_ptr)
}

Ok(new_ptr)
}

/// Behaves like `grow`, but also ensures that the new contents are set to zero before being
Expand All @@ -218,12 +221,12 @@ pub unsafe trait AllocRef {
/// The memory block will contain the following contents after a successful call to
/// `grow_zeroed`:
/// * Bytes `0..layout.size()` are preserved from the original allocation.
/// * Bytes `layout.size()..old_size` will either be preserved or zeroed,
/// depending on the allocator implementation. `old_size` refers to the size of
/// the `MemoryBlock` prior to the `grow_zeroed` call, which may be larger than the size
/// that was originally requested when it was allocated.
/// * Bytes `old_size..new_size` are zeroed. `new_size` refers to
/// the size of the `MemoryBlock` returned by the `grow` call.
/// * Bytes `layout.size()..old_size` will either be preserved or zeroed, depending on the
/// allocator implementation. `old_size` refers to the size of the memory block prior to
/// the `grow_zeroed` call, which may be larger than the size that was originally requested
/// when it was allocated.
/// * Bytes `old_size..new_size` are zeroed. `new_size` refers to the size of the memory
/// block returned by the `grow` call.
///
/// # Safety
///
Expand Down Expand Up @@ -261,12 +264,9 @@ pub unsafe trait AllocRef {
"`new_size` must be greater than or equal to `layout.size()`"
);

let new_layout =
// SAFETY: the caller must ensure that the `new_size` does not overflow.
// `layout.align()` comes from a `Layout` and is thus guaranteed to be valid for a Layout.
// The caller must ensure that `new_size` is greater than or equal to zero. If it's equal
// to zero, it's caught beforehand.
unsafe { Layout::from_size_align_unchecked(new_size, layout.align()) };
// SAFETY: the caller must ensure that the `new_size` does not overflow.
// `layout.align()` comes from a `Layout` and is thus guaranteed to be valid for a Layout.
let new_layout = unsafe { Layout::from_size_align_unchecked(new_size, layout.align()) };
let new_ptr = self.alloc_zeroed(new_layout)?;

// SAFETY: because `new_size` must be greater than or equal to `size`, both the old and new
Expand All @@ -275,10 +275,11 @@ pub unsafe trait AllocRef {
// `copy_nonoverlapping` is safe.
// The safety contract for `dealloc` must be upheld by the caller.
unsafe {
ptr::copy_nonoverlapping(ptr.as_ptr(), new_ptr.as_non_null_ptr().as_ptr(), size);
ptr::copy_nonoverlapping(ptr.as_ptr(), new_ptr.as_mut_ptr(), size);
self.dealloc(ptr, layout);
Ok(new_ptr)
}

Ok(new_ptr)
}

/// Attempts to shrink the memory block.
Expand All @@ -290,8 +291,8 @@ pub unsafe trait AllocRef {
///
/// If this returns `Ok`, then ownership of the memory block referenced by `ptr` has been
/// transferred to this allocator. The memory may or may not have been freed, and should be
/// considered unusable unless it was transferred back to the caller again via the
/// return value of this method.
/// considered unusable unless it was transferred back to the caller again via the return value
/// of this method.
///
/// If this method returns `Err`, then ownership of the memory block has not been transferred to
/// this allocator, and the contents of the memory block are unaltered.
Expand Down Expand Up @@ -332,11 +333,9 @@ pub unsafe trait AllocRef {
"`new_size` must be smaller than or equal to `layout.size()`"
);

let new_layout =
// SAFETY: the caller must ensure that the `new_size` does not overflow.
// `layout.align()` comes from a `Layout` and is thus guaranteed to be valid for a Layout.
// The caller must ensure that `new_size` is greater than zero.
unsafe { Layout::from_size_align_unchecked(new_size, layout.align()) };
let new_layout = unsafe { Layout::from_size_align_unchecked(new_size, layout.align()) };
let new_ptr = self.alloc(new_layout)?;

// SAFETY: because `new_size` must be lower than or equal to `size`, both the old and new
Expand All @@ -345,10 +344,11 @@ pub unsafe trait AllocRef {
// `copy_nonoverlapping` is safe.
// The safety contract for `dealloc` must be upheld by the caller.
unsafe {
ptr::copy_nonoverlapping(ptr.as_ptr(), new_ptr.as_non_null_ptr().as_ptr(), size);
ptr::copy_nonoverlapping(ptr.as_ptr(), new_ptr.as_mut_ptr(), size);
self.dealloc(ptr, layout);
Ok(new_ptr)
}

Ok(new_ptr)
}

/// Creates a "by reference" adaptor for this instance of `AllocRef`.
Expand Down
Loading