Skip to content

GVN misunderstands aliasing, can create overlapping assignments #141038

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

Closed
saethlin opened this issue May 15, 2025 · 4 comments · Fixed by #141218
Closed

GVN misunderstands aliasing, can create overlapping assignments #141038

saethlin opened this issue May 15, 2025 · 4 comments · Fixed by #141218
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-medium Medium priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@saethlin
Copy link
Member

saethlin commented May 15, 2025

Reduced example from rustlantis, which is accepted by Miri without optimizations enabled:

#![feature(custom_mir, core_intrinsics)]

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

#[custom_mir(dialect = "runtime")]
fn fn17() {
    mir!{
        let _17: Adt;
        let _33: *mut Adt;
        let _48: u32;
        let _73: &Adt;
        {
            _17 = Adt::Some(0);
            _33 = core::ptr::addr_of_mut!(_17);
            _73 = &(*_33);
            _48 = Field(Variant((*_73), 1), 0);
            (*_33) = Adt::Some(_48);
            Return()
        }
    }
}

fn main() {
    fn17();
}

enum Adt {
    None,
    Some(u32),
}

If I run this under Miri with -Zmir-enable-passes=+GVN, I see:

error: Undefined Behavior: `copy_nonoverlapping` called on overlapping ranges
  --> gvn-overlapping.rs:17:13
   |
17 |             (*_33) = Adt::Some(_48);
   |             ^^^^^^^^^^^^^^^^^^^^^^^ `copy_nonoverlapping` called on overlapping ranges
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
   = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
   = note: BACKTRACE:
   = note: inside `fn17` at gvn-overlapping.rs:17:13: 17:36

The MIR diff for GVN is:

-// MIR for `fn17` before GVN
+// MIR for `fn17` after GVN
 
 fn fn17() -> () {
     let mut _0: ();
@@ -8,11 +8,15 @@ fn fn17() -> () {
     let mut _4: &Adt;
 
     bb0: {
-        _1 = Adt::Some(const 0_u32);
+        _1 = const Adt::Some(0_u32);
         _2 = &raw mut _1;
         _4 = &(*_2);
         _3 = copy (((*_4) as variant#1).0: u32);
-        (*_2) = Adt::Some(copy _3);
+        (*_2) = copy (*_4);
         return;
     }
 }
+
+alloc1 (size: 8, align: 4) {
+    01 00 00 00 00 00 00 00                         │ ........
+}
@saethlin saethlin added the C-bug Category: This is a bug. label May 15, 2025
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 15, 2025
@saethlin saethlin added 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) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 15, 2025
@jieyouxu jieyouxu removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label May 15, 2025
@dianqk dianqk self-assigned this May 16, 2025
@saethlin saethlin added the I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness label May 16, 2025
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label May 16, 2025
@saethlin saethlin added P-medium Medium priority A-rustlantis A miscompilation found by Rustlantis and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels May 16, 2025
@saethlin saethlin changed the title GVN can create overlapping assignments GVN misunderstands aliasing, can create overlapping assignments May 18, 2025
@saethlin
Copy link
Member Author

saethlin commented May 18, 2025

Found a similar issue with GVN, the reproducer looks different but I suspect it's the same aliasing problem:

#![feature(custom_mir, core_intrinsics)]
#![allow(internal_features)]

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

#[custom_mir(dialect = "runtime")]
fn main() {
    mir!{
        let _1;
        let _2: &'static i32;
        let _3;
        let _4;
        let _5;
        let _6: &'static i32;
        let _7;
        let _8;
        let _9;
        {
            _4 = 0;
            _5 = 0;
            _6 = &_4;
            _2 = _6;
            _7 = [*_2, *_2, *_2, *_2, *_2];
            Goto(bb1)
        }
        bb1 = {
            _3 = 0;
            match _5 {
                0 => bb2,
                _ => bb1,
            }
        }
        bb2 = {
            _6 = &_9;
            _8 = &_7[_5];
            _5 = 5;
            match _3 {
                0 => bb3,
                _ => bb2,
            }
        }
        bb3 = {
            _1 = *_8;
            Return()
        }
    }
}
error: Undefined Behavior: indexing out of bounds: the len is 5 but the index is 5
  --> 7735683935374907524.rs:43:13
   |
43 |             _1 = *_8;
   |             ^^^^^^^^ indexing out of bounds: the len is 5 but the index is 5
   |
     bb2: {
         _6 = &_9;
         _8 = &_7[_5];
         _5 = const 5_usize;
-        switchInt(copy _3) -> [0: bb3, otherwise: bb2];
+        switchInt(const 0_i32) -> [0: bb3, otherwise: bb2];
     }
 
     bb3: {
-        _1 = copy (*_8);
+        _1 = copy _7[_5];
         return;
     }

@dianqk
Copy link
Member

dianqk commented May 18, 2025

#141218 fixes the first one.

fmease added a commit to fmease/rust that referenced this issue May 18, 2025
gvn: avoid creating overlapping assignments

Quick fix rust-lang#141038, as I couldn't find a way to avoid in-place modification. I'm considering handling all `ravlue` modifications within the `visit_statement` function.

r? mir-opt
@bors bors closed this as completed in 50b20b7 May 18, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this issue May 18, 2025
Rollup merge of rust-lang#141218 - dianqk:gvn-overlapping, r=oli-obk

gvn: avoid creating overlapping assignments

Quick fix rust-lang#141038, as I couldn't find a way to avoid in-place modification. I'm considering handling all `ravlue` modifications within the `visit_statement` function.

r? mir-opt
@dianqk
Copy link
Member

dianqk commented May 18, 2025

Reopen for the second one.

@dianqk
Copy link
Member

dianqk commented May 19, 2025

I think this could be a new issue: #141251. Also, I'd like to backport this fix.

@dianqk dianqk closed this as completed May 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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-medium Medium 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.

4 participants