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

NRVO miropt is unsound #111005

Open
JakobDegen opened this issue Apr 30, 2023 · 0 comments
Open

NRVO miropt is unsound #111005

JakobDegen opened this issue Apr 30, 2023 · 0 comments
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 requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@JakobDegen
Copy link
Contributor

I tried this code:

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

#[custom_mir(dialect = "runtime", phase = "initial")]
pub fn wrong(arg: char) -> char {
    mir!({
        let temp = arg;
        RET = temp;
        temp = 'b';
        Return()
    })
}

fn main() {
    assert_eq!('a', wrong('a'));
}
$ rustc +master -Zmir-opt-level=0 -Zmir-enable-passes=+RenameReturnPlace test3.rs
$ ./test3
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `'a'`,
 right: `'b'`', test3.rs:16:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Meta

Reproduces on master at f5adff6bd8b29ac7dd173b36f0c8c35bb1c593c5, as well as

rustc --version --verbose:

rustc 1.71.0-nightly (7908a1d65 2023-04-17)
binary: rustc
commit-hash: 7908a1d65496b88626e4b7c193c81d777005d6f3
commit-date: 2023-04-17
host: x86_64-unknown-linux-gnu
release: 1.71.0-nightly
LLVM version: 16.0.2
@JakobDegen JakobDegen 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 C-bug Category: This is a bug. requires-nightly This issue requires a nightly compiler in some way. A-mir-opt Area: MIR optimizations labels Apr 30, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue May 8, 2023
Disable nrvo mir opt

See rust-lang#111005 and rust-lang#110902 . The ICE can definitely be hit on stable, the miscompilation I'm not sure about. The pass makes some pretty sketchy assumptions though, and we should not have it on while that's the case.

I'm not going to work on actually fixing this, it's probably not excessively difficult though.

r? rust-lang/mir-opt
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 8, 2023
Fortify and re-enable RNVO MIR opt

Fixes rust-lang#111005
Fixes rust-lang#110902

This PR re-enables NRVO opt that had been disabled in rust-lang#111007

To look for RVO opportunities, we walk the MIR backwards from `return` terminators. In addition to the former implementation, we look at all the traversed statements and terminators for writes. If a local is written-to or moved-from, we discard it from the RVO candidates (`assigned_locals` bitset). If we see an indirect write or move, we discard all borrowed locals from candidates.

cc `@JakobDegen`
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 requires-nightly This issue requires a nightly compiler in some way. 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.

1 participant