InstCombine introduces an incorrect use of a local after its storage has ended

[tmiasko]

pub fn f<T>(a: &T) -> *const T {
    let b: &*const T = &(a as *const T);
    *b
}

--- mir_dump/a.f.005-003.InstCombine.before.mir
+++ mir_dump/a.f.005-003.InstCombine.after.mir
@@ -1,2 +1,2 @@
-// MIR for `f` before InstCombine
+// MIR for `f` after InstCombine
 
@@ -18,5 +18,5 @@
         _3 = &_4;                        // scope 0 at a.rs:2:24: 2:40
-        _2 = &(*_3);                     // scope 0 at a.rs:2:24: 2:40
+        _2 = _3;                         // scope 0 at a.rs:2:24: 2:40
         StorageDead(_3);                 // scope 0 at a.rs:2:40: 2:41
-        _0 = (*_2);                      // scope 1 at a.rs:3:5: 3:7
+        _0 = (*_3);                      // scope 1 at a.rs:3:5: 3:7
         StorageDead(_4);                 // scope 0 at a.rs:4:1: 4:2

Found by MIR validator #77369 & #78147.

cc @simonvandel #76683

[tmiasko]

I will open a PR to disable problematic part of InstCombine for now.

[LeSeulArtichaut]

Did you bisect the regression to #76683?

[spastorino]

I see that we've disabled the optimization, can't we also add a regression test for it? cc @tmiasko

EDIT (camelid): See the relevant discussion in wg-prioritization.

[spastorino]

Assigning P-critical as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

[camelid]

Is the cause of the unsoundness in this code? I haven't done MIR stuff before, but it looks like to me like it's not checking that the storage is live...

rust/compiler/rustc_mir/src/transform/instcombine.rs

Lines 149 to 167 in 1d27267

	for stmt in statements_to_look_in {
	match &stmt.kind {
	// Exhaustive match on statements to detect conditions that warrant we bail out of the optimization.
	rustc_middle::mir::StatementKind::Assign(box (l, r))
	if l.local == local_being_derefed =>
	{
	match r {
	// Looking for immutable reference e.g _local_being_deref = &_1;
	Rvalue::Ref(
	_,
	// Only apply the optimization if it is an immutable borrow.
	BorrowKind::Shared,
	place_taken_address_of,
	) => {
	self.optimizations
	.unneeded_deref
	.insert(location, *place_taken_address_of);
	return Some(());
	}

[tmiasko]

The optimization starts by locating a dereference of a local: ... = (*_a);, then looks back to determine a reaching definition of _a. If the definition is of the form _a = &p; it replaces (*_a) with p. In original example, _a would be _2, and p would be (*_3), so

_2 = &(*_3);
StorageDead(_3);
_0 = (*_2);

is changed as follows (there is an additional change in the diff, but it is a separate optimization, so lets ignore it):

_2 = &(*_3);
StorageDead(_3);
_0 = (*_3);

In this case, the primary issue is that reading _3 after StorageDead(_3) is undefined behaviour. The optimization never ensured that evaluating place later will give an equivalent result. The computation of reaching definition also looks suspect to me (it should stop in a presence of any indirect assignments or check that address of _a is never taken).

[tmiasko]

Example that leads to an end-to-end miscompilation:

fn main() {
    let a = 1u32;
    let b = 2u32;

    let mut c: *const u32 = &a;
    let d: &u32 = &b;

    let x = unsafe { &*c };
    c = d;
    let z = *x;

    assert_eq!(1, z);
}

[tmiasko]

This is now enabled by default again, and still an issue. Example from #78192 (comment):

$ rustc +stage1 -Aunused a.rs -Zmir-opt-level=0 && ./a
$ rustc +stage1 -Aunused a.rs && ./a
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `1`,
 right: `2`', a.rs:12:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

[camelid]

Assigning P-critical and removing I-prioritize as discussed in the prioritization working group.

[simonvandel]

I will have time to investigate this after today 1700(utc+1). If anyone finds the cause or arrives at a solution before, go ahead and open a PR.

[jonas-schievink]

This optimization should stay disabled by default until we're sure that it is sound.

[jonas-schievink]

Opened #78434 to disable it

[camelid]

Why was this reopened and then closed?

[LeSeulArtichaut]

I reopened it after #78434 since the bug wasn't fixed, only disabled so it doesn't hit stable. Is it fixed now @tmiasko?

[tmiasko]

I closed it because the code responsible is no longer there.