UB in <Range as Iterator>::advance_by (Step::forward_unchecked) for signed integers
#122420
Assignees
Comments
rustbot
added
the
needs-triage
jhpratt
added
I-unsound
20 tasks
|
Note that this is reachable on stable since the default impl of fn main() {
[(-128i8..127)].into_iter().flatten().nth(200);
} |
the8472
removed
the
requires-nightly
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this issue
Mar 14, 2024
…, r=Amanieu fix unsoundness in Step::forward_unchecked for signed integers Fixes rust-lang#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.
jhpratt
added a commit
to jhpratt/rust
that referenced
this issue
Mar 14, 2024
…, r=Amanieu fix unsoundness in Step::forward_unchecked for signed integers Fixes rust-lang#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.
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this issue
Mar 14, 2024
…, r=Amanieu fix unsoundness in Step::forward_unchecked for signed integers Fixes rust-lang#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.
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this issue
Mar 14, 2024
Rollup merge of rust-lang#122461 - the8472:fix-step-forward-unchecked, r=Amanieu fix unsoundness in Step::forward_unchecked for signed integers Fixes rust-lang#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.
@CAD97 did you find this with Miri or just use Miri to confirm the bug? |
|
Only confirmed/diagnosed with Miri, spotted manually when looking at the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I tried this code: [playground]
I expected to see this happen: No UB.
Instead, this happened: UB, confirmed by Miri.
Step::forward_unchecked(-128i8, 200usize)is called, which does(-128i8).unchecked_add(200usize as i8), or(-128i8).unchecked_add(-56i8).The implementation of
advance_byis correct here, andStep::forward_unchecked(-128i8, 200usize)should be safe to call. The implementation that does a wrappingascast fromusizetoiNbefore doing the unchecked math is the incorrect code here. Signed integers should switch to just using wrapping or unsigned math, whichever gets better codegen out of LLVM. We do havewrapping_add_unsignedandchecked_add_unsignedmethods now, but nounchecked_add_unsigned.Meta
rustc --version:The text was updated successfully, but these errors were encountered: