Skip to content

[IR] Test the "zeroize-stack" unrecognized-mode inlining rule - #18

Merged
kumarak merged 3 commits into
enforced_secrecy_mainfrom
zeroize-inline-review-fixes
Aug 18, 2026
Merged

[IR] Test the "zeroize-stack" unrecognized-mode inlining rule#18
kumarak merged 3 commits into
enforced_secrecy_mainfrom
zeroize-inline-review-fixes

Conversation

@claude

@claude claude Bot commented Aug 18, 2026

Copy link
Copy Markdown

Requested by Akshay Kumar · Slack thread

What this is

Fixes from an independent review of #17, which merged into enforced_secrecy_main as fb0e33f75 before 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.cpp gains 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-format sets. llvm/include/llvm/IR/Attributes.h:1403 and llvm/lib/IR/Attributes.cpp:2613, both 81 characters, both comment prose. Rewrapped with git 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-missed RUN line, five define lines carrying long mode strings, and seven REMARK: 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-format is a C/C++ convention LLVM does not apply to .ll lit 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.md 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 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 sensitive caller pins that an unrecognized mode is read as clearing the most:

callee caller result
unrecognized mode sensitive refused — same as a used callee, because the unrecognized mode clears the most
sensitive unrecognized mode inlined — the caller clears its whole frame
unrecognized mode unrecognized mode inlined — two unequal spellings both denote the widest mode, so they are not a mismatch

The 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 checkZeroizeStack tests != "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::getAsString emits ="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:

$ cat rt.ll
define void @empty_value() "zeroize-stack"="" { ret void }
define void @no_value()    "zeroize-stack"     { ret void }
$ llvm-as < rt.ll | llvm-dis | tail -1
attributes #0 = { "zeroize-stack" }

So the spelling carrying the = is the one that never survives a round trip, while the valueless form is what -S and bitcode actually emit. Both are covered. Four cases pin the reading:

callee caller result
"" sensitive refused — same as a used or an unrecognized-mode callee
sensitive "" inlined — the caller clears its whole frame
"" "" inlined — both denote the widest mode, so they are not a mismatch
"zeroize-stack" with no value sensitive refused — the form the empty value prints and reloads as, so the form a module actually reaching the inliner carries

Both refusals join the exhaustive -pass-remarks-missed list, 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 a sensitive caller 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. alwaysinline was covered in one direction only. The single alwaysinline case 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 early alwaysinline block in getAttributeBasedInliningDecision still passed. Added the permitted direction — an alwaysinline protected 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.cpp returns for this rule was exercised by nothing at all. A -pass-remarks-missed=inline run 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 after llvm/test/Transforms/Inline/always-inline-strictfp.ll and no-inline-incompatible-gc.ll. Because that --implicit-check-not is exhaustive, a note sits beside the RUN line: any function added below which produces a missed-inline remark has to add its REMARK line 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 skipped apart from a greeter bot 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, a license/cla commit status from CLAassistant is pending because the bot account has not signed the CLA, and that pending status is why the mergeable state reads unstable; #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 --porcelain empty.

  • 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.ll and zeroize-stack-lto.ll both confirmed PASS individually rather than skipped, and each new case confirmed to actually run rather than report UNSUPPORTED.

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:

  • Unrecognized, empty, and valueless modes read as clearing the least. Replaced the "sensitive" comparison on the callee side of checkZeroizeStack with getValueAsString() != "used". All three refusals into a sensitive caller inline, failing at zeroize-stack.ll:177, :234 and :286. Every other row in those three groups still passes, and so does the pre-existing used-into-sensitive case, which is precisely why this mutation was invisible before.
  • Unrecognized and empty modes mapped onto the narrowest mode. Mapped every value other than "used" and "sensitive" onto "sensitive" inside checkZeroizeStack. Exactly the same three lines fail, :177, :234 and :286, and nothing else does. That is the measurement behind the claim above that only the refusal row in each group pins the direction.
  • Flat refusal restored in the alwaysinline block. Replaced the isZeroizeStackInlineCompatible call with Callee->hasFnAttribute("zeroize-stack"). The new permitted case fails at zeroize-stack.ll:355; the old refusal case still passes.
  • Refusal reason changed. Changed the failure string returned at that refusal. The remarks check fails at zeroize-stack.ll:341 while 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 strictfp precedent stops

#17's description said this change mirrors the strictfp precedent "exactly". It does not, and the difference is worth recording rather than leaving to be rediscovered. strictfp is 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, currently noduplicate, presplitcoroutine, returns_twice, strictfp — and "zeroize-stack" is absent from it. It cannot be added mechanically: that scan walks funcOp.getPassthrough() and only inspects entries where dyn_cast<StringAttr> succeeds, so it handles valueless string attributes only, while this attribute carries a value and is therefore not a bare StringAttr. Catching it needs a differently shaped check, not another name in the set.

Three further paths reach InlineFunction without consulting inline legality at all:

  • llvm/lib/Transforms/Coroutines/CoroAnnotationElide.cpp calls InlineFunction directly, with no legality consult.
  • ReplayInlineAdvisor (-cgscc-inline-replay=) hands back InlineCost::getAlways("previously inlined") straight from the replay file.
  • -skip-partial-inlining-cost-analysis bypasses the partial inliner's cost path.

All of these bypass strictfp identically 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::areOutlineCompatible is 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::createOutlinedFunction gives the function it creates only optsize and minsize and 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.

  1. Mode strictness was not settled in the meeting. The requirement that the caller clear at least as much as the callee arrived with the LangRef text on the squash of [IR] Do not inline functions carrying "zeroize-stack" #4, as the only reading that preserves what the callee promised, not from the discussion.
  2. Which marking the caller test reads is unanswered, and the spelling is inverted. The check reads the positive function-level attribute "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.
  3. The opt-in warning is deliberately absent. LangRef permits one and it is worth having, but it needs a flag name and a home, and the check runs per call site inside a cost function, so a naive diagnostic would repeat for the same function pair until deduplicated.

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 contain fb0e33f75 and 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

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.
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions

Copy link
Copy Markdown

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.

  • All contributions to LLVM must follow our LLVM AI Tool Use Policy. In particular, if you used AI while working on this PR, remember to add a note to the PR description.
  • The LLVM Code-Review Policy and Practices document contains practical information about the PR process, including how patches are reviewed and accepted, and who can review a PR.
  • Our LLVM Developer Policy describes our expectations for code quality, commit summaries and contains notes on our CI system.

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 questions

How 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 @ followed by their GitHub username.

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,
The LLVM Community

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.
@kumarak

kumarak commented Aug 18, 2026

Copy link
Copy Markdown
Member

@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.
@kumarak
kumarak merged commit f4d9ab6 into enforced_secrecy_main Aug 18, 2026
7 of 8 checks passed
@kumarak
kumarak deleted the zeroize-inline-review-fixes branch August 18, 2026 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants