[IR] Test the "zeroize-stack" unrecognized-mode inlining rule - #18
Conversation
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.
|
|
|
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, |
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.
|
@claude, review this PR changes |
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.
Requested by Akshay Kumar · Slack thread
What this is
Fixes from an independent review of #17, which merged into
enforced_secrecy_mainasfb0e33f75before the review findings were addressed. This is the follow-up, branched off that merge commit.No behaviour change. Two comment lines are rewrapped, the comment at the comparison site in
Attributes.cppgains three lines on what a mode value this consumer cannot interpret means, and the rest is test coverage for rules #17 already implements but left unexercised.The blocking findings
1. Two lines over the 80-column limit
.clang-formatsets.llvm/include/llvm/IR/Attributes.h:1403andllvm/lib/IR/Attributes.cpp:2613, both 81 characters, both comment prose. Rewrapped withgit clang-format; that produced exactly these two hunks and nothing else. No line added to either C++ file exceeds 80 columns, and the two rewrapped lines are now 75 and 77. The lit test file does contain added lines over 80 — thirteen of them, 81 to 138 columns: the new-pass-remarks-missedRUN line, fivedefinelines carrying long mode strings, and sevenREMARK:check lines. No added line of comment prose runs over 80 in either kind of file. That is acceptable because the 80-column limit in.clang-formatis a C/C++ convention LLVM does not apply to.lllit tests, and the base version of that same test file already carries six lines over 80, up to 86 columns.2. A normative LangRef rule with zero test coverage.
llvm/docs/LangRef.mdsays 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.checkZeroizeStackrests on exactly that: it names only"sensitive"and lets every other value mean the widest mode. But every mode string in the tests was"used"or"sensitive", so the sentence had no coverage, and a mutation reading an unparseable mode as clearing the least still passed.One of the three new cases asserts the direction the documentation forbids; the other two stop that assertion from being read more broadly than it is. Only the refusal into a
sensitivecaller pins that an unrecognized mode is read as clearing the most:sensitiveusedcallee, because the unrecognized mode clears the mostsensitiveThe first row is the one that pins the direction, and it is the only one that does. The reading that looks conservative — treat a mode you cannot parse as clearing the least — is the reading LangRef rules out, and it would inline there; that row now fails if anyone takes it. The two rows below it still pass under that reading, and they earn their place differently: they pin the refusal above as the widest-mode reading rather than as a blanket refusal of any value the code cannot parse.
The same code path takes an empty value, and a valueless attribute, and neither was tested. The comparison in
checkZeroizeStacktests!= "sensitive", and an empty string is just another non-"sensitive"string, so"zeroize-stack"=""is read as the widest mode by the identical path that handles an unrecognized one. Nothing anywhere in the tree gave the attribute an empty value in a test — confirmed by grep before adding these.The two spellings are the same attribute once parsed, and the valueless one is the form that survives.
Attribute::getAsStringemits="value"only when the value is non-empty, so"zeroize-stack"=""prints as the bare"zeroize-stack"and the two collapse into a single attribute group:So the spelling carrying the
=is the one that never survives a round trip, while the valueless form is what-Sand bitcode actually emit. Both are covered. Four cases pin the reading:""sensitiveusedor an unrecognized-mode calleesensitive"""""""zeroize-stack"with no valuesensitiveBoth refusals join the exhaustive
-pass-remarks-missedlist, so each is pinned to the reason the inliner reports rather than merely to the call surviving. As in the unrecognized-mode table, the refusals into asensitivecaller are the discriminating rows: rows two and three still pass if an empty value is read as the narrowest mode, and they are there to pin the refusals as the widest-mode reading rather than as a blanket refusal of the value.No value means
"used", and that is the intended semantics rather than a gap. An absent value, an empty value, and a value this version of LLVM does not recognize all mean"used", the widest mode. A value this consumer cannot interpret therefore clears more of the frame than it must, never less, so a producer naming a mode a consumer has not learned loses no protection. #5 is where that is written down: it makes the LangRef value optional and says the attribute with no value means"used". #5 is not merged, and nothing here implements or duplicates it — this PR only pins what the inliner does with such a value, so that changing it has to be a decision rather than an accident. That reasoning is recorded in the test comments and in three lines at the comparison site, not only in the commit message, because a commit message can be lost when a patch is applied by hand.The should-fix findings
3.
alwaysinlinewas covered in one direction only. The singlealwaysinlinecase was a refusal, and a refusal on its own is indistinguishable from the flat "refuse every caller" rule #17 replaced: restoring that flat refusal inside the earlyalwaysinlineblock ingetAttributeBasedInliningDecisionstill passed. Added the permitted direction — analwaysinlineprotected callee into a protected caller whose mode clears at least as much — with CHECK lines pinning that the call is gone.4. The refusals never checked why they refused. They asserted only that the call survived, which would also hold with inlining switched off entirely, or with the call refused for some unrelated cause. And the failure string
InlineCost.cppreturns for this rule was exercised by nothing at all. A-pass-remarks-missed=inlinerun line now pins the reason for every refusal in the file, with--implicit-check-not="remark: "so the list is exhaustive in both directions: a refusal for the wrong reason fails, and an inline that quietly stops being refused fails too. Structured afterllvm/test/Transforms/Inline/always-inline-strictfp.llandno-inline-incompatible-gc.ll. Because that--implicit-check-notis exhaustive, a note sits beside the RUN line: any function added below which produces a missed-inline remark has to add itsREMARKline too, in source order. Without it a later author gets a failure attributed to the command line rather than to the case they added.Testing
No build or test check ran on this PR, or on #17. Every check reported on those commits is
skippedapart from agreeterbot job — the normal behaviour for this fork, where the upstream LLVM build workflows do not fire on these branches. Separately from those eight check-runs, alicense/clacommit status from CLAassistant ispendingbecause the bot account has not signed the CLA, and that pending status is why the mergeable state readsunstable; #17 carried the same pending status and merged anyway, so it is not known to block a merge here. The local run below is the only test evidence, so the tree hash is given for it.Built with assertions on, X86 only. Captured from the worktree lit ran in, at run time: tree
3aa1fb5502a75f3ba5c366c03a29695a29a04c15,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.zeroize-stack.llandzeroize-stack-lto.llboth confirmedPASSindividually rather than skipped, and each new case confirmed to actually run rather than reportUNSUPPORTED.Negative controls
A test that passes without the code under it is worthless, so each new assertion was broken deliberately, confirmed failing, restored, and confirmed passing again:
"sensitive"comparison on the callee side ofcheckZeroizeStackwithgetValueAsString() != "used". All three refusals into asensitivecaller inline, failing atzeroize-stack.ll:177,:234and:286. Every other row in those three groups still passes, and so does the pre-existingused-into-sensitivecase, which is precisely why this mutation was invisible before."used"and"sensitive"onto"sensitive"insidecheckZeroizeStack. Exactly the same three lines fail,:177,:234and:286, and nothing else does. That is the measurement behind the claim above that only the refusal row in each group pins the direction.alwaysinlineblock. Replaced theisZeroizeStackInlineCompatiblecall withCallee->hasFnAttribute("zeroize-stack"). The new permitted case fails atzeroize-stack.ll:355; the old refusal case still passes.zeroize-stack.ll:341while every IR-level assertion still passes.Each mutation was reverted and the file confirmed passing again, and both full suites were re-run green against the committed tree afterwards.
Where the
strictfpprecedent stops#17's description said this change mirrors the
strictfpprecedent "exactly". It does not, and the difference is worth recording rather than leaving to be rediscovered.strictfpis enforced in three places; the merged change covers two.The third is MLIR's LLVM-dialect inliner,
mlir/lib/Dialect/LLVMIR/Transforms/InlinerInterfaceImpl.cpp, which keeps its own hardcoded list of disallowed function attributes —disallowedFunctionAttrs, currentlynoduplicate,presplitcoroutine,returns_twice,strictfp— and"zeroize-stack"is absent from it. It cannot be added mechanically: that scan walksfuncOp.getPassthrough()and only inspects entries wheredyn_cast<StringAttr>succeeds, so it handles valueless string attributes only, while this attribute carries a value and is therefore not a bareStringAttr. Catching it needs a differently shaped check, not another name in the set.Three further paths reach
InlineFunctionwithout consulting inline legality at all:llvm/lib/Transforms/Coroutines/CoroAnnotationElide.cppcallsInlineFunctiondirectly, with no legality consult.ReplayInlineAdvisor(-cgscc-inline-replay=) hands backInlineCost::getAlways("previously inlined")straight from the replay file.-skip-partial-inlining-cost-analysisbypasses the partial inliner's cost path.All of these bypass
strictfpidentically today. None is introduced here and none is made worse here. They are named so they get tracked rather than rediscovered later by whoever wonders why a protected function was folded in anyway.The machine outliner concern, stated precisely
Not that
AttributeFuncs::areOutlineCompatibleis consulted and comes out wrong. It is consulted nowhere: it has no callers anywhere in the tree, so it is dead API and a latent trap for whoever wires it up, nothing more.The actual concern is
MachineOutliner.MachineOutliner::createOutlinedFunctiongives the function it creates onlyoptsizeandminsizeand copies nothing else from the source, so a region lifted out of a"zeroize-stack"function lands in an outlined function carrying no such attribute and no obligation to clear anything. That is a lowering problem and it belongs to the unlanded lowering work, not to the inliner.Still open from #17's review, unchanged here
These were flagged on #17 as things for a reviewer to decide rather than inherit. They remain open; this PR does not settle any of them.
"zeroize-stack", the only function-level marking in the tree, while the object-level marking is spelled negatively —MD_nozeroize/"nozeroize", despite [IR] Add !sensitive metadata for sensitive stack objects #3's title saying!sensitive.Stack context
All ten downstream branches (
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 do not containfb0e33f75and need rebasing onto it. This PR touches only comment wrapping and one test file, so it is not in the way of that rebase either way.AI tool use
This change was written with Claude Code. It was reviewed before it was submitted.
Generated by Claude Code