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

Simplify manual ptr arithmetic in slice::Iter with ptr_sub #106393

Merged
merged 1 commit into from
Jan 15, 2023
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
8 changes: 1 addition & 7 deletions library/core/src/slice/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ mod macros;
use crate::cmp;
use crate::cmp::Ordering;
use crate::fmt;
use crate::intrinsics::{assume, exact_div, unchecked_sub};
use crate::intrinsics::assume;
use crate::iter::{FusedIterator, TrustedLen, TrustedRandomAccess, TrustedRandomAccessNoCoerce};
use crate::marker::{PhantomData, Send, Sized, Sync};
use crate::mem::{self, SizedTypeProperties};
Expand Down Expand Up @@ -35,12 +35,6 @@ impl<'a, T> IntoIterator for &'a mut [T] {
}
}

// Macro helper functions
#[inline(always)]
fn size_from_ptr<T>(_: *const T) -> usize {
mem::size_of::<T>()
}

/// Immutable slice iterator
///
/// This struct is created by the [`iter`] method on [slices].
Expand Down
22 changes: 6 additions & 16 deletions library/core/src/slice/iter/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,30 +9,20 @@ macro_rules! is_empty {
};
}

// To get rid of some bounds checks (see `position`), we compute the length in a somewhat
// unexpected way. (Tested by `codegen/slice-position-bounds-check`.)
macro_rules! len {
($self: ident) => {{
#![allow(unused_unsafe)] // we're sometimes used within an unsafe block

let start = $self.ptr;
let size = size_from_ptr(start.as_ptr());
if size == 0 {
// This _cannot_ use `unchecked_sub` because we depend on wrapping
if T::IS_ZST {
Copy link
Member

Choose a reason for hiding this comment

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

This is odd, where is T bound? Does this macro assume that there is a T in scope wherever it is used (violating hygiene)? That at least warrants a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

The macro is only used in another macro (in the same file) which defines T

// This _cannot_ use `ptr_sub` because we depend on wrapping
// to represent the length of long ZST slice iterators.
$self.end.addr().wrapping_sub(start.as_ptr().addr())
} else {
// We know that `start <= end`, so can do better than `offset_from`,
// which needs to deal in signed. By setting appropriate flags here
// we can tell LLVM this, which helps it remove bounds checks.
// SAFETY: By the type invariant, `start <= end`
let diff = unsafe { unchecked_sub($self.end.addr(), start.as_ptr().addr()) };
// By also telling LLVM that the pointers are apart by an exact
// multiple of the type size, it can optimize `len() == 0` down to
// `start == end` instead of `(end - start) < size`.
// SAFETY: By the type invariant, the pointers are aligned so the
// distance between them must be a multiple of pointee size
unsafe { exact_div(diff, size) }
// To get rid of some bounds checks (see `position`), we use ptr_sub instead of
// offset_from (Tested by `codegen/slice-position-bounds-check`.)
// SAFETY: by the type invariant pointers are aligned and `start <= end`
unsafe { $self.end.sub_ptr(start.as_ptr()) }
Copy link
Member

@RalfJung RalfJung Jan 16, 2023

Choose a reason for hiding this comment

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

This safety comment is incomplete. sub_ptr (via the offset_from safety requirements) requires the pointers to point to the same allocation, and they must be dereferenceable, i.e., point to allocated memory. The latter is not currently always the case, though whether that is a bug in this function or a bug in whatever creates those dangling raw pointers is unclear.

See Zulip

}
}};
}
Expand Down
1 change: 1 addition & 0 deletions tests/codegen/issue-45964-bounds-check-slice-pos.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// prevent optimizing away bounds checks

// compile-flags: -O
// ignore-debug: the debug assertions get in the way

#![crate_type="rlib"]

Expand Down