Miscompile in the GVN transform

[jwong101]

I tried this code:

unsafe fn src(x: &&u8) -> bool {
    let y = **x;
    unknown();
    **x == y
}

static mut SUSSY: *mut u8 = core::ptr::null_mut();

#[inline(never)]
unsafe fn unknown() {
    *SUSSY = 1;
}

fn main() {
    let mut s = 0;
    unsafe {
        SUSSY = core::ptr::addr_of_mut!(s);
        println!("{}", src(&*core::ptr::addr_of!(SUSSY).cast::<&u8>()));
    }
}

I expected to see this happen:

This should print false, as I believe this is DB under both Stacked and Tree borrows(according to MIRI).

Instead, this happened:

It returns false in Debug mode, and the GVN MIR pass makes src() unconditionally return true in Release mode.

$ cargo miri run
Compiling sus v0.1.0 (/Users/jwong3/test/sus)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.17s
     Running `/Users/jwong3/.rustup/toolchains/nightly-aarch64-apple-darwin/bin/cargo-miri runner target/miri/aarch64-apple-darwin/debug/sus`
false

$ cargo run -r
   Compiling sus v0.1.0 (/Users/jwong3/test/sus)
    Finished `release` profile [optimized] target(s) in 0.31s
     Running `target/release/sus`
true

Meta

Here's the MIR before the GVN pass:

// MIR for `src` before GVN

fn src(_1: &&u8) -> bool {
    debug x => _1;
    let mut _0: bool;
    let _2: u8;
    let _3: ();
    let mut _4: u8;
    let mut _5: u8;
    let mut _6: &u8;
    let mut _7: &u8;
    scope 1 {
        debug y => _2;
    }

    bb0: {
        StorageLive(_2);
        _6 = deref_copy (*_1);
        _2 = copy (*_6);
        _3 = unknown() -> [return: bb1, unwind continue];
    }

    bb1: {
        StorageLive(_4);
        _7 = deref_copy (*_1);
        _4 = copy (*_7);
        StorageLive(_5);
        _5 = copy _2;
        _0 = Eq(move _4, move _5);
        StorageDead(_5);
        StorageDead(_4);
        StorageDead(_2);
        return;
    }
}

After

// MIR for `src` after GVN

fn src(_1: &&u8) -> bool {
    debug x => _1;
    let mut _0: bool;
    let _2: u8;
    let _3: ();
    let mut _4: u8;
    let mut _5: u8;
    let mut _6: &u8;
    let mut _7: &u8;
    scope 1 {
        debug y => _2;
    }

    bb0: {
        nop;
        _6 = copy (*_1);
        _2 = copy (*_6);
        _3 = unknown() -> [return: bb1, unwind continue];
    }

    bb1: {
        StorageLive(_4);
        _7 = copy _6;
        _4 = copy _2;
        StorageLive(_5);
        _5 = copy _2;
        _0 = const true;
        StorageDead(_5);
        StorageDead(_4);
        nop;
        return;
    }
}

It would be justified to make src() return true if _6 was dereferenced again in bb1, however, the write in unknown() shouldn't invalidate the actual pointer stored in _1 if my understanding of Stacked Borrows is correct.

This is present in both Stable and Nightly.

[RalfJung]

Cc @rust-lang/wg-mir-opt

[RalfJung]

Nice find, thanks. :)

[DianQK]

Hmm, I don't think it's a miscompile. Your unsafe code breaks the semantic of immutable borrowing.

[RalfJung]

The test passes Miri. So no, it does not break the semantics of immutable borrowing as we understand them currently.

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

[RalfJung]

Cc @rust-lang/opsem

[lqd]

cc @cjgillot

[workingjubilee]

...huh, the semantics of immutable borrowing can't justify unifying two reads of a reference to a reference of a Freeze type?

[workingjubilee]

I thought this was THE optimization we were doing this for?

[oxalica]

...huh, the semantics of immutable borrowing can't justify unifying two reads of a reference to a reference of a Freeze type?

There are already some discussion in the linked issue and more linked issues in it. rust-lang/unsafe-code-guidelines#532

I also agree this is my intuition about the goal of aliasing analysis. But it's not for now. So at least one of the current model and this optimization should change.

BTW, the optimization in this issue already exists since 1.77.0 (https://godbolt.org/z/anPK3Ef5z) which is more than 7months ago. Is there any more user data, other than this, showing that this optimization is unintended/un-intuitive? If not, I think we may need to question about the current model itself.

[RalfJung]

I thought this was THE optimization we were doing this for?

Not quite, THE optimization is for reads from a reference.

The nested case is a lot more complicated, and so far there's no model at all that could justify the optimization.

There are already some discussion in the linked issue and more linked issues in it. rust-lang/unsafe-code-guidelines#532

Yeah that issue description still needs to be rewritten to be a self-contained introduction of the problem -- see my comment there. :)

If not, I think we may need to question about the current model itself.

That's what rust-lang/unsafe-code-guidelines#532 is for. But we don't do optimizations that we don't have a model for, so this remains an I-unsound issue until a model is proposed (and ideally implemented in Miri).

[workingjubilee]

Yeah, we have to drop this optimization for now, not because it is unjustifiable per se but because it is unjustified at the moment.

[RalfJung]

@cjgillot you are our main expert for the GVN transform... any chance you could take a look at this? Or leave some notes as starting points for someone else on what would be the best way to fix this?

[jieyouxu]

P-high review pre-triage: assigning this issue to @cjgillot since they have a PR open (#132461) to address this already.