From e6396bca013b1b4cc1a177ef11e8fd5c270a4ea5 Mon Sep 17 00:00:00 2001 From: Zachary S Date: Sat, 11 May 2024 09:54:32 -0500 Subject: [PATCH 1/5] Use a single static for all default slice Arcs. Also adds debug_asserts in Drop for Weak/Arc that the shared static is not being "dropped"/"deallocated". --- library/alloc/src/sync.rs | 77 ++++++++++++++++++++++++--------------- 1 file changed, 48 insertions(+), 29 deletions(-) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index a925b544bc2c9..9081ea5c679b7 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -2468,6 +2468,15 @@ unsafe impl<#[may_dangle] T: ?Sized, A: Allocator> Drop for Arc { // [2]: (https://github.com/rust-lang/rust/pull/41714) acquire!(self.inner().strong); + // Make sure we aren't trying to "drop" the shared static for empty slices + // used by Default::default. + debug_assert!( + !ptr::addr_eq(self.ptr.as_ptr(), &STATIC_INNER_SLICE.inner), + "Arcs backed by a static should never be reach a strong count of 0. \ + Likely decrement_strong_count or from_raw were called too many times.", + ); + + unsafe { self.drop_slow(); } @@ -3059,6 +3068,15 @@ unsafe impl<#[may_dangle] T: ?Sized, A: Allocator> Drop for Weak { if inner.weak.fetch_sub(1, Release) == 1 { acquire!(inner.weak); + + // Make sure we aren't trying to "deallocate" the shared static for empty slices + // used by Default::default. + debug_assert!( + !ptr::addr_eq(self.ptr.as_ptr(), &STATIC_INNER_SLICE.inner), + "Arc/Weaks backed by a static should never be deallocated. \ + Likely decrement_strong_count or from_raw were called too many times.", + ); + unsafe { self.alloc.deallocate(self.ptr.cast(), Layout::for_value_raw(self.ptr.as_ptr())) } @@ -3300,6 +3318,28 @@ impl Default for Arc { } } +/// Struct to hold the static `ArcInner` used for empty `Arc` as +/// returned by `Default::default`. +/// +/// Layout notes: +/// * `repr(align(16))` so we can use it for `[T]` with `align_of::() <= 16`. +/// * `repr(C)` so `inner` is at offset 0 (and thus guaranteed to actually be aligned to 16). +/// * `[u8; 1]` (to be initialized with 0) so it can be used for `Arc`. +#[repr(C, align(16))] +struct SliceArcInnerForStatic { + inner: ArcInner<[u8; 1]>, +} +const MAX_STATIC_INNER_SLICE_ALIGNMENT: usize = 16; + +static STATIC_INNER_SLICE: SliceArcInnerForStatic = SliceArcInnerForStatic { + inner: ArcInner { + strong: atomic::AtomicUsize::new(1), + weak: atomic::AtomicUsize::new(1), + data: [0], + }, +}; + + #[cfg(not(no_global_oom_handling))] #[stable(feature = "more_rc_default_impls", since = "CURRENT_RUSTC_VERSION")] impl Default for Arc { @@ -3324,12 +3364,7 @@ impl Default for Arc { #[inline] fn default() -> Self { use core::ffi::CStr; - static STATIC_INNER_CSTR: ArcInner<[u8; 1]> = ArcInner { - strong: atomic::AtomicUsize::new(1), - weak: atomic::AtomicUsize::new(1), - data: [0], - }; - let inner: NonNull> = NonNull::from(&STATIC_INNER_CSTR); + let inner: NonNull> = NonNull::from(&STATIC_INNER_SLICE.inner); let inner: NonNull> = NonNull::new(inner.as_ptr() as *mut ArcInner).unwrap(); // `this` semantically is the Arc "owned" by the static, so make sure not to drop it. let this: mem::ManuallyDrop> = unsafe { mem::ManuallyDrop::new(Arc::from_inner(inner)) }; @@ -3345,31 +3380,15 @@ impl Default for Arc<[T]> { /// This may or may not share an allocation with other Arcs. #[inline] fn default() -> Self { - let alignment_of_t: usize = mem::align_of::(); - // We only make statics for the lowest five alignments. - // Alignments greater than that will use dynamic allocation. - macro_rules! use_static_inner_for_alignments { - ($($alignment:literal),*) => { - $(if alignment_of_t == $alignment { - // Note: this must be in a new scope because static and type names are unhygenic. - #[repr(align($alignment))] - struct Aligned; - static ALIGNED_STATIC_INNER: ArcInner = ArcInner { - strong: atomic::AtomicUsize::new(1), - weak: atomic::AtomicUsize::new(1), - data: Aligned, - }; - let inner: NonNull> = NonNull::from(&ALIGNED_STATIC_INNER); - let inner: NonNull> = inner.cast(); - // `this` semantically is the Arc "owned" by the static, so make sure not to drop it. - let this: mem::ManuallyDrop> = unsafe { mem::ManuallyDrop::new(Arc::from_inner(inner)) }; - return (*this).clone(); - })* - }; + if mem::align_of::() <= MAX_STATIC_INNER_SLICE_ALIGNMENT { + let inner: NonNull> = NonNull::from(&STATIC_INNER_SLICE.inner); + let inner: NonNull> = inner.cast(); + // `this` semantically is the Arc "owned" by the static, so make sure not to drop it. + let this: mem::ManuallyDrop> = unsafe { mem::ManuallyDrop::new(Arc::from_inner(inner)) }; + return (*this).clone(); } - use_static_inner_for_alignments!(1, 2, 4, 8, 16); - // If T's alignment is not one of the ones we have a static for, make a new unique allocation. + // If T's alignment is too large for the static, make a new unique allocation. let arr: [T; 0] = []; Arc::from(arr) } From 2dacd70e1e65cb078e7459295f99c457dc836bac Mon Sep 17 00:00:00 2001 From: Zachary S Date: Sun, 19 May 2024 11:42:35 -0500 Subject: [PATCH 2/5] Fix stacked borrows violation --- library/alloc/src/sync.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index 9081ea5c679b7..89dbf9f5cdd4f 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -3381,7 +3381,11 @@ impl Default for Arc<[T]> { #[inline] fn default() -> Self { if mem::align_of::() <= MAX_STATIC_INNER_SLICE_ALIGNMENT { - let inner: NonNull> = NonNull::from(&STATIC_INNER_SLICE.inner); + // We take a reference to the whole struct instead of the ArcInner<[u8; 1]> inside it so + // we don't shrink the range of bytes the ptr is allowed to access under Stacked Borrows. + // (Miri complains on 32-bit targets with Arc<[Align16]> otherwise.) + // (Note that NonNull::from(&STATIC_INNER_SLICE.inner) is fine under Tree Borrows.) + let inner: NonNull = NonNull::from(&STATIC_INNER_SLICE); let inner: NonNull> = inner.cast(); // `this` semantically is the Arc "owned" by the static, so make sure not to drop it. let this: mem::ManuallyDrop> = unsafe { mem::ManuallyDrop::new(Arc::from_inner(inner)) }; From 6fae171e54adb0985a6ee131d4b89012856e7cd9 Mon Sep 17 00:00:00 2001 From: Zachary S Date: Sun, 19 May 2024 13:21:53 -0500 Subject: [PATCH 3/5] fmt --- library/alloc/src/sync.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index 89dbf9f5cdd4f..62c622890b5a7 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -2476,7 +2476,6 @@ unsafe impl<#[may_dangle] T: ?Sized, A: Allocator> Drop for Arc { Likely decrement_strong_count or from_raw were called too many times.", ); - unsafe { self.drop_slow(); } @@ -3339,7 +3338,6 @@ static STATIC_INNER_SLICE: SliceArcInnerForStatic = SliceArcInnerForStatic { }, }; - #[cfg(not(no_global_oom_handling))] #[stable(feature = "more_rc_default_impls", since = "CURRENT_RUSTC_VERSION")] impl Default for Arc { @@ -3365,9 +3363,11 @@ impl Default for Arc { fn default() -> Self { use core::ffi::CStr; let inner: NonNull> = NonNull::from(&STATIC_INNER_SLICE.inner); - let inner: NonNull> = NonNull::new(inner.as_ptr() as *mut ArcInner).unwrap(); + let inner: NonNull> = + NonNull::new(inner.as_ptr() as *mut ArcInner).unwrap(); // `this` semantically is the Arc "owned" by the static, so make sure not to drop it. - let this: mem::ManuallyDrop> = unsafe { mem::ManuallyDrop::new(Arc::from_inner(inner)) }; + let this: mem::ManuallyDrop> = + unsafe { mem::ManuallyDrop::new(Arc::from_inner(inner)) }; (*this).clone() } } @@ -3388,7 +3388,8 @@ impl Default for Arc<[T]> { let inner: NonNull = NonNull::from(&STATIC_INNER_SLICE); let inner: NonNull> = inner.cast(); // `this` semantically is the Arc "owned" by the static, so make sure not to drop it. - let this: mem::ManuallyDrop> = unsafe { mem::ManuallyDrop::new(Arc::from_inner(inner)) }; + let this: mem::ManuallyDrop> = + unsafe { mem::ManuallyDrop::new(Arc::from_inner(inner)) }; return (*this).clone(); } From 58f8ed122ab649c87c6d4c5585d9c8476d87d338 Mon Sep 17 00:00:00 2001 From: Zachary S Date: Sun, 19 May 2024 13:22:34 -0500 Subject: [PATCH 4/5] cfg-out unused code under no_global_oom_handling --- library/alloc/src/sync.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index 62c622890b5a7..2b338a1d6475d 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -3328,6 +3328,7 @@ impl Default for Arc { struct SliceArcInnerForStatic { inner: ArcInner<[u8; 1]>, } +#[cfg(not(no_global_oom_handling))] const MAX_STATIC_INNER_SLICE_ALIGNMENT: usize = 16; static STATIC_INNER_SLICE: SliceArcInnerForStatic = SliceArcInnerForStatic { From 3299823d62024d2f665f762e33cf1bced1922521 Mon Sep 17 00:00:00 2001 From: Zachary S Date: Sun, 19 May 2024 13:29:45 -0500 Subject: [PATCH 5/5] Fix typo in assert message --- library/alloc/src/sync.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/alloc/src/sync.rs b/library/alloc/src/sync.rs index 2b338a1d6475d..8a374dd30f850 100644 --- a/library/alloc/src/sync.rs +++ b/library/alloc/src/sync.rs @@ -2472,7 +2472,7 @@ unsafe impl<#[may_dangle] T: ?Sized, A: Allocator> Drop for Arc { // used by Default::default. debug_assert!( !ptr::addr_eq(self.ptr.as_ptr(), &STATIC_INNER_SLICE.inner), - "Arcs backed by a static should never be reach a strong count of 0. \ + "Arcs backed by a static should never reach a strong count of 0. \ Likely decrement_strong_count or from_raw were called too many times.", );