Reject tail calls of #[track_caller] functions
#144762
Conversation
|
r? @SparrowLii rustbot has assigned @SparrowLii. Use |
rustbot
added
A-LLVM
|
Hm. Alternatively, instead of rejecting callee being |
|
nit in PR description: s/monopolization/monomorphization |
This is what we have to do, because adding |
|
@Jules-Bertholet I think "must not" is too strict. We probably don't want it to be, but it could be a breaking change. |
|
It was an explicit goal of the |
Also see this comment: #88302 (comment)
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 |
|
I'm trying to figure
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?
Playing this through with all relevant combinations I get this following table.
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. |
|
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 |
|
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. |
|
I've open #144865 with an alternative approach. |
rustbot
removed
the
S-waiting-on-review
Fix tail calls to `#[track_caller]` functions We want `#[track_caller]` to be semver independent, i.e. it should not be a breaking change to add or remove it. Since it changes ABI of a function (adding an additional argument) we have to be careful to preserve this property when adding tail calls. The only way to achieve this that I can see is: - we forbid tail calls in functions which are marked with `#[track_caller]` (already implemented) - tail-calling a `#[track_caller]` marked function downgrades the tail-call to a normal call (or equivalently tail-calls the shim made by fn def to fn ptr cast) (this pr) Ideally the downgrade would be performed by a MIR pass, but that requires post mono MIR opts (cc `@saethlin,` rust-lang#131650). For now I've changed code in cg_ssa to accomodate this behaviour (+ added a hack to mono collector so that the shim is actually generated) Additionally I added a lint, although I don't think it's strictly necessary. Alternative to rust-lang#144762 (and thus closes rust-lang#144762) Fixes rust-lang#144755
Fix tail calls to `#[track_caller]` functions We want `#[track_caller]` to be semver independent, i.e. it should not be a breaking change to add or remove it. Since it changes ABI of a function (adding an additional argument) we have to be careful to preserve this property when adding tail calls. The only way to achieve this that I can see is: - we forbid tail calls in functions which are marked with `#[track_caller]` (already implemented) - tail-calling a `#[track_caller]` marked function downgrades the tail-call to a normal call (or equivalently tail-calls the shim made by fn def to fn ptr cast) (this pr) Ideally the downgrade would be performed by a MIR pass, but that requires post mono MIR opts (cc ``@saethlin,`` rust-lang#131650). For now I've changed code in cg_ssa to accomodate this behaviour (+ added a hack to mono collector so that the shim is actually generated) Additionally I added a lint, although I don't think it's strictly necessary. Alternative to rust-lang#144762 (and thus closes rust-lang#144762) Fixes rust-lang#144755
Fix tail calls to `#[track_caller]` functions We want `#[track_caller]` to be semver independent, i.e. it should not be a breaking change to add or remove it. Since it changes ABI of a function (adding an additional argument) we have to be careful to preserve this property when adding tail calls. The only way to achieve this that I can see is: - we forbid tail calls in functions which are marked with `#[track_caller]` (already implemented) - tail-calling a `#[track_caller]` marked function downgrades the tail-call to a normal call (or equivalently tail-calls the shim made by fn def to fn ptr cast) (this pr) Ideally the downgrade would be performed by a MIR pass, but that requires post mono MIR opts (cc ```@saethlin,``` rust-lang#131650). For now I've changed code in cg_ssa to accomodate this behaviour (+ added a hack to mono collector so that the shim is actually generated) Additionally I added a lint, although I don't think it's strictly necessary. Alternative to rust-lang#144762 (and thus closes rust-lang#144762) Fixes rust-lang#144755
Rollup merge of #144865 - WaffleLapkin:track-tail, r=lqd Fix tail calls to `#[track_caller]` functions We want `#[track_caller]` to be semver independent, i.e. it should not be a breaking change to add or remove it. Since it changes ABI of a function (adding an additional argument) we have to be careful to preserve this property when adding tail calls. The only way to achieve this that I can see is: - we forbid tail calls in functions which are marked with `#[track_caller]` (already implemented) - tail-calling a `#[track_caller]` marked function downgrades the tail-call to a normal call (or equivalently tail-calls the shim made by fn def to fn ptr cast) (this pr) Ideally the downgrade would be performed by a MIR pass, but that requires post mono MIR opts (cc ```@saethlin,``` #131650). For now I've changed code in cg_ssa to accomodate this behaviour (+ added a hack to mono collector so that the shim is actually generated) Additionally I added a lint, although I don't think it's strictly necessary. Alternative to #144762 (and thus closes #144762) Fixes #144755
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