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

Do not allocate for ZST ThinBox #123184

Closed
wants to merge 2 commits into from
Closed
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
120 changes: 106 additions & 14 deletions library/alloc/src/boxed/thin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@ use core::error::Error;
use core::fmt::{self, Debug, Display, Formatter};
use core::marker::PhantomData;
#[cfg(not(no_global_oom_handling))]
use core::marker::Unsize;
use core::mem::{self, SizedTypeProperties};
use core::marker::{Freeze, Unsize};
use core::mem;
#[cfg(not(no_global_oom_handling))]
use core::mem::{MaybeUninit, SizedTypeProperties};
use core::ops::{Deref, DerefMut};
use core::ptr::Pointee;
use core::ptr::{self, NonNull};
Expand Down Expand Up @@ -91,6 +93,93 @@ impl<T> ThinBox<T> {

#[unstable(feature = "thin_box", issue = "92791")]
impl<Dyn: ?Sized> ThinBox<Dyn> {
#[cfg(not(no_global_oom_handling))]
fn new_unsize_zst<T>(value: T) -> Self
where
T: Unsize<Dyn>,
{
assert!(mem::size_of::<T>() == 0);

#[repr(C)]
struct ReprC<A, B> {
a: A,
b: B,
}

struct EmptyArray<T> {
_array: [T; 0],
}

// SAFETY: this is a zero-sized type, there can't be any mutable memory here.
// It's a private type so this can never leak to the user either.
// If the user tried to write through the shared reference to this type,
// they would be causing library UB as that's a write outside the memory
// inhabited by their type (which is zero-sized). Therefore making this
// language UB is justified.
unsafe impl<T> Freeze for EmptyArray<T> {}
stepancheg marked this conversation as resolved.
Show resolved Hide resolved

// Allocate header with padding in the beginning:
// ```
// [ padding | header ]
// ```
// where the struct is aligned to both header and value.
#[repr(C)]
struct AlignedHeader<H: Copy, T> {
header_data: MaybeUninit<ReprC<H, EmptyArray<T>>>,
}

impl<H: Copy + Freeze, T> AlignedHeader<H, T> {
const fn make(header: H) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't sound, MaybeUninit does not preserve padding bytes when moved (see #99604), so we might loose these writes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm out of ideas how to deal with this issue.

let mut header_data = MaybeUninit::<ReprC<H, EmptyArray<T>>>::zeroed();
unsafe {
header_data.as_mut_ptr().add(1).cast::<H>().sub(1).write(header);
}
AlignedHeader { header_data }
}
}

#[repr(C)]
struct DynZstAlloc<T, Dyn: ?Sized> {
header: AlignedHeader<<Dyn as Pointee>::Metadata, T>,
value: EmptyArray<T>,
}

impl<T, Dyn: ?Sized> DynZstAlloc<T, Dyn>
where
T: Unsize<Dyn>,
{
const ALLOCATION: DynZstAlloc<T, Dyn> = DynZstAlloc {
header: AlignedHeader::make(ptr::metadata::<Dyn>(
ptr::dangling::<T>() as *const Dyn
)),
value: EmptyArray { _array: [] },
};

fn static_alloc<'a>() -> &'a DynZstAlloc<T, Dyn> {
&Self::ALLOCATION
}
}

let alloc: &DynZstAlloc<T, Dyn> = DynZstAlloc::<T, Dyn>::static_alloc();

let value_offset = mem::offset_of!(DynZstAlloc<T, Dyn>, value);
assert_eq!(value_offset, mem::size_of::<AlignedHeader<<Dyn as Pointee>::Metadata, T>>());

let ptr = WithOpaqueHeader(
NonNull::new(
// SAFETY: there's no overflow here because we add field offset.
unsafe {
(alloc as *const DynZstAlloc<T, Dyn> as *mut u8).add(value_offset) as *mut _
},
)
.unwrap(),
);
let thin_box = ThinBox::<Dyn> { ptr, _marker: PhantomData };
// Forget the value to avoid double drop.
mem::forget(value);
thin_box
}

/// Moves a type to the heap with its [`Metadata`] stored in the heap allocation instead of on
Copy link
Contributor

Choose a reason for hiding this comment

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

Please mirror the check to try_new, otherwise we get memory leaks. Since it's infallible, new_unsize_zst can be used for both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, didn't get it.

There's no try_new_unsize, and try_new handles ZST.

/// the stack.
///
Expand All @@ -109,9 +198,13 @@ impl<Dyn: ?Sized> ThinBox<Dyn> {
where
T: Unsize<Dyn>,
{
let meta = ptr::metadata(&value as &Dyn);
let ptr = WithOpaqueHeader::new(meta, value);
ThinBox { ptr, _marker: PhantomData }
if mem::size_of::<T>() == 0 {
Self::new_unsize_zst(value)
} else {
let meta = ptr::metadata(&value as &Dyn);
let ptr = WithOpaqueHeader::new(meta, value);
ThinBox { ptr, _marker: PhantomData }
}
}
}

Expand Down Expand Up @@ -300,20 +393,19 @@ impl<H> WithHeader<H> {

impl<H> Drop for DropGuard<H> {
fn drop(&mut self) {
// All ZST are allocated statically.
if self.value_layout.size() == 0 {
return;
}

unsafe {
// SAFETY: Layout must have been computable if we're in drop
let (layout, value_offset) =
WithHeader::<H>::alloc_layout(self.value_layout).unwrap_unchecked();

// Note: Don't deallocate if the layout size is zero, because the pointer
// didn't come from the allocator.
if layout.size() != 0 {
alloc::dealloc(self.ptr.as_ptr().sub(value_offset), layout);
} else {
debug_assert!(
value_offset == 0 && H::IS_ZST && self.value_layout.size() == 0
);
}
// Since we only allocate for non-ZSTs, the layout size cannot be zero.
debug_assert!(layout.size() != 0);
alloc::dealloc(self.ptr.as_ptr().sub(value_offset), layout);
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions library/alloc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,8 @@
#![feature(extend_one)]
#![feature(fmt_internals)]
#![feature(fn_traits)]
#![feature(freeze)]
#![feature(freeze_impls)]
Copy link
Member

@RalfJung RalfJung Mar 29, 2024

Choose a reason for hiding this comment

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

This feature was never meant to be used outside libcore :/ I'm beginning to regret that we made this trait public again...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if instead of this custom Freeze thing we implement a new builtin type like AlignAs<T>, which will have all properties like Copy, Sync and Freeze, but without inheriting properties of T except alignment?

That would solve this issue with Freeze (but won't solve the padding issue from the thread above).

Copy link
Member

Choose a reason for hiding this comment

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

That's a good idea. We don't even necessarily have to do this now, we could say that's our backup plan if Freeze ever becomes un-implementable.

I still haven't understood the padding issue.

#![feature(generic_nonzero)]
#![feature(hasher_prefixfree_extras)]
#![feature(hint_assert_unchecked)]
Expand Down
3 changes: 2 additions & 1 deletion library/core/src/ptr/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

use crate::fmt;
use crate::hash::{Hash, Hasher};
use crate::marker::Freeze;

/// Provides the pointer metadata type of any pointed-to type.
///
Expand Down Expand Up @@ -57,7 +58,7 @@ pub trait Pointee {
// NOTE: Keep trait bounds in `static_assert_expected_bounds_for_metadata`
// in `library/core/src/ptr/metadata.rs`
// in sync with those here:
type Metadata: Copy + Send + Sync + Ord + Hash + Unpin;
type Metadata: Copy + Send + Sync + Ord + Hash + Unpin + Freeze;
}

/// Pointers to types implementing this trait alias are “thin”.
Expand Down
1 change: 1 addition & 0 deletions library/core/tests/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#![feature(duration_constructors)]
#![feature(exact_size_is_empty)]
#![feature(extern_types)]
#![feature(freeze)]
#![feature(flt2dec)]
#![feature(fmt_internals)]
#![feature(float_minimum_maximum)]
Expand Down
3 changes: 2 additions & 1 deletion library/core/tests/ptr.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use core::cell::RefCell;
use core::marker::Freeze;
use core::mem::{self, MaybeUninit};
use core::num::NonZero;
use core::ptr;
Expand Down Expand Up @@ -841,7 +842,7 @@ fn ptr_metadata_bounds() {
fn static_assert_expected_bounds_for_metadata<Meta>()
where
// Keep this in sync with the associated type in `library/core/src/ptr/metadata.rs`
Meta: Copy + Send + Sync + Ord + std::hash::Hash + Unpin,
Meta: Copy + Send + Sync + Ord + std::hash::Hash + Unpin + Freeze,
{
}
}
Expand Down
Loading