From 80526016581efa8e605e9a34cd7d0061d8ec19bf Mon Sep 17 00:00:00 2001 From: Kevin Reid Date: Tue, 4 Nov 2025 07:38:07 -0800 Subject: [PATCH 1/2] Explicitly forget the zero remaining elements in `vec::IntoIter::fold()`. This seems to help LLVM notice that dropping the elements in the destructor is not necessary. --- library/alloc/src/vec/into_iter.rs | 6 +++++ tests/codegen-llvm/vec-into-iter-drops.rs | 28 +++++++++++++++++++++++ 2 files changed, 34 insertions(+) create mode 100644 tests/codegen-llvm/vec-into-iter-drops.rs diff --git a/library/alloc/src/vec/into_iter.rs b/library/alloc/src/vec/into_iter.rs index 358bdeacae790..e23f3c2909279 100644 --- a/library/alloc/src/vec/into_iter.rs +++ b/library/alloc/src/vec/into_iter.rs @@ -329,6 +329,12 @@ impl Iterator for IntoIter { accum = f(accum, tmp); } } + + // There are in fact no remaining elements to forget, but by doing this we can avoid + // potentially generating a needless loop to drop the elements that cannot exist at + // this point. + self.forget_remaining_elements(); + accum } diff --git a/tests/codegen-llvm/vec-into-iter-drops.rs b/tests/codegen-llvm/vec-into-iter-drops.rs new file mode 100644 index 0000000000000..1d6959e0a8a08 --- /dev/null +++ b/tests/codegen-llvm/vec-into-iter-drops.rs @@ -0,0 +1,28 @@ +//@ compile-flags: -Copt-level=3 +// Test that we can avoid generating an element dropping loop when `vec::IntoIter` is consumed. +#![crate_type = "lib"] + +use std::vec; + +struct Bomb; +impl Drop for Bomb { + #[inline] + fn drop(&mut self) { + panic!("dropped") + } +} + +/// This test case from https://users.rust-lang.org/t/unnecessary-drop-in-place-emitted-for-a-fully-consumed-intoiter/135119 +/// It should not emit any `drop::()` because every element is forgotten. +// CHECK-LABEL: @vec_into_iter_drop_option +#[no_mangle] +pub fn vec_into_iter_drop_option(v: vec::Vec<(usize, Option)>) -> usize { + // CHECK-NOT: panic + // CHECK-NOT: Bomb$u20$as$u20$core..ops..drop..Drop + let mut last = 0; + v.into_iter().for_each(|(x, bomb)| { + last = x; + std::mem::forget(bomb); + }); + last +} From 5fa286ddc6f88b9f309ea9e229a0e49071fd282b Mon Sep 17 00:00:00 2001 From: Kevin Reid Date: Tue, 4 Nov 2025 13:49:48 -0800 Subject: [PATCH 2/2] Explicitly deallocate instead of dropping the `IntoIter`. This has the same end goal as the previous commit but does not rely on compiler optimizations to delete the unwanted code; instead it enters a code path which explicitly frees the allocation and forgets the `IntoIter`. --- .../src/collections/vec_deque/spec_extend.rs | 4 +- library/alloc/src/vec/into_iter.rs | 46 ++++++++++++++++-- tests/codegen-llvm/vec-into-iter-drops.rs | 47 +++++++++++++++++-- 3 files changed, 86 insertions(+), 11 deletions(-) diff --git a/library/alloc/src/collections/vec_deque/spec_extend.rs b/library/alloc/src/collections/vec_deque/spec_extend.rs index 6c2199135e08a..407a6cc4b7e92 100644 --- a/library/alloc/src/collections/vec_deque/spec_extend.rs +++ b/library/alloc/src/collections/vec_deque/spec_extend.rs @@ -78,7 +78,7 @@ where #[cfg(not(test))] impl SpecExtend> for VecDeque { - fn spec_extend(&mut self, mut iterator: vec::IntoIter) { + fn spec_extend(&mut self, iterator: vec::IntoIter) { let slice = iterator.as_slice(); self.reserve(slice.len()); @@ -86,7 +86,7 @@ impl SpecExtend> for VecDeque { self.copy_slice(self.to_physical_idx(self.len), slice); self.len += slice.len(); } - iterator.forget_remaining_elements(); + iterator.forget_remaining_elements_and_dealloc(); } } diff --git a/library/alloc/src/vec/into_iter.rs b/library/alloc/src/vec/into_iter.rs index e23f3c2909279..83ab2d54bde9e 100644 --- a/library/alloc/src/vec/into_iter.rs +++ b/library/alloc/src/vec/into_iter.rs @@ -160,12 +160,51 @@ impl IntoIter { } /// Forgets to Drop the remaining elements while still allowing the backing allocation to be freed. + /// + /// This method does not consume `self`, and leaves deallocation to `impl Drop for IntoIter`. + /// If consuming `self` is possible, consider calling + /// [`Self::forget_remaining_elements_and_dealloc()`] instead. pub(crate) fn forget_remaining_elements(&mut self) { // For the ZST case, it is crucial that we mutate `end` here, not `ptr`. // `ptr` must stay aligned, while `end` may be unaligned. self.end = self.ptr.as_ptr(); } + /// Forgets to Drop the remaining elements and frees the backing allocation. + /// Consuming version of [`Self::forget_remaining_elements()`]. + /// + /// This can be used in place of `drop(self)` when `self` is known to be exhausted, + /// to avoid producing a needless `drop_in_place::<[T]>()`. + #[inline] + pub(crate) fn forget_remaining_elements_and_dealloc(self) { + let mut this = ManuallyDrop::new(self); + // SAFETY: `this` is in ManuallyDrop, so it will not be double-freed. + unsafe { + this.dealloc_only(); + } + } + + /// Frees the allocation, without checking or dropping anything else. + /// + /// The safe version of this method is [`Self::forget_remaining_elements_and_dealloc()`]. + /// This function exists only to share code between that method and the `impl Drop`. + /// + /// # Safety + /// + /// This function must only be called with an [`IntoIter`] that is not going to be dropped + /// or otherwise used in any way, either because it is being forgotten or because its `Drop` + /// is already executing; otherwise a double-free will occur, and possibly a read from freed + /// memory if there are any remaining elements. + #[inline] + unsafe fn dealloc_only(&mut self) { + unsafe { + // SAFETY: our caller promises not to touch `*self` again + let alloc = ManuallyDrop::take(&mut self.alloc); + // RawVec handles deallocation + let _ = RawVec::from_nonnull_in(self.buf, self.cap, alloc); + } + } + #[cfg(not(no_global_oom_handling))] #[inline] pub(crate) fn into_vecdeque(self) -> VecDeque { @@ -333,7 +372,7 @@ impl Iterator for IntoIter { // There are in fact no remaining elements to forget, but by doing this we can avoid // potentially generating a needless loop to drop the elements that cannot exist at // this point. - self.forget_remaining_elements(); + self.forget_remaining_elements_and_dealloc(); accum } @@ -502,10 +541,7 @@ unsafe impl<#[may_dangle] T, A: Allocator> Drop for IntoIter { impl Drop for DropGuard<'_, T, A> { fn drop(&mut self) { unsafe { - // `IntoIter::alloc` is not used anymore after this and will be dropped by RawVec - let alloc = ManuallyDrop::take(&mut self.0.alloc); - // RawVec handles deallocation - let _ = RawVec::from_nonnull_in(self.0.buf, self.0.cap, alloc); + self.0.dealloc_only(); } } } diff --git a/tests/codegen-llvm/vec-into-iter-drops.rs b/tests/codegen-llvm/vec-into-iter-drops.rs index 1d6959e0a8a08..eabda7a356204 100644 --- a/tests/codegen-llvm/vec-into-iter-drops.rs +++ b/tests/codegen-llvm/vec-into-iter-drops.rs @@ -12,12 +12,16 @@ impl Drop for Bomb { } } -/// This test case from https://users.rust-lang.org/t/unnecessary-drop-in-place-emitted-for-a-fully-consumed-intoiter/135119 -/// It should not emit any `drop::()` because every element is forgotten. -// CHECK-LABEL: @vec_into_iter_drop_option +/// Test case originally from https://users.rust-lang.org/t/unnecessary-drop-in-place-emitted-for-a-fully-consumed-intoiter/135119 +/// +/// What we are looking for is that there should be no calls to `impl Drop for Bomb` +/// because every element is unconditionally forgotten. +// +// CHECK-LABEL: @vec_for_each_doesnt_drop #[no_mangle] -pub fn vec_into_iter_drop_option(v: vec::Vec<(usize, Option)>) -> usize { +pub fn vec_for_each_doesnt_drop(v: vec::Vec<(usize, Option)>) -> usize { // CHECK-NOT: panic + // CHECK-NOT: {{call.*drop_in_place}} // CHECK-NOT: Bomb$u20$as$u20$core..ops..drop..Drop let mut last = 0; v.into_iter().for_each(|(x, bomb)| { @@ -26,3 +30,38 @@ pub fn vec_into_iter_drop_option(v: vec::Vec<(usize, Option)>) -> usize { }); last } + +/// Test that does *not* get the above optimization we are expecting: +/// it uses a normal `for` loop which calls `Iterator::next()` and then drops the iterator, +/// and dropping the iterator drops remaining items. +/// +/// This test exists to prove that the above CHECK-NOT is looking for the right things. +/// However, it might start failing if LLVM figures out that there are no remaining items. +// +// CHECK-LABEL: @vec_for_loop +#[no_mangle] +pub fn vec_for_loop(v: vec::Vec<(usize, Option)>) -> usize { + // CHECK: {{call.*drop_in_place}} + let mut last = 0; + for (x, bomb) in v { + last = x; + std::mem::forget(bomb); + } + last +} + +/// Test where there still should be drops because there are no forgets. +/// +/// This test exists to prove that the above CHECK-NOT is looking for the right things +/// and does not say anything interesting about codegen itself. +// +// CHECK-LABEL: @vec_for_each_does_drop +#[no_mangle] +pub fn vec_for_each_does_drop(v: vec::Vec<(usize, Option)>) -> usize { + // CHECK: begin_panic + let mut last = 0; + v.into_iter().for_each(|(x, bomb)| { + last = x; + }); + last +}