Repository navigation
Add a loop's dead temporaries and loop-body variables to an explicit loop_modifies - #5071
hung1962003 wants to merge 1 commit into
Conversation
|
Friendly ping: this is a small follow-up to #4947 (only the last commit is new). It unblocks unbounded proofs of the provided |
|
@hung1962003 can you rebase this PR? |
An explicit `#[kani::loop_modifies]` must cover every local that the loop writes. model-checking#4947 adds the loop-only locals that Kani generates, but MIR temporaries (e.g. the scrutinee of a `match` with `return` arms) and variables of the loop body that are still live when a `return` leaves the loop cannot be named by the user, and still fail with `Check that var_N is assignable`. Add to the write set the locals that are dead at the loop head where the write set is havocked (by a MIR liveness analysis), are not storage-local to the loop, and are temporaries or declared inside the loop's span, for loops that are not nested in another contracted loop. Emit `StorageLive` for them at the latch, so that havocking an address-taken temporary whose storage the last iteration ended is not a write to a dead object. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a96994f to
28e2cc6
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Lifetime resets can mask dangling-pointer errors, and source-coordinate matching can wrongly permit writes outside a modifies clause; loop-form and re-entry regressions also remain unresolved.
4 open issues
What changed in this PR
Extends Kani’s explicit loop_modifies handling to include eligible dead temporaries and loop-body variables, building on #4947 to support loops with early returns.
Changes:
- Adds MIR liveness and storage analysis to identify locals eligible for automatic write-set inclusion.
- Reuses storage-lifetime code generation at loop latches to support havoc of added locals.
- Adds three regression tests and updates eight expected-output check numbers without changing their expected statuses.
| File | Description |
|---|---|
tests/expected/loop-contract/loop_modifies_returned_temporary.rs |
Tests havoc of a borrowed temporary whose storage ended. |
tests/expected/loop-contract/loop_modifies_returned_temporary.expected |
Expects successful verification. |
tests/expected/loop-contract/loop_modifies_returned_local.rs |
Tests a loop-body local used by an early return. |
tests/expected/loop-contract/loop_modifies_returned_local.expected |
Expects successful verification. |
tests/expected/loop-contract/loop_modifies_match_temporary.rs |
Tests a match scrutinee with returning arms. |
tests/expected/loop-contract/loop_modifies_match_temporary.expected |
Expects successful verification. |
tests/expected/loop-contract/loop_assigns_for_vec.expected |
Renumbers an assigns check. |
tests/expected/loop-contract/loop_assigns_for_ref.expected |
Renumbers an assigns check. |
tests/expected/loop-contract/loop_assigns_for_ref_fail.expected |
Renumbers an expected failing check. |
tests/expected/loop-contract/loop_assigns_for_ptr.expected |
Renumbers assigns checks. |
tests/expected/loop-contract/loop_assigns_for_ptr_fail.expected |
Renumbers an expected failing check. |
tests/expected/loop-contract/loop_assigns_for_fat_ptr.expected |
Renumbers assigns checks. |
tests/expected/loop-contract/loop_assigns_for_fat_ptr_fail.expected |
Renumbers an expected failing check. |
tests/expected/loop-contract/loop_assigns_for_empty_slice_fail.expected |
Renumbers an expected failing check. |
kani-compiler/src/kani_middle/transform/loop_contracts.rs |
Identifies additional locals for explicit write sets. |
kani-compiler/src/codegen_cprover_gotoc/overrides/hooks.rs |
Emits storage-live operations before loop back jumps. |
kani-compiler/src/codegen_cprover_gotoc/codegen/statement.rs |
Extracts reusable StorageLive code generation. |
Review effort: Balanced
| storage_live = | ||
| generated.iter().map(|local| gcx.codegen_storage_live(*local, loc)).collect(); |
| let lines = body.locals()[local].span.get_lines(); | ||
| let start = (lines.start_line, lines.start_col); | ||
| loop_span.len() == 4 | ||
| && start > (loop_span[0], loop_span[1]) | ||
| && start < (loop_span[2], loop_span[3]) |
| .call(vec![Expr::pointer_constant(0, Type::void_pointer())], loc) | ||
| .as_stmt(loc), | ||
| ]; | ||
| stmts.extend(storage_live); |
| // body that a `return` in it reads. A variable declared before the loop is not | ||
| // added, even if it is dead at the loop head: the user names it in the clause. | ||
| let heads = body.blocks()[new_latch].terminator.successors(); | ||
| let loop_span: Vec<usize> = loop_id.split('_').filter_map(|n| n.parse().ok()).collect(); |
feliperodri
left a comment
There was a problem hiding this comment.
Approving. This unblocks unbounded proofs of loops that match on a call and return from the
loop, a common shape in core (Challenge 20's Searcher methods). Users can't name the
temporaries involved in loop_modifies, so without this change those loops have no workaround.
Soundness: adding a local to a loop's write set only makes CBMC havoc more at the loop head. That
can cause spurious failures, never a false proof, so the liveness filter is about precision, not
soundness. I looked hardest at the StorageLive emitted at the latch, because it clears the
dead-object marker one step early. It only affects the checks between the latch and the back edge
(invariant and assigns), and a pointer to such a local is havocked at the head anyway. So I don't
see a way to get a false "verified" from it. A comment in the code saying so would help the next
reader (inline).
What I ran, on the PR rebased onto main (c5e65fd):
tests/expected/loop-contractand the other loop tests: 93 passed, 4 ignored.tests/kaniloop
tests: 6 passed.kani-fmt --checkandclippy -D warningsonkani-compilerare clean.- The new tests on
mainwithout the compiler change:loop_modifies_match_temporaryand
loop_modifies_returned_localfail with the "assignable" errors from the description.
loop_modifies_returned_temporarypasses onmain, because the temporary isn't added at all.
With the PR minus only the latchStorageLive, it fails withdereference failure: dead object. So each test guards the part it's meant to. - A probe where the loop invariant reads, through a pointer, a loop-body variable whose storage
ended (p = &tin the body, then*pin the invariant). Kani still reportsdead objecton it,
along with failures from the havocked pointer.
Not run: verify-rust-std#740 (Challenge 20), macOS, and the full regression suite.
The judgment call worth a maintainer's look is the two heuristics that position-match the loop:
- the loop's line/column range, parsed from the register function's name;
- block-index ranges for
writtenandnested, while the new code computes the natural loop.
Both fail safe (the local isn't added, or a spurious failure), but they're the most likely things
to break with a toolchain or macro change.
Good work on:
- the description, which states each condition and why it's there;
- tests that each fail for the right reason;
- factoring
codegen_storage_liveout instead of duplicating it.
| // body that a `return` in it reads. A variable declared before the loop is not | ||
| // added, even if it is dead at the loop head: the user names it in the clause. | ||
| let heads = body.blocks()[new_latch].terminator.successors(); | ||
| let loop_span: Vec<usize> = loop_id.split('_').filter_map(|n| n.parse().ok()).collect(); |
There was a problem hiding this comment.
Suggestion: declared_in_loop recovers the loop's span by parsing the
_<line>_<col>_<line>_<col> suffix of the register function's name, then compares line and
column only, without the file. Two things can go wrong:
- Coupling: this ties the compiler to the macro's naming format, and the format changes silently.
- Files: a variable whose span is in another file (from a macro expansion) can fall inside the
numeric range and be added without the user naming it. That relaxes Keep loop attributes on rewritten for loops and add loop-only locals to loop_modifies #4947's rule that variables
declared before the loop must be named.
Neither is unsound; the variable is just havocked when it shouldn't need to be. Could this use the
span of the loop's MIR instead, for example the span of the register call or of the latch
terminator? That carries the file and needs no parsing.
| started.difference(&ended_outside).copied().collect(); | ||
| // Not for a loop in another loop with a contract: the write set that CBMC infers for | ||
| // the outer loop does not have these locals, so its inclusion check would fail. | ||
| let nested = loop_positions |
There was a problem hiding this comment.
Suggestion: nested, and written at line 1493 (loop_head + 1..=*loop_latch), infer the
loop's blocks from block indices, while this PR adds natural_loop, which computes them. MIR
block order doesn't guarantee that a loop's blocks are contiguous or that a nested loop's indices
fall inside its parent's. If they don't:
writtencan miss a write, and then the loop fails with "assignable" as before;nestedcan misclassify a loop, and then it fails "assigns clause inclusion".
Both are spurious failures, not soundness problems. Using natural_loop for both would make the
three checks agree. If that's out of scope, a comment noting the assumption would do.
Question: do the existing loop_modifies_nested* tests fail if the nested filter is
removed? If not, a test where an inner contracted loop has a match temporary with a return
arm would pin the reason for the filter.
|
|
||
| /// The locals that are live at the start of each block: read (in whole or in part, or | ||
| /// borrowed) on some path from there before the path overwrites them as a whole or ends | ||
| /// their storage. A loop can havoc a local that is not live at its head: no value of it |
There was a problem hiding this comment.
Nit: "A loop can havoc a local that is not live at its head: no value of it reaches the loop
body" holds for direct reads only. A read through a pointer to the local isn't seen. For example,
a temporary without storage markers, borrowed in one iteration and read through the pointer in the
next. Havocking it then is still sound, because it over-approximates, but it can cause a spurious
failure. It would help to say that the analysis ignores indirect reads and why that's safe.
| // dead object (see `codegen_storage_live`) when the latch jumps back to the | ||
| // loop head, where the loop havocs it. It is dead there: mark its storage live, | ||
| // as its `StorageLive` in the body would. | ||
| storage_live = |
There was a problem hiding this comment.
FYI: for readers of this thread. This marks the storage live at the end of the iteration, one
step before the program would. Between this point and the back edge, CBMC only checks the
invariant (and the assigns clause). A dangling read of such a local in the invariant would lose its
dead-object check on the base-case path. But a pointer to it must be in the write set, so it's
havocked at the head, and the step case flags the dereference anyway. Worth one sentence in the
comment above, so the next person doesn't have to redo this reasoning.
|
@hung1962003 can you also address all Copilot's comments before we merge it? |


Follow-up to #4947 (merged), rebased onto
main: one commit.Problem
With an explicit
#[kani::loop_modifies], CBMC requires every local the loop writes to be in the write set. #4947 adds the loop-only locals that Kani generates. Two more kinds of locals cannot be named by the user, and loops that use them still fail withCheck that var_N is assignable:matchwithreturnarms. This is the shape of the providedSearcher::next_rejectincore::str::pattern:returnleaves the loop (theirStorageDeadis outside the loop), e.g.found_charinCharSearcher::next_match.Change
This PR adds a local to an explicit write set when all of the following hold:
StorageLiveand everyStorageDeadare not all inside the natural loop. CBMC already allows writes to storage-local locals.*_failtests are unchanged.assigns clause inclusion.The PR also emits
StorageLivefor these locals at the latch, before the back jump. Without it, havocking an address-taken temporary whose storage the last iteration ended fails withdereference failure: dead object.codegen_storage_liveis factored out of theStorageLivestatement codegen for this reuse, with no behavior change.Testing
loop_modifies_match_temporary.rs(thematchscrutinee) andloop_modifies_returned_local.rs(a loop-body variable read by areturn) fail without this PR (Check that var_12 is assignable,Check that found is assignable) and pass with it.loop_modifies_returned_temporary.rscovers theStorageLiveat the latch. Its borrowed temporaryx + 0is added to the write set, and havocking it after its storage ended must not fail.tests/expected/loop-contract: all pass. Eightloop_assigns_for_*.expectedfiles are renumbered (check numbers only; no status changes).Searchermethods in verify-rust-std'score::str::pattern(Challenge 20) verify unbounded on unmodified bodies with the bundled CBMC 6.11.0 (Challenge 20: Unbounded proofs for the char Searchers of str::pattern verify-rust-std#740).By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.
🤖 Generated with Claude Code