Storage liveness is generated incorrectly for this code snippet

[nagisa]

fn good_loop() {
    let mut should_break = false;
    loop {
        if *should_break {
            break;
        }
        should_break = true;
    }
}

has

    bb1: {
        StorageLive(_4);                 // scope 1 at test.rs:4:12: 4:24
        _4 = _1;                         // scope 1 at test.rs:4:12: 4:24
        StorageDead(_4);                 // scope 1 at test.rs:4:24: 4:24
        if(_4) -> [true: bb2, false: bb3]; // scope 1 at test.rs:4:9: 6:10
    }

as the generated MIR. The LLVM IR ends up being generated correctly, because _4 is determined to be an SSA-like lvalue, but I fear that there may also be cases where equivalent of _4 would be forced into an alloca (and generate incorrect IR, after some compiler change maybe?) and would certainly interfere with MIR optimisations and MIR borrowck.

[Aatch]

/cc @rust-lang/compiler

[jrmuizel]

It seems like this could possibly be the cause of #39455

[Aatch]

Related: #39685

[arielb1]

This has to be fixed for MIR borrowck and the fix should not be that hard.

[nikomatsakis]

@arielb1 I forget -- did we have a planned fix? =)

It seems like adopting the "extended basic block" formulation might help here, but given that IF is now SwitchInt, and we want to support the notion of a "complete set with no fallthrough", I'm not sure if it really does. That is, ultimately you want to push the StorageDead into the successors, right? Only other alternative I can see is adding implicit mutable condition codes, which I am not keen on.

[nikomatsakis]

As discussed in @rust-lang/compiler meeting, the plan would be to introduce a fresh temporary with the value being tested. This temporary would have no storage-live/storage-dead, so it technically "lives forever", but since it's never borrowed, its lifetime can be trivially analyzed. In LLVM terms, it'd be represented as an SSA value.

[nikomatsakis]

One other point is that we would want to be careful around constants (like false), which don't need said temporary.

[arielb1]

StorageDead is not really needed unless you have borrows.

[jrmuizel]

It looks like this fixed #39455. Is it possible to get this uplifted all the way to stable? It seems like it can cause some pretty insidious problems.

[nagisa]

Is it possible to get this uplifted all the way to stable?

No. We do not release point stable releases, except for very exceptional cases.

The fix seems non-trivial, so I doubt we’d backport it to beta either, especially given the fact that train rollover is near.

[jrmuizel]

Do we have a good idea of how much code will be miscompiled without this fix?

[nagisa]

Not much at all. There’s a number of conditions that are necessary to reproduce this issue and -Clto is a considerably rarely used option.