Skip to content

Commit

Permalink
Rollup merge of #122461 - the8472:fix-step-forward-unchecked, r=Amanieu
Browse files Browse the repository at this point in the history
fix unsoundness in Step::forward_unchecked for signed integers

Fixes #122420

```rust
pub fn foo(a: i8, b: u8) -> i8 {
    unsafe { a.checked_add_unsigned(b).unwrap_unchecked() }
}
```

still compiles down to a single arithmetic instruction ([godbolt](https://rust.godbolt.org/z/qsd3xYWfE)).

But we may be losing some loop optimizations if llvm can no longer easily derive that it's a finite counted loop from the no-wrapping flags.
  • Loading branch information
matthiaskrgr authored Mar 14, 2024
2 parents 95e3d1b + d3cab9f commit 54328c7
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 3 deletions.
30 changes: 28 additions & 2 deletions library/core/src/iter/range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,8 +184,25 @@ pub trait Step: Clone + PartialOrd + Sized {
}
}

// These are still macro-generated because the integer literals resolve to different types.
macro_rules! step_identical_methods {
// Separate impls for signed ranges because the distance within a signed range can be larger
// than the signed::MAX value. Therefore `as` casting to the signed type would be incorrect.
macro_rules! step_signed_methods {
($unsigned: ty) => {
#[inline]
unsafe fn forward_unchecked(start: Self, n: usize) -> Self {
// SAFETY: the caller has to guarantee that `start + n` doesn't overflow.
unsafe { start.checked_add_unsigned(n as $unsigned).unwrap_unchecked() }
}

#[inline]
unsafe fn backward_unchecked(start: Self, n: usize) -> Self {
// SAFETY: the caller has to guarantee that `start - n` doesn't overflow.
unsafe { start.checked_sub_unsigned(n as $unsigned).unwrap_unchecked() }
}
};
}

macro_rules! step_unsigned_methods {
() => {
#[inline]
unsafe fn forward_unchecked(start: Self, n: usize) -> Self {
Expand All @@ -198,7 +215,12 @@ macro_rules! step_identical_methods {
// SAFETY: the caller has to guarantee that `start - n` doesn't overflow.
unsafe { start.unchecked_sub(n as Self) }
}
};
}

// These are still macro-generated because the integer literals resolve to different types.
macro_rules! step_identical_methods {
() => {
#[inline]
#[allow(arithmetic_overflow)]
#[rustc_inherit_overflow_checks]
Expand Down Expand Up @@ -239,6 +261,7 @@ macro_rules! step_integer_impls {
#[unstable(feature = "step_trait", reason = "recently redesigned", issue = "42168")]
impl Step for $u_narrower {
step_identical_methods!();
step_unsigned_methods!();

#[inline]
fn steps_between(start: &Self, end: &Self) -> Option<usize> {
Expand Down Expand Up @@ -271,6 +294,7 @@ macro_rules! step_integer_impls {
#[unstable(feature = "step_trait", reason = "recently redesigned", issue = "42168")]
impl Step for $i_narrower {
step_identical_methods!();
step_signed_methods!($u_narrower);

#[inline]
fn steps_between(start: &Self, end: &Self) -> Option<usize> {
Expand Down Expand Up @@ -335,6 +359,7 @@ macro_rules! step_integer_impls {
#[unstable(feature = "step_trait", reason = "recently redesigned", issue = "42168")]
impl Step for $u_wider {
step_identical_methods!();
step_unsigned_methods!();

#[inline]
fn steps_between(start: &Self, end: &Self) -> Option<usize> {
Expand All @@ -360,6 +385,7 @@ macro_rules! step_integer_impls {
#[unstable(feature = "step_trait", reason = "recently redesigned", issue = "42168")]
impl Step for $i_wider {
step_identical_methods!();
step_signed_methods!($u_wider);

#[inline]
fn steps_between(start: &Self, end: &Self) -> Option<usize> {
Expand Down
5 changes: 5 additions & 0 deletions library/core/tests/iter/range.rs
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,11 @@ fn test_range_advance_by() {
assert_eq!(Ok(()), r.advance_back_by(usize::MAX));

assert_eq!((r.start, r.end), (0u128 + usize::MAX as u128, u128::MAX - usize::MAX as u128));

// issue 122420, Step::forward_unchecked was unsound for signed integers
let mut r = -128i8..127;
assert_eq!(Ok(()), r.advance_by(200));
assert_eq!(r.next(), Some(72));
}

#[test]
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
The loop took around 7s
The loop took around 12s

0 comments on commit 54328c7

Please sign in to comment.