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

GVN causes moved function arguments to be used again #120613

Closed
cbeuw opened this issue Feb 3, 2024 · 3 comments · Fixed by #120688
Closed

GVN causes moved function arguments to be used again #120613

cbeuw opened this issue Feb 3, 2024 · 3 comments · Fixed by #120688
Assignees
Labels
A-mir-opt Area: MIR optimizations I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@cbeuw
Copy link
Contributor

cbeuw commented Feb 3, 2024

#![feature(custom_mir, core_intrinsics)]
extern crate core;
use core::intrinsics::mir::*;

#[inline(never)]
fn dump_var(x: [u128; 6]) {
    println!("{x:?}");
}

#[custom_mir(dialect = "runtime", phase = "initial")]
pub fn fn0() {
    mir! {
    let _12: usize;
    let _14: [u128; 6];
    let _26: ([u128; 6],);
    let _29: ([u128; 6],);
    let x: ();
    {
    _12 = 1_usize;
    _14 = [42; 6];
    _14[_12] = 1;
    _29 = (_14,);
    _26 = _29;
    Call(x = fn1(_29.0, _26), ReturnTo(bb14), UnwindUnreachable())
    }
    bb14 = {
    Return()
    }
    }
}

pub fn fn1(mut _13: [u128; 6], mut _14: ([u128; 6],)) {
    _14.0 = [0; 6];
    dump_var(_13);
    dump_var(_14.0);
}
pub fn main() {
    fn0();
}

Right:

$ rustc -Zmir-opt-level=2 -Zmir-enable-passes=-GVN -Copt-level=0 1717541-debug.rs && ./1717541-debug
[42, 1, 42, 42, 42, 42]
[0, 0, 0, 0, 0, 0]

Wrong:

$ rustc -Zmir-opt-level=2 -Copt-level=0 1717541-debug.rs && ./1717541-debug
[0, 0, 0, 0, 0, 0]
[0, 0, 0, 0, 0, 0]

The first argument to fn1 is partially moved. GVN causes the second argument to use the partially moved value:

--- a/mir_dump/1717541_debug.fn0.005-015.GVN.before.mir
+++ b/mir_dump/1717541_debug.fn0.005-015.GVN.after.mir
@@ -1,4 +1,4 @@
-// MIR for `fn0` before GVN
+// MIR for `fn0` after GVN
 
 fn fn0() -> () {
     let mut _0: ();
@@ -11,10 +11,10 @@ fn fn0() -> () {
     bb0: {
         _1 = const 1_usize;
         _2 = [const 42_u128; 6];
-        _2[_1] = const 1_u128;
+        _2[1 of 2] = const 1_u128;
         _4 = (_2,);
         _3 = _4;
-        _5 = fn1(move (_4.0: [u128; 6]), move _3) -> [return: bb1, unwind unreachable];
+        _5 = fn1(move (_4.0: [u128; 6]), _4) -> [return: bb1, unwind unreachable];
     }
 
     bb1: {
rustc 1.77.0-nightly (11f32b73e 2024-01-31)
binary: rustc
commit-hash: 11f32b73e0dc9287e305b5b9980d24aecdc8c17f
commit-date: 2024-01-31
host: x86_64-unknown-linux-gnu
release: 1.77.0-nightly
LLVM version: 17.0.6
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 3, 2024
@saethlin saethlin added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness A-mir-opt Area: MIR optimizations and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 3, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Feb 3, 2024
@saethlin saethlin added the regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. label Feb 3, 2024
@saethlin
Copy link
Member

saethlin commented Feb 3, 2024

I can't provoke the miscompilation on beta using this example (just had to replace ReturnTo(bb14) with bb14 to make it build), so I don't think there's any need to rush to fix this.

(Some part of GVN is used in the beta that's about to become stable, but it looks like it doesn't have this bug in it)

@RalfJung
Copy link
Member

RalfJung commented Feb 5, 2024

Cc @cjgillot

@apiraino
Copy link
Contributor

apiraino commented Feb 7, 2024

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

@rustbot rustbot added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Feb 7, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 7, 2024
GVN: also turn moves into copies with projections

Fixes rust-lang#120613
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 8, 2024
GVN: also turn moves into copies with projections

Fixes rust-lang#120613
@bors bors closed this as completed in 65aa9ea Feb 8, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 8, 2024
Rollup merge of rust-lang#120688 - cjgillot:gvn-partial-move, r=oli-obk

GVN: also turn moves into copies with projections

Fixes rust-lang#120613
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 I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-medium Medium priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. 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