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

ConstProp miscompilation around references #73609

Closed
RalfJung opened this issue Jun 22, 2020 · 13 comments · Fixed by #73613
Closed

ConstProp miscompilation around references #73609

RalfJung opened this issue Jun 22, 2020 · 13 comments · Fixed by #73613
Assignees
Labels
A-mir-opt Area: MIR optimizations C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical 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

@RalfJung
Copy link
Member

RalfJung commented Jun 22, 2020

#71911 introduced a (nightly-to-nightly) regression in mir-opt-level=3: since that landed, this assertion fails when running the test in Miri.

@jonas-schievink jonas-schievink added A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) A-mir-opt Area: MIR optimizations C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. requires-nightly This issue requires a nightly compiler in some way. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness labels Jun 22, 2020
@RalfJung

This comment has been minimized.

@RalfJung RalfJung changed the title Miscompilation with mir-opt-level=3 Miscompilation with mir-opt-level=2 Jun 22, 2020
@RalfJung
Copy link
Member Author

In fact even level 2 is broken.

@RalfJung
Copy link
Member Author

... and level 1, for the union issue (so let's treat the other one separately).

@RalfJung RalfJung added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed requires-nightly This issue requires a nightly compiler in some way. labels Jun 22, 2020
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 22, 2020
@RalfJung
Copy link
Member Author

RalfJung commented Jun 22, 2020

This doesn't require nightly after all, we have a stable-to-nightly regression when just running this code:

fn main() {
    #[allow(dead_code)]
    union U {
        f1: u32,
        f2: f32,
    }
    let mut u = U { f1: 1 };
    unsafe {
        let b1 = &mut u.f1;
        *b1 = 5;
    }
    assert_eq!(unsafe { u.f1 }, 5);
}

gives

thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `5`', src/main.rs:12:5

@RalfJung RalfJung changed the title Miscompilation with mir-opt-level=2 ConstProp miscompilation around unions Jun 22, 2020
@RalfJung
Copy link
Member Author

In fact we don't even need a union, nor an unsafe block:

fn main() {
    #[allow(dead_code)]
    struct U {
        f1: u32,
        f2: f32,
    }
    let mut u = U { f1: 1, f2: 1.0 };
    let b1 = &mut u.f1;
    *b1 = 5;
    assert_eq!( { u.f1 }, 5);
}

@RalfJung RalfJung changed the title ConstProp miscompilation around unions ConstProp miscompilation around references Jun 22, 2020
@RalfJung RalfJung removed the A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) label Jun 22, 2020
@jonas-schievink
Copy link
Contributor

Minimized:

fn main() {
    let mut u = (1,);
    *&mut u.0 = 5;
    assert_eq!( { u.0 }, 5);
}

@oli-obk oli-obk self-assigned this Jun 22, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Jun 22, 2020

on it

@oli-obk oli-obk added regression-from-stable-to-beta Performance or correctness regression from stable to beta. and removed regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Jun 22, 2020
@Dylan-DPC-zz
Copy link

giving this p-critical based on our discussion

@wesleywiser
Copy link
Member

Why is this regression-from-stable-to-beta when the offending PR only merged last night?

@Dylan-DPC-zz Dylan-DPC-zz added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jun 22, 2020
@Aaron1011
Copy link
Member

@RalfJung: Why didn't the assertion failure cause a Miri toolstate change?

@RalfJung
Copy link
Member Author

I think this is stable-to-nightly only... @oli-obk why did you change that?

@Aaron1011 rustc's CI currently only tests Miri with mir-opt-level=0.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 23, 2020

I tested it in the playground and I thought it also happened on beta?

@oli-obk oli-obk added regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. and removed regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Jun 23, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Jun 23, 2020

Hmm... not sure what happened, must have mis-clicked on the playground

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 C-bug Category: This is a bug. I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness P-critical Critical 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.

7 participants