diff options
| author | Matthias Krüger <matthias.krueger@famsik.de> | 2024-03-14 11:10:00 +0100 |
|---|---|---|
| committer | GitHub <noreply@github.com> | 2024-03-14 11:10:00 +0100 |
| commit | 75dc99b9963a3553e515f9c5da50eb19fbcdaba4 (patch) | |
| tree | 252a834a19d085b785f510b96397c14d0863808b | |
| parent | bdf84ea00ec8f81adb0857fe39df34be045daca2 (diff) | |
| parent | d3cab9f4d6c778da99185a4548ef7f0899f04766 (diff) | |
| download | rust-75dc99b9963a3553e515f9c5da50eb19fbcdaba4.tar.gz rust-75dc99b9963a3553e515f9c5da50eb19fbcdaba4.zip | |
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.
| -rw-r--r-- | library/core/src/iter/range.rs | 30 | ||||
| -rw-r--r-- | library/core/tests/iter/range.rs | 5 | ||||
| -rw-r--r-- | src/tools/miri/tests/pass/shims/time-with-isolation2.stdout | 2 |
3 files changed, 34 insertions, 3 deletions
diff --git a/library/core/src/iter/range.rs b/library/core/src/iter/range.rs index 2ecf9ce5b81..055ead117ea 100644 --- a/library/core/src/iter/range.rs +++ b/library/core/src/iter/range.rs @@ -183,8 +183,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 { @@ -197,7 +214,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] @@ -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 @@ 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> { @@ -334,6 +358,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> { @@ -359,6 +384,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> { diff --git a/library/core/tests/iter/range.rs b/library/core/tests/iter/range.rs index 9af07119a89..e31db0732e0 100644 --- a/library/core/tests/iter/range.rs +++ b/library/core/tests/iter/range.rs @@ -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] diff --git a/src/tools/miri/tests/pass/shims/time-with-isolation2.stdout b/src/tools/miri/tests/pass/shims/time-with-isolation2.stdout index c68b40b744b..dce51a7fdbe 100644 --- a/src/tools/miri/tests/pass/shims/time-with-isolation2.stdout +++ b/src/tools/miri/tests/pass/shims/time-with-isolation2.stdout @@ -1 +1 @@ -The loop took around 7s +The loop took around 12s |
