Reject tail calls of #[track_caller] functions

[WaffleLapkin]

To make tail calls possible either both functions need to be #[track_caller], or neither can. For simplicity I propose to just disallow using tail calling #[track_caller] functions / tail calling from #[track_caller] functions. I don't think #[track_caller] is useful with tail calls anyway.

However, we do need to have a quite late check, after monopolization (as otherwise you don't necessarily know if the callee has #[track_caller]).

I'm not very happy with this implementation -- error'ing in the backend seems like a hack and will probably lead to disagreements between backends. Ideally we'd have a mir specific check (that would also probably make getting the span easier...).

Fixes #144755

[rustbot]

r? @SparrowLii

rustbot has assigned @SparrowLii.
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

[WaffleLapkin]

Hm. Alternatively, instead of rejecting callee being #[track_caller], we could use a shim (similarly to what happens when casting a #[track_caller] fn to a function pointer). I'm not sure how to do that though 🤔

[bjorn3]

nit in PR description: s/monopolization/monomorphization

[Jules-Bertholet]

Hm. Alternatively, instead of rejecting callee being #[track_caller], we could use a shim (similarly to what happens when casting a #[track_caller] fn to a function pointer).

This is what we have to do, because adding #[track_caller] to a function must not be a breaking change. rust-lang/rfcs#3407 (comment)

[WaffleLapkin]

@Jules-Bertholet I think "must not" is too strict. We probably don't want it to be, but it could be a breaking change.

[Jules-Bertholet]

It was an explicit goal of the track_caller RFC to preserve that compatibility. It’s effectively a stable guarantee of the language that libraries can add or remove the attribute without fear of breaking dependents

[phi-go]

It was an explicit goal of the track_caller RFC to preserve that compatibility. It’s effectively a stable guarantee of the language that libraries can add or remove the attribute without fear of breaking dependents

Also see this comment: #88302 (comment)

We discussed this in the library api meeting just now. We agreed that #[track_caller] is not a promise that that attribute will stay there in the future. We can imagine such a promise might be made in the public documentation of a function, but that's not an implicit guarantee.
#[track_caller] is an important part of the quality of the standard library. Not having it in certain situations might be bad, but it's not a stability guarantee by itself.

Though, I feel like this guarantee is not really properly documented for users, for example, there is no mention of this in: https://doc.rust-lang.org/reference/attributes/codegen.html#r-attributes.codegen.track_caller

[phi-go]

I'm trying to figure #[track_caller] out for the RFC, so I hope you can help me with a few questions.

I don't think #[track_caller] is useful with tail calls anyway.

As all functions with tail calls would need to have #[track_caller] this would allow to track the top level caller function starting the recursion. (Nested #[track_caller] functions also track the top level caller.) I can see this being useful sometimes. Am I missing something?

The ___location info would just be an argument that is passed through untouched. Even if it is spilled on the stack this would not cause a growing stack per call. Is this correct?

It was an explicit goal of the track_caller RFC to preserve that compatibility. It’s effectively a stable guarantee of the language that libraries can add or remove the attribute without fear of breaking dependents

Playing this through with all relevant combinations I get this following table.
Where <caller function> -> <called function>, caller is a normal function, rec is a function using become, and f is either a normal function or a function using become. Add and rem indicate if #[track_caller] is added or removed from the caller function. The (no) and (yes) indicate if the called function has #[track_caller]. Finally, the cells indicate what needs to be done for the ___location information / argument.

	caller -> rec (no)	caller -> rec (yes)	rec -> f (no)	rec -> f (yes)
add	drops loc	forward loc	drops loc	forward loc
rem	normal call	use call ___location in caller	normal call	use call ___location in rec

So, to my understanding we are dealing with exactly the same issues as for normal calls which track the caller. I would guess we could even use the same functionality. However, the main issue is that we need to special case the check for matching arguments? Also there could be a hidden performance cost, adding and dropping the ___location information, which seems to be a documentation / lint issue. There would not be a unexpected stack growth as we do not track all callers in the call stack but only one caller.

I'm not saying we need to support this fully, just trying to understand the topic.

[bjorn3]

For portability reasons we have to ensure that the ABI of the caller and callee match exactly when doing a tail call. Adding or removing an argument for the caller ___location may result in the amount of stack space for arguments changing, which can't be implemented on caller-pops calling conventions (like we use on most (all?) architectures as that is what the C ABI has to do to support varargs) for tail calls. The caller of the first tail-call would expect to pop X bytes of the stack, but the last tail-call may have Y bytes on the stack that need to be popped if the ABI of the caller and callee don't exactly match. Changing the amount of arguments can only work when you either ensure all arguments stay in registers or you use a callee-pop calling convention (as eg Wasmtime does for wasm tail calls). The former restriction is even worse that not allowing #[track_caller] at all IMO, while the latter option is not supported by LLVM on most architectures right now, so that is not an option either.

[phi-go]

Ah, I see, for some reason I was thinking that this would use callee cleanup. Thank you for the answer.

The discussion around the calling convention also still seems to be quite active, looking at recent comments in the RFC PR.

[WaffleLapkin]

I've open #144865 with al alternative approach.