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

slice::get_mut() followed by slice::copy_from_slice generates unreachable panic branch #98294

Closed
bugadani opened this issue Jun 20, 2022 · 3 comments · Fixed by #101232
Closed
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. I-slow Issue: Problems and improvements with respect to performance of generated code. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@bugadani
Copy link
Contributor

In code like the following, the compiler misses a possible optimization: in f1, the length of dst is equal to the length of bytes, yet the compiler generates a call to len_mismatch_fail.

pub fn f1(a: &mut [u8], offset: usize, bytes: &[u8]) {
    if let Some(dst) = a.get_mut(offset..offset + bytes.len()) {
        dst.copy_from_slice(bytes);
    }
}

The compiler knows the lengths are equal (and thus, the panic is unreachable), because the following snippet optimizes the panic away:

pub fn f2(a: &mut [u8], offset: usize, bytes: &[u8]) {
    if let Some(dst) = a.get_mut(offset..offset + bytes.len()) {
        assert!(dst.len() == bytes.len());
        dst.copy_from_slice(bytes);
    }
}

This is a regression between rustc versions 1.51 and 1.52.

https://rust.godbolt.org/z/YhEY78E9P

@bugadani bugadani changed the title slice::get[_mut]() followed by slice::copy_from_slice generates unreachable panic branch slice::get_mut() followed by slice::copy_from_slice generates unreachable panic branch Jun 20, 2022
@workingjubilee workingjubilee added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. C-bug Category: This is a bug. labels Jul 27, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jul 27, 2022
@steffahn
Copy link
Member

@rustbot label I-slow, E-needs-bisection

@rustbot rustbot added E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc I-slow Issue: Problems and improvements with respect to performance of generated code. labels Jul 27, 2022
@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 3, 2022
@steffahn
Copy link
Member

steffahn commented Aug 30, 2022

This seems to be fixed on nightly. @rustbot label E-needs-test

Bisection would still be interesting IMO, both for the regression and the fix.

@rustbot rustbot added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Aug 30, 2022
@steffahn
Copy link
Member

Alright, that’s pretty boring in terms of rustc-bisection: it was fixed by #99464 (Update to LLVM 15), and it regressed in nightly-2021-03-05 which includes #81451 (Upgrade1 to LLVM 12).

@rustbot label -E-needs-bisection, +A-llvm.

Footnotes

  1. what’s up with the inconsistent naming, upgrade vs. update?

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. and removed E-needs-bisection Call for participation: This issue needs bisection: https://github.com/rust-lang/cargo-bisect-rustc labels Aug 30, 2022
@nikic nikic removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Aug 31, 2022
nikic added a commit to nikic/rust that referenced this issue Aug 31, 2022
Add a test to make that the failure condition for this pattern is
optimized away.

Fixes rust-lang#98294.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Sep 3, 2022
Add test for rust-lang#98294

Add a test to make that the failure condition for this pattern is optimized away.

Fixes rust-lang#98294.
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 7, 2022
Add test for rust-lang#98294

Add a test to make that the failure condition for this pattern is optimized away.

Fixes rust-lang#98294.
@bors bors closed this as completed in cbf3b24 Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. I-slow Issue: Problems and improvements with respect to performance of generated code. regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants