Rollup merge of #122461 - the8472:fix-step-forward-unchecked, r=Amanieu

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.
This commit is contained in:
Matthias Krüger 2024-03-14 11:10:00 +01:00 committed by GitHub
commit 75dc99b996
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 34 additions and 3 deletions

View File

@ -183,8 +183,25 @@ unsafe fn backward_unchecked(start: Self, count: usize) -> Self {
}
}
// 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 {
@ -197,7 +214,12 @@ unsafe fn backward_unchecked(start: Self, n: usize) -> Self {
// 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]
@ -238,6 +260,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> {
@ -270,6 +293,7 @@ fn backward_checked(start: Self, n: usize) -> Option<Self> {
#[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> {
@ -334,6 +358,7 @@ fn backward_checked(start: Self, n: usize) -> Option<Self> {
#[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> {
@ -359,6 +384,7 @@ fn backward_checked(start: Self, n: usize) -> Option<Self> {
#[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> {

View File

@ -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]

View File

@ -1 +1 @@
The loop took around 7s
The loop took around 12s