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

@simonvandel

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

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

Did you bisect the regression to #76683?

Auto merge of rust-lang#78195 - tmiasko:instcombine, r=jonas-schievink

@tmiasko

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.

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

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(()); 
        
                           }

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.

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

Opened #78434 to disable it

Auto merge of rust-lang#78434 - jonas-schievink:disable-miropt, r=wes…

Fix rust-lang#78192

Why was this reopened and then closed?

@tmiasko

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

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

Navigation Menu

Repository navigation

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

24 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	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(());
	}

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

Description

Activity

tmiasko commented on Oct 21, 2020

LeSeulArtichaut commented on Oct 21, 2020

spastorino commented on Oct 21, 2020

spastorino commented on Oct 21, 2020

camelid commented on Oct 21, 2020

24 remaining items

simonvandel commented on Oct 27, 2020

jonas-schievink commented on Oct 27, 2020

jonas-schievink commented on Oct 27, 2020

camelid commented on Jan 23, 2021

LeSeulArtichaut commented on Jan 23, 2021

tmiasko commented on Jan 23, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions