Repository navigation
Miscompile in the GVN transform #130853
Description
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`
trueMeta
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.
Metadata
Metadata
Assignees
Labels
Area: Code generationArea: MIR optimizationsArea: MIR opt Global Value Numbering (GVN)Issue: Correct Rust code lowers to incorrect machine codeIssue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/SoundnessHigh priorityRelevant to the compiler team, which will review and decide on the PR/issue.Working group: MIR optimizations
Activity
RalfJung commentedon Sep 29, 2024
Cc @rust-lang/wg-mir-opt
RalfJung commentedon Sep 29, 2024
Nice find, thanks. :)
dianqk commentedon Sep 29, 2024
Hmm, I don't think it's a miscompile. Your unsafe code breaks the semantic of immutable borrowing.
RalfJung commentedon Sep 30, 2024
The test passes Miri. So no, it does not break the semantics of immutable borrowing as we understand them currently.
9 remaining items
workingjubilee commentedon Oct 11, 2024
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 commentedon Nov 1, 2024
@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 commentedon Nov 13, 2024
P-high review pre-triage: assigning this issue to @cjgillot since they have a PR open (#132461) to address this already.
Auto merge of rust-lang#133474 - RalfJung:gvn-miscompile, r=<try>
Auto merge of rust-lang#133474 - RalfJung:gvn-miscompile, r=compiler-…
Auto merge of #133474 - RalfJung:gvn-miscompile, r=compiler-errors
Auto merge of #150485 - dianqk:gvn-ssa-borrow, r=cjgillot
Auto merge of #150485 - dianqk:gvn-ssa-borrow, r=cjgillot
Auto merge of #150485 - dianqk:gvn-ssa-borrow, r=cjgillot
Auto merge of #150485 - dianqk:gvn-ssa-borrow, r=cjgillot