`-Zmir-opt-level=2` misoptimises valid code under Tree Borrows

[cbeuw]

This code has UB under Stacked Borrows in Miri, but is fine with -Zmiri-tree-borrows, and it should print false

#![feature(custom_mir, core_intrinsics)]
extern crate core;
use core::intrinsics::mir::*;
#[custom_mir(dialect = "runtime", phase = "initial")]
pub fn fn0() -> bool {
    mir! {
    type RET = bool;
    let pair: (i8, bool);
    let ptr: *mut bool;
    {
        pair = (1, false);
        ptr = core::ptr::addr_of_mut!(pair.1);
        RET = pair.1 <= (*ptr);
        pair = (1, false);
        (*ptr) = RET | RET;
        RET = !pair.1;
        Return()
    }

    }
}
pub fn main() {
    println!("{}", fn0());
}

However, under -Zmir-opt-level=2 and above, it prints true

% rustc -Zmir-opt-level=2 repro.rs && ./repro
true

Meta

rustc --version --verbose:

rustc 1.71.0-nightly (1c42cb4ef 2023-04-26)
binary: rustc
commit-hash: 1c42cb4ef0544fbfaa500216e53382d6b079c001
commit-date: 2023-04-26
host: aarch64-apple-darwin
release: 1.71.0-nightly
LLVM version: 16.0.2

cc @Vanille-N @RalfJung

[cbeuw]

Here's a pure-surface Rust reproduction

pub fn fn0() -> bool {
    let mut pair = (1, false);
    let ptr = core::ptr::addr_of_mut!(pair.1);
    let mut ret = pair.1 <= unsafe { *ptr };
    pair = (1, false);
    unsafe {
        *ptr = ret | ret;
    }
    ret = !pair.1;
    return ret;
}

pub fn main() {
    println!("{}", fn0());
}

[RalfJung]

@JakobDegen any guess which MIR opt might be the culprit here?

[RalfJung]

Even more minimized reproducer:

pub fn fn0() -> bool {
    let mut pair = (1, false);
    let ptr = core::ptr::addr_of_mut!(pair.1);
    pair = (1, false);
    unsafe {
        *ptr = true;
    }
    let ret = !pair.1;
    return ret;
}

pub fn main() {
    println!("{}", fn0());
}

My first guess would be that some MIR analysis assumes that pair = (1, false) means that no pointer to pair can exist any more -- with Stacked Borrows that was true, but not with Tree Borrows. I usually push back against making Stacked Borrows assumptions in optimizations but looks like something slipped through?

Cc @cjgillot

[RalfJung]

Even stranger, with Rust 1.69 we get true even without optimizations. This goes back all the way to Rust 1.51; before that, we didn't have addr_of_mut. Only beta and nightly without optimizations seem to do this correctly.

[saethlin]

Does this look like the bad optimization?

-// MIR for `fn0` before ConstProp
+// MIR for `fn0` after ConstProp
 
 fn fn0() -> bool {
     let mut _0: bool;                    // return place in scope 0 at src/main.rs:1:17: 1:21
@@ -19,13 +19,13 @@ fn fn0() -> bool {
     }
 
     bb0: {
-        _1 = (const 1_i32, const false); // scope 0 at src/main.rs:2:20: 2:30
+        _1 = const (1_i32, false);       // scope 0 at src/main.rs:2:20: 2:30
         _2 = &raw mut (_1.1: bool);      // scope 1 at /rustc/1a6ae3d692cfb52b21d0f45ba50b659486e53d6c/library/core/src/ptr/mod.rs:2192:5: 2192:20
-        _1 = (const 1_i32, const false); // scope 2 at src/main.rs:4:5: 4:22
+        _1 = const (1_i32, false);       // scope 2 at src/main.rs:4:5: 4:22
         (*_2) = const true;              // scope 3 at src/main.rs:6:9: 6:20
-        _4 = (_1.1: bool);               // scope 2 at src/main.rs:8:16: 8:22
-        _3 = Not(move _4);               // scope 2 at src/main.rs:8:15: 8:22
-        _0 = _3;                         // scope 4 at src/main.rs:9:12: 9:15
+        _4 = const false;                // scope 2 at src/main.rs:8:16: 8:22
+        _3 = const true;                 // scope 2 at src/main.rs:8:15: 8:22
+        _0 = const true;                 // scope 4 at src/main.rs:9:12: 9:15
         return;                          // scope 0 at src/main.rs:10:2: 10:2
     }
 }

[RalfJung]

@saethlin yes, very much so. Specifically the _4 = const false is wrong, the rest then follows from that.

Did ConstProp never learn that locals that have their address taken cannot be propagated?

[saethlin]

I'm about to be busy for a few hours, so if someone else wants to bisect what changed with ConstProp they should do that. I'll check back later.

[cjgillot]

I've been suspecting such a bug without managing to reproduce it for a few months. The bug is in the CanConstProp visitor, which mistreats Projection place context. I can make a targeted fix in the hour.

[saethlin]

@cbeuw I'm curious, how did you come up with this example?

[cbeuw]

@saethlin I'm making a fuzzer targeting custom MIR :D

It's still quite incomplete and currently hosted on an ETH Zürich private GitLab instance https://gitlab.inf.ethz.ch/ou-plf/rustlantis. I guess I should make a public mirror on GitHub soon...

[saethlin]

That is awesome! This is exactly the kind of stuff I was hoping to find with one.

[matthiaskrgr]

Ah so should we report these? I think I read somewhere that custom mir is likely to get wrong and would always cause crashes then or something which is why I have ignored all of the custom-mir /core_instrinsics ICEs that I came across.

[saethlin]

This isn't an ICE, this is a change in behavior due to enabling an optimization.