Allocate arguments from topmost frame into temporary storage before popping stack frame in init_fn_tail_call

[compiler-errors]

When doing a tail call, we pop the topmost frame and then push a new frame to replace it. If we have an argument that is being passed indirectly from an mplace of a local from that old stack frame, then it will be invalidated before we can copy them into the locals of the new stack frame.

THis PR detects arguments that are being indirectly via pointers which point into the allocations of the topmost stack frame's locals. If we find an argument, we copy it into new temporary memory for constructing the new stack frame, and then we deallocate that old memory.

Not totally sure who should review this, since I'm not totally sure if I'm using CTFE correctly 😎 Specifically I don't know if I am implementing the right logic for "does this argument come from an allocation that corresponds to a local in the top stack frame".

r? @WaffleLapkin @RalfJung @oli-obk

Fix #144820

[rustbot]

wafflelapkin is currently at their maximum review capacity.
They may take a while to respond.

[rustbot]

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

[WaffleLapkin]

Both Ralf and Oli are on vacations so I guess I'll have to review it for now ^^'

This looks reasonable to me, r=me with/out nits.

[WaffleLapkin]

Suggested change

	match self.value {
	LocalValue::Dead => false,
	LocalValue::Live(val) => match val {
	Operand::Immediate(_) => false,
	Operand::Indirect(_) => true,
	},
	}
	match self.value {
	LocalValue::Dead | LocalValue::Live(Operand::Immediate(_)) => false,
	LocalValue::Live(Operand::Indirect(_)) => true,
	}

[WaffleLapkin]

Can you wrap this in std::cell::LazyCell, so that we don't create a hash map if none of the arguments have mplaces?

[RalfJung]

I won't have time to review this during my vacation, but as a quick first note -- I rather strongly object to anything that involves special casing move arguments based on whether they happen to be a local in the caller's stack frame. That kind of ad-hoc special case is surely not something we want in our MIR spec. (Currently the interpreter implements the de-facto spec for tail calls, so what you are suggesting here is a change in the MIR spec.) I suggest we figure out what the spec should be before adding hacks like this to every backend implementing tail calls (ISTM the same hacks would be required in the codegen backend, if it supported tail calls).

It would seem to me that passing a move argument to a tail call when the argument doesn't outlive the callee is simply UB. If you move an argument out of a Box and free the box during the call, surely that is UB. So I think the bug here is in MIR building, not in the interpreter. Fixing the MIR would also nicely fix this for all backends, instead of requiring complicated logic in each of them.

[RalfJung]

I'd appreciate if you could hold off on landing this until I'm back, since this is an unstable feature I hope it's not urgent.

[compiler-errors]

I'll wait until Ralf is back.

I will note that I don't see what this has to do with move arguments. This has to do with arguments that are passed indirectly and how that interacts with the current spec for tail calls.

Right now we pop a stack frame and deinit the locals in that stack frame before initializing a new stack frame for the tail call. If an arg is being passed indirectly from an allocation corresponding to local that has been deinited, then I don't see any way to make that a defined operation.

[WaffleLapkin]

This is a general tail call problem -- you need to move the indirect ABI arguments somewhere off of the current stack frame. I think this is (one of?) the reason(s?) why we have to have the same signature requirement -- that allows backends to move arguments to the caller (pseudocode):

fn src_tail(arg: /* implicitly passed by ref in the ABI */ String) {
    let local = arg.clone();
    drop(arg);
    become src_tail(local)
}

// ==>>

fn src_tail(arg: /* implicitly passed by ref in the ABI */ String) {
    let local = arg.clone();
    drop(arg);
    write_through_implicit_ref(arg, local);
    become src_tail(arg)
}

I'm not sure we we want to emulate this in miri, but we do have to do something. I agree that we should indeed wait for Ralf to return though ^^'

[WaffleLapkin]

@rustbot blocked (on Ralf's return)

[RalfJung]

This is a general tail call problem

Exactly. So what is the general plan to deal with this? Every backend needs to detect this case and then store the argument for the tail call in its own parameter slots as those are the only slots it knows that are guaranteed to have the right size and lifetime? Would it make sense to instead have that logic in MIR so it is shared across all backends and then Miri also gets it "for free"?

Basically it seems like we need all tail calls to be of the form

_1 = arg1;
_2 = arg2;
...
become f(_1, _2, ...);

i.e. we have to reuse our own parameter locals. Makes one wonder why beomce even takes an argument list, it could just be

_1 = arg1;
_2 = arg2;
...
become f; // implicitly: pass the arguments from our own parameter slots

[compiler-errors]

Just noting that storing the args in the current caller's param locals and then becomeing with no args could possibly complicate borrowck, since for example we can tail-call a callee with the same signature modulo lifetimes, and those args could be shorter (e.g. call from &'long str to &'short str).

[RalfJung]

I guess that means also the form with explicit lifetimes would be problematic since e.g _1 would have the wrong type (wrong lifetime)? So this would have to be done somewhere on analysis MIR but after borrowck, and the MIR semantics before this step are... kind of weird, they have to magically conjur up and then later destroy the stack slots for these arguments.

But also I think I misunderstood the problem. This isn't about move at all, it affects copy arguments as well. Even if we did rewrite the call to f(_1, _2, ...), the problem would persist.
What really happens is that we deallocate the caller's locals before initializing the callee's arguments, and that... is kind of wrong? Well, in MiniRust it'd probably be right, but in Miri we we can't quite do that so we may want to delay deallocating the caller's locals until after the callee's stack frame has been initialized.

But that's not enough -- if we do that, then we'll have the problem with move arguments. This is the harder conceptual issue. I feel like we just can't use move with tail calls since the entire point of move is to do in-place argument passing and, well, that can't work when the place the argument is stored in disappears before the callee starts executing...