[CodeGen] Fall back to clearing more when the analysis is incomplete - #12
[CodeGen] Fall back to clearing more when the analysis is incomplete#12claude[bot] wants to merge 1 commit into
Conversation
|
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, |
78bf8ec to
537628e
Compare
4116012 to
3d68722
Compare
|
|
|
A full The test covers the already-shipped Build: Generated by Claude Code |
537628e to
3a8b2b4
Compare
Every narrowing the clearing machinery does is an optimization over a guarantee: the guarantee is that nothing the function held survives it, and the narrowings exist so that discharging it costs less. A narrowing has to be able to say why each thing it drops is safe to drop. Where it cannot, the answer has to be to clear more, never less, because the two errors are not symmetric: clearing something that did not need it costs instructions, and clearing nothing that did costs the thing the attribute exists to protect. This is the same one-directional rule "zeroize-stack"="sensitive" already follows, where losing metadata widens the cleared set. Three places in the code as it stands resolved an incomplete analysis the wrong way, or resolved it not at all. Each is fixed here, and each is grounded in a path through the code rather than in a hypothetical. The one that leaked is the register set. A "used" mode of "zero-call-used-regs" clears the registers the function touched, and the sweep that computes them skipped implicit operands. An implicit operand is how the machine layer records a register an instruction touches without naming it, so the effect was that every such register was treated as untouched and left alone. Inline assembly is the worst case, because every register an asm block names -- its clobber list and its physical-register outputs alike -- arrives as an implicit operand of the INLINEASM instruction. A function whose register traffic was an asm block therefore cleared nothing at all under "used-gpr", and the asm's registers carried their contents past the return. It is not only inline assembly: rdtsc leaves a counter in %eax and %edx and names neither, and any pseudo that defines a register on the side reads the same way. Implicit operands now count. This widens what a "used" mode clears; it does not collapse it into "all", and a function that touches no call-used register still clears none. The second is the mode itself. The switch over the value of "zero-call-used-regs" had no default, so a value that is not one of the nine names ran off the end of a StringSwitch: an assertion where builds have them, and in a release compiler an uninitialized mode deciding what gets cleared. An unrecognized mode now means "all", the widest. The modes are a scale, an unknown name says nothing about where on it the producer meant to be, and "all" is the only reading that cannot clear less than was asked for. It is also the reading LangRef already fixes for an unrecognized "zeroize-stack" mode, so the two attributes now agree. "skip" is untouched: it is a name on the scale, not a failure to read one. The third is the exit classification. A block with no successors whose last instruction matched none of the recognized shapes was reported as unreachable, which is out of scope -- that is, the classification answered "control stops here" whenever it did not recognize what it was looking at. Not recognizing an instruction is not the same as knowing what it does. Inline assembly can jump, can issue a system call that does not come back, and can return into another frame, and nothing here can establish that it does not. Those blocks are now a kind of their own, Unknown, which is in scope, and the sequence is emitted in front of the opaque instruction. Unreachable keeps the cases that can be settled positively rather than merely not ruled out: a block with no instructions left in it, and an instruction the target has marked as a trap. A call that does not return keeps its own kind and its own reason, which is about an abandoned frame rather than about not knowing. At an Unknown exit the sequence spares the registers the opaque instruction declares, the same as at every other exit, because a sequence that breaks the instruction the exit leaves through is not an option. What it clears is what the rest of the function used, which is what would otherwise leave with the frame. The two halves fit together: those same asm-declared registers are now cleared at the function's ordinary returns, by the first change above. One more path is made fail-closed without being reachable today. An in-scope exit that the sequence could not be placed at was skipped with an assertion, which in a release compiler is a silent skip: a function that reports itself protected, leaves through a point at which nothing ran, and says nothing about it. It now diagnoses instead. No IR reaches it -- every in-scope exit has an insertion point by construction -- so it carries no test; it is here so that a later change which introduces one is stopped rather than absorbed. Three cases were checked and found already conservative, and are left alone rather than given redundant code. A target that cannot clear registers or the frame is asked before anything is computed and its refusal is reported as an error, so an unimplemented capability fails closed. An unrecognized "zeroize-stack" mode is fixed by LangRef as "used", the widest, and no code consumes the mode yet, so there is nothing to make conservative. A protected function's tail calls are suppressed and musttail rejected, which is the same rule applied to an exit that cannot be cleared at all. No existing test changes. CodeGen/X86, CodeGen/ARM and the whole of test/CodeGen pass unchanged, as do the tests this stack has added. The new tests pin the direction in each case: an asm clobber and an rdtsc cleared under "used-gpr" where they were not, against a function that touches nothing and still clears nothing; an unreadable mode and an empty one producing what "all" produces, against "used-gpr" and "skip" still meaning what they say; and an asm-terminated block reported in scope and cleared in front of, against a trap and an empty block still reported out of scope and left alone. The classification and the mode fallback are pinned on ARM as well, where they are decided before the target is consulted, and where the widened mode reaches that target's refusal rather than resolving quietly to clearing nothing. Each test was confirmed load-bearing by breaking the implementation once and restoring it: restoring the implicit-operand skip failed the register test, defaulting the mode to "skip" failed the mode test on both targets, and making Unknown out of scope failed the exit tests on both targets. "zero-call-used-regs" has no LangRef entry to record the mode rule in; adding one is separate work. This is trailofbits/vspells-ct-internal-notes#24, under the umbrella trailofbits/vspells-ct-internal-notes#17.
3d68722 to
abdc7d6
Compare
2 participants
Requested by Francesco Bertolaccini · Slack thread
Every narrowing the clearing machinery does is an optimization over a guarantee:
the guarantee is that nothing the function held survives it, and the narrowings
exist so that discharging it costs less. A narrowing has to be able to say why
each thing it drops is safe to drop. Where it cannot, the answer has to be to
clear more, never less, because the two errors are not symmetric: clearing
something that did not need it costs instructions, and clearing nothing that
did costs the thing the attribute exists to protect. This is the same
one-directional rule "zeroize-stack"="sensitive" already follows, where losing
metadata widens the cleared set.
Three places in the code as it stands resolved an incomplete analysis the wrong
way, or resolved it not at all. Each is fixed here, and each is grounded in a
path through the code rather than in a hypothetical.
The one that leaked is the register set. A "used" mode of "zero-call-used-regs"
clears the registers the function touched, and the sweep that computes them
skipped implicit operands. An implicit operand is how the machine layer records
a register an instruction touches without naming it, so the effect was that
every such register was treated as untouched and left alone. Inline assembly is
the worst case, because every register an asm block names -- its clobber list
and its physical-register outputs alike -- arrives as an implicit operand of the
INLINEASM instruction. A function whose register traffic was an asm block
therefore cleared nothing at all under "used-gpr", and the asm's registers
carried their contents past the return. It is not only inline assembly: rdtsc
leaves a counter in %eax and %edx and names neither, and any pseudo that
defines a register on the side reads the same way. Implicit operands now count.
This widens what a "used" mode clears; it does not collapse it into "all", and
a function that touches no call-used register still clears none.
The second is the mode itself. The switch over the value of "zero-call-used-regs"
had no default, so a value that is not one of the nine names ran off the end of
a StringSwitch: an assertion where builds have them, and in a release compiler
an uninitialized mode deciding what gets cleared. An unrecognized mode now means
"all", the widest. The modes are a scale, an unknown name says nothing about
where on it the producer meant to be, and "all" is the only reading that cannot
clear less than was asked for. It is also the reading LangRef already fixes for
an unrecognized "zeroize-stack" mode, so the two attributes now agree. "skip" is
untouched: it is a name on the scale, not a failure to read one.
The third is the exit classification. A block with no successors whose last
instruction matched none of the recognized shapes was reported as unreachable,
which is out of scope -- that is, the classification answered "control stops
here" whenever it did not recognize what it was looking at. Not recognizing an
instruction is not the same as knowing what it does. Inline assembly can jump,
can issue a system call that does not come back, and can return into another
frame, and nothing here can establish that it does not. Those blocks are now a
kind of their own, Unknown, which is in scope, and the sequence is emitted in
front of the opaque instruction. Unreachable keeps the cases that can be settled
positively rather than merely not ruled out: a block with no instructions left
in it, and an instruction the target has marked as a trap. A call that does not
return keeps its own kind and its own reason, which is about an abandoned frame
rather than about not knowing.
At an Unknown exit the sequence spares the registers the opaque instruction
declares, the same as at every other exit, because a sequence that breaks the
instruction the exit leaves through is not an option. What it clears is what the
rest of the function used, which is what would otherwise leave with the frame.
The two halves fit together: those same asm-declared registers are now cleared
at the function's ordinary returns, by the first change above.
One more path is made fail-closed without being reachable today. An in-scope
exit that the sequence could not be placed at was skipped with an assertion,
which in a release compiler is a silent skip: a function that reports itself
protected, leaves through a point at which nothing ran, and says nothing about
it. It now diagnoses instead. No IR reaches it -- every in-scope exit has an
insertion point by construction -- so it carries no test; it is here so that a
later change which introduces one is stopped rather than absorbed.
Three cases were checked and found already conservative, and are left alone
rather than given redundant code. A target that cannot clear registers or the
frame is asked before anything is computed and its refusal is reported as an
error, so an unimplemented capability fails closed. An unrecognized
"zeroize-stack" mode is fixed by LangRef as "used", the widest, and no code
consumes the mode yet, so there is nothing to make conservative. A protected
function's tail calls are suppressed and musttail rejected, which is the same
rule applied to an exit that cannot be cleared at all.
No existing test changes. CodeGen/X86, CodeGen/ARM and the whole of test/CodeGen
pass unchanged, as do the tests this stack has added. The new tests pin the
direction in each case: an asm clobber and an rdtsc cleared under "used-gpr"
where they were not, against a function that touches nothing and still clears
nothing; an unreadable mode and an empty one producing what "all" produces,
against "used-gpr" and "skip" still meaning what they say; and an asm-terminated
block reported in scope and cleared in front of, against a trap and an empty
block still reported out of scope and left alone. The classification and the
mode fallback are pinned on ARM as well, where they are decided before the
target is consulted, and where the widened mode reaches that target's refusal
rather than resolving quietly to clearing nothing. Each test was confirmed
load-bearing by breaking the implementation once and restoring it: restoring the
implicit-operand skip failed the register test, defaulting the mode to "skip"
failed the mode test on both targets, and making Unknown out of scope failed the
exit tests on both targets.
"zero-call-used-regs" has no LangRef entry to record the mode rule in; adding
one is separate work.
This is trailofbits/vspells-ct-internal-notes#24, 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