MIR Call terminator: evaluate destination place before arguments

[RalfJung]

See #155680 for context.
Cc @Amanieu @rust-lang/wg-mir-opt

Looking at codegen, the order there seems to be

• func
• dest
• args

However, the order also should not make any semantic difference since evaluating operands and places are read-only operations. It just matters for Miri since while evaluating arguments we set up "protectors" for in-place arguments.

[rustbot]

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @oli-obk, @lcnr

This PR changes MIR

cc @oli-obk, @JakobDegen, @vakaras

[rustbot]

r? @eholk

rustbot has assigned @eholk.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler, mir
• compiler, mir expanded to 73 candidates
• Random selection from 21 candidates

[Amanieu]

This feels like a step backwards since in rust-lang/rfcs#3943 I'm moving towards always having destination places (for assignments, calls, everything) evaluated last.

However, the order also should not make any semantic difference since evaluating operands and places are read-only operations. It just matters for Miri since while evaluating arguments we set up "protectors" for in-place arguments.

Isn't the order also significant due to tree/stack borrow invalidations? For example:

let _2 = &mut _1;
_1 = foo(*_2);

If the destination is evaluated first then the _2 pointer is invalidated and the dereference then becomes invalid. The same issue also works the other way around (*_2 = foo(_1)) if we choose the evaluate the destination first.

[Amanieu]

As another data point, it seems that for Yield terminators, not only do we evaluate the resume place last, we also only evaluate it after the coroutine is resumed.

[RalfJung]

Isn't the order also significant due to tree/stack borrow invalidations?

"evaluating the destination" means computing the place. It doesn't mean writing anything to that place. Obviously the actual write can only happen after the function returns.

So in your example, evaluating the destination just consists of looking up the address of _1 in the current stacktrace. Nothing aliasing-related happens here. The write to _1 happens after the function returns; only then does _2 get invalidated.

For the same reason, *_2 = foo(_1) is also fine. Before the call we load from _2 to figure out the place the result is written to, but we don't write yet. Then we read from _1 which under TB is fine because _2 is a two-phase borrow that's still in its "reserved" state. Then later we write to *_2 which activates the two-phase borrow, making it unique.

As another data point, it seems that for Yield terminators, not only do we evaluate the resume place last, we also only evaluate it after the coroutine is resumed.

I can't make heads or tails of that code, but this would be quite unexpected in my eyes. I would consider this a bug in coroutine lowering. Note that this is inconsistent with functions calls already before my PR -- for function calls, the destination unambiguously gets evaluated before the call. I don't think evaluating it after the call is even an option, the only question is in which order dest, func, and args get evaluated before the call.

[Amanieu]

Right, I'd forgotten that deref doesn't actually perform a read for the destination place. However you can still force a read by using index projections which perform a read of a local during destination place evaluation:

let _2 = &mut _1;
array[_1] = foo(*_2);
// If the destination is evaluated first, then the deref of the argument becomes invalid.
// If the argument is evaluated first then this is fine.

You can try this in the playground: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=41066970b751d6dd9bc53a16010521e5

With that said, all this shows is that the evaluation order does have a semantic difference, it's not an argument on whether or not we should make the change.