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

Don't use mem::zeroed in vec::IntoIter #120952

Merged
merged 1 commit into from
Feb 17, 2024
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
63 changes: 29 additions & 34 deletions library/alloc/src/vec/into_iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use core::iter::{
TrustedRandomAccessNoCoerce,
};
use core::marker::PhantomData;
use core::mem::{self, ManuallyDrop, MaybeUninit, SizedTypeProperties};
use core::mem::{ManuallyDrop, MaybeUninit, SizedTypeProperties};
use core::num::NonZero;
#[cfg(not(no_global_oom_handling))]
use core::ops::Deref;
Expand Down Expand Up @@ -200,27 +200,23 @@ impl<T, A: Allocator> Iterator for IntoIter<T, A> {

#[inline]
fn next(&mut self) -> Option<T> {
if T::IS_ZST {
if self.ptr.as_ptr() == self.end as *mut _ {
None
} else {
// `ptr` has to stay where it is to remain aligned, so we reduce the length by 1 by
// reducing the `end`.
self.end = self.end.wrapping_byte_sub(1);

// Make up a value of this ZST.
Some(unsafe { mem::zeroed() })
let ptr = if T::IS_ZST {
if self.ptr.as_ptr() == self.end as *mut T {
return None;
}
// `ptr` has to stay where it is to remain aligned, so we reduce the length by 1 by
// reducing the `end`.
self.end = self.end.wrapping_byte_sub(1);
self.ptr
} else {
if self.ptr == non_null!(self.end, T) {
None
} else {
let old = self.ptr;
self.ptr = unsafe { old.add(1) };

Some(unsafe { ptr::read(old.as_ptr()) })
return None;
}
}
let old = self.ptr;
self.ptr = unsafe { old.add(1) };
old
};
Some(unsafe { ptr.read() })
}

#[inline]
Expand Down Expand Up @@ -305,7 +301,7 @@ impl<T, A: Allocator> Iterator for IntoIter<T, A> {
// Also note the implementation of `Self: TrustedRandomAccess` requires
// that `T: Copy` so reading elements from the buffer doesn't invalidate
// them for `Drop`.
unsafe { if T::IS_ZST { mem::zeroed() } else { self.ptr.add(i).read() } }
unsafe { self.ptr.add(i).read() }
}
}

Expand All @@ -314,23 +310,22 @@ impl<T, A: Allocator> DoubleEndedIterator for IntoIter<T, A> {
#[inline]
fn next_back(&mut self) -> Option<T> {
if T::IS_ZST {
if self.end as *mut _ == self.ptr.as_ptr() {
None
} else {
// See above for why 'ptr.offset' isn't used
self.end = self.end.wrapping_byte_sub(1);

// Make up a value of this ZST.
Some(unsafe { mem::zeroed() })
if self.ptr.as_ptr() == self.end as *mut _ {
return None;
}
// See above for why 'ptr.offset' isn't used
self.end = self.end.wrapping_byte_sub(1);
// Note that even though this is next_back() we're reading from `self.ptr`, not
// `self.end`. We track our length using the byte offset from `self.ptr` to `self.end`,
// so the end pointer may not be suitably aligned for T.
Some(unsafe { ptr::read(self.ptr.as_ptr()) })
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
} else {
if non_null!(self.end, T) == self.ptr {
None
} else {
let new_end = unsafe { non_null!(self.end, T).sub(1) };
*non_null!(mut self.end, T) = new_end;

Some(unsafe { ptr::read(new_end.as_ptr()) })
Comment on lines -330 to -333
Copy link
Member

Choose a reason for hiding this comment

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

next_back isn't covered by codegen/vec-iter. can you check if it still has the nonnull attribute?

Copy link
Member Author

Choose a reason for hiding this comment

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

While working on this, I think I realized that the transmute strategy I was trying to adopt was working by sheer luck. I thought I was getting !nonnull from the transmute range assumes, but we don't insert those for pointers. I'm looking into that, but not in this PR.

if self.ptr == non_null!(self.end, T) {
return None;
}
unsafe {
self.end = self.end.sub(1);
Some(ptr::read(self.end))
}
}
}
Expand Down
17 changes: 15 additions & 2 deletions tests/codegen/vec-iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ pub fn vec_iter_is_empty_nonnull(it: &vec::IntoIter<u8>) -> bool {
it.is_empty()
}

// CHECK-LABEL: @vec_iter_next
// CHECK-LABEL: @vec_iter_next_nonnull
#[no_mangle]
pub fn vec_iter_next(it: &mut vec::IntoIter<u8>) -> Option<u8> {
pub fn vec_iter_next_nonnull(it: &mut vec::IntoIter<u8>) -> Option<u8> {
// CHECK: load ptr
// CHECK-SAME: !nonnull
// CHECK-SAME: !noundef
Expand All @@ -44,3 +44,16 @@ pub fn vec_iter_next(it: &mut vec::IntoIter<u8>) -> Option<u8> {
// CHECK: ret
it.next()
}

// CHECK-LABEL: @vec_iter_next_back_nonnull
#[no_mangle]
pub fn vec_iter_next_back_nonnull(it: &mut vec::IntoIter<u8>) -> Option<u8> {
// CHECK: load ptr
// CHECK-SAME: !nonnull
// CHECK-SAME: !noundef
// CHECK: load ptr
// CHECK-SAME: !nonnull
// CHECK-SAME: !noundef
// CHECK: ret
it.next_back()
}
Loading