Skip to content

GVN makes an incorrect index access #141251

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
dianqk opened this issue May 19, 2025 · 0 comments · Fixed by #141252
Closed

GVN makes an incorrect index access #141251

dianqk opened this issue May 19, 2025 · 0 comments · Fixed by #141252
Assignees
Labels
A-mir-opt Area: MIR optimizations A-mir-opt-GVN Area: MIR opt Global Value Numbering (GVN) A-rustlantis A miscompilation found by Rustlantis C-bug Category: This is a bug. I-miscompile Issue: Correct Rust code lowers to incorrect machine code I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@dianqk
Copy link
Member

dianqk commented May 19, 2025

From #141038 (comment).

The following code with -Zmir-enable-passes=+GVN triggers the assertion.

#![feature(custom_mir, core_intrinsics)]

use std::intrinsics::mir::*;

#[custom_mir(dialect = "runtime")]
pub fn repeat_place(mut idx1: usize, idx2: usize, val: &i32) -> i32 {
    mir! {
        let array;
        let elem;
        {
            array = [*val; 5];
            elem = &array[idx1];
            idx1 = idx2;
            RET = *elem;
            Return()
        }
    }
}

fn main() {
    assert_eq!(repeat_place(0, 5, &0), 0);
}
@dianqk dianqk self-assigned this May 19, 2025
@dianqk dianqk added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness C-bug Category: This is a bug. A-mir-opt Area: MIR optimizations I-miscompile Issue: Correct Rust code lowers to incorrect machine code A-mir-opt-GVN Area: MIR opt Global Value Numbering (GVN) A-rustlantis A miscompilation found by Rustlantis labels May 19, 2025
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels May 19, 2025
@dianqk dianqk added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels May 19, 2025
@jieyouxu jieyouxu added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 19, 2025
@bors bors closed this as completed in ee4efa1 May 28, 2025
rust-timer added a commit that referenced this issue May 28, 2025
Rollup merge of #141252 - dianqk:gvn-repeat-index, r=saethlin

gvn: bail out unavoidable non-ssa locals in repeat

Fixes #141251.

We cannot transform `*elem` to `array[idx1]` in the following code, as `idx1` has already been modified.

```rust
    mir! {
        let array;
        let elem;
        {
            array = [*val; 5];
            elem = &array[idx1];
            idx1 = idx2;
            RET = *elem;
            Return()
        }
    }
```

Perhaps I could transform it to `array[0]`, but I prefer the conservative approach.

r? mir-opt
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-mir-opt Area: MIR optimizations A-mir-opt-GVN Area: MIR opt Global Value Numbering (GVN) A-rustlantis A miscompilation found by Rustlantis C-bug Category: This is a bug. I-miscompile Issue: Correct Rust code lowers to incorrect machine code I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-high High priority 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.

3 participants