[CodeGen] Suppress tail calls in protected functions - #11
Draft
claude[bot] wants to merge 1 commit into
Draft
Conversation
A tail call replaces the caller's frame with the callee's and jumps, so control never comes back to the caller. A function carrying "zeroize-stack" has undertaken to clear its frame before it returns, and a tail call takes away the point at which it would do that: the frame stays live underneath the callee, and the function reports itself protected while leaving in memory exactly what the attribute exists to destroy. Tail-call optimization is suppressed in a protected function. This is decision DD8, which the design records as settled. The suppression goes where LLVM already decides tail-call eligibility rather than into a check of its own. isInTailCallPosition in CodeGen/Analysis.cpp is the target-independent answer to that question, and everything that forms a tail call out of a call in the IR reaches it: SelectionDAGBuilder through canTailCall, FastISel, and GlobalISel's CallLowering. Those are the same three places that each honor "disable-tail-calls" separately, which is what asking once here avoids. Folding a memcpy, memmove or memset into a tail call to the library routine goes through it too, and replaces the frame just as thoroughly. A libcall the legalizer generates has no call in the IR behind it and never reaches that function. It is asked separately, by the SDNode overload of TargetLowering::isInTailCallPosition, which carries its own "disable-tail-calls" check for the same reason, and the second half of the change sits next to it. The path is not hypothetical: an frem in return position becomes a tail call to fmod on both x86 and ARM, and was the one remaining way a protected function still jumped away from its frame. musttail is diagnosed rather than suppressed. Declining an ordinary tail call is available because forming one is an optimization; musttail is a requirement the caller is not allowed to drop. A function that must be replaced at the call and must clear its frame after it is a function that cannot be generated, so the combination is rejected instead of being honored in one direction without saying so. The rejection is in the Verifier, in verifyMustTailCall, next to "cannot use musttail call with inline asm". That neighbor has the same shape: not malformed IR, but musttail combined with something that makes it impossible to honor, and the Verifier is where that shape already lives. It is also the layer at which the conflict is fully visible without a target. Leaving it to CodeGen would surface as the backend's existing "failed to perform tail call elimination on a call site marked musttail", which is fatal but never names the attribute that caused it, and which is reached per target and twice over for a call FastISel starts and SelectionDAG finishes. The frontend diagnostic for the same conflict written in source is separate work. Rejecting the combination in the Verifier makes it a bug for a pass to build one, and one pass did. MergeFunctions rewrites a merged function into a thunk that tail-calls the body, copies the attributes of the function it replaces onto that thunk, and uses musttail when both functions are swifttailcc, so two identical protected swifttailcc functions became a thunk carrying "zeroize-stack" around a musttail call, aborting the compilation with "Broken module found" from valid input. Protected functions are excluded from merging, which is the answer inlining already gives them: a thunk standing in for a protected function undertakes to clear a frame that no longer holds anything, and where the convention makes its call musttail it cannot discharge the undertaking at all. tailcc and -tailcallopt are covered as well. Both exist to guarantee the optimization rather than to permit it, and the guarantee is over the frame the attribute is about, so a protected function does not obtain it by choosing the convention. The cost is real: a protected tailcc function doing unbounded mutual recursion now grows the stack. Whether that should be rejected the way musttail is, rather than quietly losing the guarantee, is left to trailofbits/vspells-ct-internal-notes#22 rather than settled here. Where this meets the per-exit register clearing is worth stating, because the two can look like they overlap. Clearing at a tail-call exit spares the registers the callee is about to read as outgoing arguments, and a protected function no longer has a tail-call exit: the exit classification for one now reports both exits as returns where it used to report a tail call and a return. That path is unreachable for a protected function. It is not dead. Register clearing is driven by "zero-call-used-regs", a separate attribute that long predates this work and that functions carry without "zeroize-stack"; those functions still tail-call, and the per-exit set is still what makes their tail-call exits correct. The test covering that case carries only "zero-call-used-regs" and is untouched here. The two mechanisms answer for disjoint sets of functions rather than for the same one twice. No existing test changes, in CodeGen/X86, CodeGen/ARM, or anywhere under Transforms. Nothing in the tree combines "zeroize-stack" with a tail call, which is why the suppression arrives without an old test starting to expect less. The new tests are the contrast in both directions on both targets: a protected function that would otherwise jump does not, an unprotected one with the same body still does, and the same pair for a legalizer libcall; the exit classification changing from a tail call to a return; musttail in a protected function rejected under two modes with an unprotected musttail untouched; and the merged pair left unmerged. Each was confirmed load-bearing by breaking the implementation once and restoring it: dropping either half of the suppression failed the CodeGen tests at the corresponding check, dropping the Verifier check failed the musttail test, and dropping the merging exclusion failed the MergeFunc test. This is trailofbits/vspells-ct-internal-notes#22, under the umbrella trailofbits/vspells-ct-internal-notes#17.
|
|
|
Hello @claude[bot] 👋 Thank you for submitting a Pull Request (PR) to the LLVM Project. Since this is your first PR, here are a few useful links covering our main contribution policies and review practices.
Please reply to this message to confirm that you have read these policies, especially the LLVM AI Tool Use Policy, and that any AI tool usage has been noted in the PR description. Frequently asked questionsHow do I add reviewers? This PR will be automatically labeled, and the relevant teams will be notified. For some parts of the project, reviewers may also be added automatically. You can also add reviewers manually using the Reviewers section on this page. If you cannot use that section, it is probably because you do not have write permissions for the repository. In that case, you can request a review by tagging reviewers in a comment using What if there are no comments? If you have not received any comments on your PR after a week, you can request a review by pinging the PR with a comment such as “Ping”. The common courtesy ping rate is once a week. Please remember that you are asking for volunteer time from other developers. Are any special GitHub settings required to contribute to LLVM? We only require contributors to have a public email address associated with their GitHub commits, see this section of LLVM Developer Policy for details. If you have questions, feel free to leave a comment on this PR, or ask on LLVM Discord or LLVM Discourse. Thank you, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Requested by Francesco Bertolaccini · Slack thread
A tail call replaces the caller's frame with the callee's and jumps, so control
never comes back to the caller. A function carrying "zeroize-stack" has
undertaken to clear its frame before it returns, and a tail call takes away the
point at which it would do that: the frame stays live underneath the callee,
and the function reports itself protected while leaving in memory exactly what
the attribute exists to destroy. Tail-call optimization is suppressed in a
protected function. This is decision DD8, which the design records as settled.
The suppression goes where LLVM already decides tail-call eligibility rather
than into a check of its own. isInTailCallPosition in CodeGen/Analysis.cpp is
the target-independent answer to that question, and everything that forms a
tail call out of a call in the IR reaches it: SelectionDAGBuilder through
canTailCall, FastISel, and GlobalISel's CallLowering. Those are the same three
places that each honor "disable-tail-calls" separately, which is what asking
once here avoids. Folding a memcpy, memmove or memset into a tail call to the
library routine goes through it too, and replaces the frame just as thoroughly.
A libcall the legalizer generates has no call in the IR behind it and never
reaches that function. It is asked separately, by the SDNode overload of
TargetLowering::isInTailCallPosition, which carries its own "disable-tail-calls"
check for the same reason, and the second half of the change sits next to it.
The path is not hypothetical: an frem in return position becomes a tail call to
fmod on both x86 and ARM, and was the one remaining way a protected function
still jumped away from its frame.
musttail is diagnosed rather than suppressed. Declining an ordinary tail call
is available because forming one is an optimization; musttail is a requirement
the caller is not allowed to drop. A function that must be replaced at the call
and must clear its frame after it is a function that cannot be generated, so
the combination is rejected instead of being honored in one direction without
saying so.
The rejection is in the Verifier, in verifyMustTailCall, next to "cannot use
musttail call with inline asm". That neighbor has the same shape: not malformed
IR, but musttail combined with something that makes it impossible to honor, and
the Verifier is where that shape already lives. It is also the layer at which
the conflict is fully visible without a target. Leaving it to CodeGen would
surface as the backend's existing "failed to perform tail call elimination on a
call site marked musttail", which is fatal but never names the attribute that
caused it, and which is reached per target and twice over for a call FastISel
starts and SelectionDAG finishes. The frontend diagnostic for the same conflict
written in source is separate work.
Rejecting the combination in the Verifier makes it a bug for a pass to build
one, and one pass did. MergeFunctions rewrites a merged function into a thunk
that tail-calls the body, copies the attributes of the function it replaces
onto that thunk, and uses musttail when both functions are swifttailcc, so two
identical protected swifttailcc functions became a thunk carrying
"zeroize-stack" around a musttail call, aborting the compilation with "Broken
module found" from valid input. Protected functions are excluded from merging,
which is the answer inlining already gives them: a thunk standing in for a
protected function undertakes to clear a frame that no longer holds anything,
and where the convention makes its call musttail it cannot discharge the
undertaking at all.
tailcc and -tailcallopt are covered as well. Both exist to guarantee the
optimization rather than to permit it, and the guarantee is over the frame the
attribute is about, so a protected function does not obtain it by choosing the
convention. The cost is real: a protected tailcc function doing unbounded
mutual recursion now grows the stack. Whether that should be rejected the way
musttail is, rather than quietly losing the guarantee, is left to
trailofbits/vspells-ct-internal-notes#22 rather than settled here.
Where this meets the per-exit register clearing is worth stating, because the
two can look like they overlap. Clearing at a tail-call exit spares the
registers the callee is about to read as outgoing arguments, and a protected
function no longer has a tail-call exit: the exit classification for one now
reports both exits as returns where it used to report a tail call and a return.
That path is unreachable for a protected function. It is not dead. Register
clearing is driven by "zero-call-used-regs", a separate attribute that long
predates this work and that functions carry without "zeroize-stack"; those
functions still tail-call, and the per-exit set is still what makes their
tail-call exits correct. The test covering that case carries only
"zero-call-used-regs" and is untouched here. The two mechanisms answer for
disjoint sets of functions rather than for the same one twice.
No existing test changes, in CodeGen/X86, CodeGen/ARM, or anywhere under
Transforms. Nothing in the tree combines "zeroize-stack" with a tail call,
which is why the suppression arrives without an old test starting to expect
less. The new tests are the contrast in both directions on both targets: a
protected function that would otherwise jump does not, an unprotected one with
the same body still does, and the same pair for a legalizer libcall; the exit
classification changing from a tail call to a return; musttail in a protected
function rejected under two modes with an unprotected musttail untouched; and
the merged pair left unmerged. Each was confirmed load-bearing by breaking the
implementation once and restoring it: dropping either half of the suppression
failed the CodeGen tests at the corresponding check, dropping the Verifier
check failed the musttail test, and dropping the merging exclusion failed the
MergeFunc test.
This is trailofbits/vspells-ct-internal-notes#22, under the umbrella
trailofbits/vspells-ct-internal-notes#17.
AI tool use
This pull request contains AI-generated content. It was prepared with the assistance of Claude Code; the contributor has reviewed the generated code and text, is the author of the contribution, and is accountable for it, per the LLVM AI Tool Use Policy.
Generated by Claude Code