[IR] Inline a "zeroize-stack" callee into an equally protected caller - #17
Merged
kumarak merged 1 commit intoAug 18, 2026
Merged
Conversation
The attribute's inlining rule as implemented refuses every caller, including one that carries the attribute itself, while the LangRef text that landed alongside it documents the opposite: the callee is inlined into a caller that carries the attribute and whose mode clears at least as much of the frame. Make the compiler do what the documentation says. checkZeroizeStack now reads both sides. A callee without the attribute promises nothing and goes anywhere. A callee carrying it needs a caller carrying it too, or there is no clear for the callee's frame bytes to be folded into. Where both carry it, the caller must not ask for less of its frame than the callee did: "used" is the widest mode and LangRef gives any unrecognized value that same widest meaning, so "sensitive" is the only mode that clears less, and the only value the comparison needs to name. A request to inline does not override the rule, so the check has to run on the path a request takes. getAttributeBasedInliningDecision returns for an alwaysinline call site before it consults the attribute compatibility table, so expose the predicate as AttributeFuncs::isZeroizeStackInlineCompatible and call it inside that early block, exactly as checkStrictFP is exposed and called there. The always-inliner pass reaches the same function, so the one call covers both inliners. The mode-strictness half of the rule, that the caller clear at least as much as the callee, was not settled in discussion. It arrived with the LangRef text as the only reading that preserves what the callee promised. The caller test reads the positive function-level attribute "zeroize-stack" and compares its value against the literal "sensitive". Which marking should answer whether a caller is protected is not settled either. That attribute is the only function-level marking in the tree, so it is the only determinate choice, but the object-level marking beside it is spelled in the negative sense, as nozeroize, while the rule was discussed in the positive one. No warning is emitted when the refusal fires. LangRef allows one and it is worth having, but it needs a flag name and a place to live, and the check runs per call site inside a cost function, so a diagnostic would repeat for the same function pair until it is deduplicated. Left for a later change.
|
|
|
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, |
kumarak
marked this pull request as ready for review
August 18, 2026 19:02
kumarak
approved these changes
Aug 18, 2026
kumarak
pushed a commit
that referenced
this pull request
Aug 18, 2026
* [IR] Test the "zeroize-stack" unrecognized-mode inlining rule Fixes findings from an independent review of the now-merged #17. LangRef says any "zeroize-stack" mode other than "used" and "sensitive" is treated as "used", and that an unrecognized mode must not clear less than a recognized one. checkZeroizeStack rests on that sentence: it names only "sensitive" and lets every other value mean the widest mode. Nothing tested it. Every mode string in the tests was "used" or "sensitive", so a mutation that read an unparseable mode as clearing the least still passed. Three cases pin the direction the documentation forbids: an unrecognized-mode callee is refused into a "sensitive" caller exactly as a "used" callee is, an unrecognized-mode caller accepts a "sensitive" callee, and two different unrecognized spellings inline into each other because both denote the widest mode and unequal strings are not a mismatch. The alwaysinline path was covered in one direction only. Its single case was a refusal, which is indistinguishable from the flat refusal the rule replaced, so restoring that flat refusal inside the early alwaysinline block still passed. Add the permitted direction: an alwaysinline protected callee into a protected caller whose mode clears at least as much, with the call gone. The refusals asserted only that the call survived, which would also hold with inlining switched off or with the call refused for an unrelated cause. A -pass-remarks-missed=inline run line now pins the reason for each one, with --implicit-check-not so the list is exhaustive. The alwaysinline refusal pins the string InlineCost.cpp returns for it, which nothing exercised before. Rewrap two comment lines that ran to 81 columns. * [IR] Test the empty "zeroize-stack" attribute value Fixes a finding from an independent review of #18. checkZeroizeStack names only "sensitive" and lets every other value mean the widest mode. An empty value is one of those other values: it is not equal to "sensitive", so it reaches the inlining decision through the identical comparison an unrecognized value reaches it through, and is silently treated as clearing the whole frame. Nothing in the tree gave the attribute an empty value in a test, so that path was untested in both directions. Three cases pin it. An empty-valued callee is refused into a "sensitive" caller, exactly as a "used" or an unrecognized-mode callee is. An empty-valued caller accepts a "sensitive" callee, which asked for less, so the refusal is pinned as the widest mode rather than as a blanket refusal of the empty value. Empty into empty inlines, because two equally non-modal strings are not a mismatch. The new refusal joins the exhaustive -pass-remarks-missed list, so it is pinned to the reason the inliner reports rather than merely to the call surviving. Reading the empty value as the widest mode is the conservative direction: it can only cost an inline, never buy one into a caller that clears less. It is not an endorsement of writing it. LangRef documents the value as required and nothing in the tree enforces that yet, with enforcement pending in a separate change; these cases assert today's behaviour so that changing it, whether by rejecting the empty value outright or by giving it a meaning of its own, is a decision rather than an accident. Record that reasoning in a comment at the comparison site and at the test cases, so it does not depend on this message surviving. * [IR][Inline] Cover the valueless "zeroize-stack" spelling The empty and the valueless spellings are the same attribute once parsed: Attribute::getAsString prints ="value" only for a non-empty value, so "zeroize-stack"="" prints as the bare "zeroize-stack" and both land in one attribute group. The valueless form is therefore what -S and bitcode emit, so cover it as a refusal into a "sensitive" caller alongside the empty one. Correct the comments that described the empty value as unenforced with enforcement to come. An absent, an empty, and an unrecognized value all mean "used", the widest mode, so a value this consumer cannot interpret clears more of the frame than it must, never less. Correct the claim that all three rows per mode-value group pin that direction. Only the refusal into a "sensitive" caller does; the other two pin that refusal as the widest-mode reading rather than as a blanket refusal of the value. Note next to the remarks RUN line that its exhaustive --implicit-check-not makes a new missed-inline remark a failure unless its REMARK line is added too, and trim the comparison-site comment, which had grown out of proportion to the code it explains. --------- Co-authored-by: Claude <noreply@anthropic.com>
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
3 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 Akshay Kumar · Slack thread
Before / After
Before. The base branch documents one inlining rule and implements a different one.
The squash of #4 carried in two things at once. Its code change made the rule callee-side with no same-attribute exemption —
checkZeroizeStackinllvm/lib/IR/Attributes.cppended inreturn !Callee.hasFnAttribute("zeroize-stack");, ignoring the caller entirely, so a function carrying"zeroize-stack"is refused to every caller. Its LangRef text, which touches no code, says the opposite: a protected callee "is inlined only into a caller that carries it as well, and only where the caller's mode clears at least as much of the frame", with"used"the widest mode and"sensitive"the only narrower one. The commit message states the callee-side rule; the documentation in the same commit states the exemption. Right now the compiler does not do what the merged documentation says it does.After. The compiler does what that documentation says. A protected callee is inlined into a protected caller whose mode clears at least as much, and refused otherwise — including when something asks for the inline anyway.
What it does
Seven cases, all covered by tests:
usedusedsensitivesensitivesensitiveusedusedsensitivealwaysinlinecallee +alwaysinlinecall siteThe last one is new — nothing exercised
alwaysinlinebefore.How
checkZeroizeStacknow reads both sides: no callee attribute means no promise and it goes anywhere; otherwise the caller must carry the attribute; and where both carry it the caller must not ask for less."used"is the widest mode and LangRef gives any unrecognized value that same widest meaning, so"sensitive"is the only value the comparison has to name.Attributes.tdneeds no change — the rule is already registered as aCompatRule, so the rewritten body reaches every consumer ofAttributeFuncs::areInlineCompatible, LTO and ThinLTO included.The compatibility table alone is not enough, because
getAttributeBasedInliningDecisioninllvm/lib/Analysis/InlineCost.cppreturns for analwaysinlinecall site before it consults that table. This follows thestrictfpprecedent exactly:AttributeFuncs::isStrictFPInlineCompatibleexists socheckStrictFPcan be called inside that early block, so the zeroize predicate is exposed the same way and called on the next line.AlwaysInliner.cpproutes through the same function, so one call covers both inliners. Confirmed load-bearing: with that one call disabled, thealwaysinlinecase inlines and the test fails.Three things a reviewer should decide, not inherit
1. Mode strictness was not settled in the meeting. The requirement that the caller clear at least as much as the callee did not come from the discussion. It arrived with the LangRef text that rode in on the squash, as the only reading that preserves what the callee promised. It is implemented here because the merged documentation now states it, not because it was agreed.
2. Which marking the caller test reads is unanswered, and the spelling is inverted. This check reads the positive function-level attribute
"zeroize-stack"and compares its value against the literal string"sensitive". That is the only determinate choice available: it is the only function-level marking in the tree, and the object-level marking cannot answer "is this caller protected" because it is attached peralloca. But note the inversion — the discussion used the positive sense throughout, while the object-level marking's IR spelling is negative: #3 is titled "Add!sensitivemetadata for sensitive stack objects" and what it actually landed isMD_nozeroize/"nozeroize"inFixedMetadataKinds.def, tested asnozeroize-metadata.ll. So "the marking" is genuinely ambiguous rather than merely unstated. Flagging the choice rather than leaving it to be inferred from the diff; correct it if the intended sense is the other one.3. The opt-in warning is deliberately not here. LangRef says an implementation "may report a warning when it declines to inline for this reason, and should not report an error", and that warning is worth having — but it needs a flag name, which is still an open question, and it needs a place to live. There is also a design constraint worth recording: this compatibility check runs per call site, inside a cost function, so a naive diagnostic would fire repeatedly for the same caller/callee pair and would need deduplicating before it is usable. That affects whether the flag is worth having at all, which is why it belongs here rather than only in chat. No flag has been invented.
Do not merge this yet
zeroize-verifier,zeroize-lowering,zeroize-capabilities,zeroize-exit-coverage,zeroize-ordering,zeroize-per-exit-regs,zeroize-tailcall,zeroize-fallback,zeroize-scratch-regs,zeroize-machine-dce) still need rebasing onto the squash of [IR] Do not inline functions carrying "zeroize-stack" #4.Opened as a draft, as a follow-up on top of the merged #4 rather than an amendment to it. Branched off
enforced_secrecy_mainat1f24967c3d83a3f60d5629c7bae4aedb0fed2e61; #4's branch was deleted after the squash, so there is no #4 head to stack on.Testing
Built with assertions on, X86 only. Worktree at test time: tree
33e550709ce29d16efe2391d7cfa3651ab797611,git status --porcelainempty.ninja check-llvm-transforms-inline— 355 discovered, 289 passed, 0 failed, 65 unsupported, 1 expectedly failed.ninja check-llvm-verifier— 416 discovered, 393 passed, 0 failed, 23 unsupported.PASSindividually rather than skipped.AI tool use
This change was written with Claude Code. It was reviewed before it was submitted.
Generated by Claude Code