Add support for unaligned_volatile_load and unaligned_volatile_store#4673
Add support for unaligned_volatile_load and unaligned_volatile_store#4673ivmat wants to merge 6 commits into
unaligned_volatile_load and unaligned_volatile_store#4673Conversation
…mory and volatile_set_memory Implements three of the intrinsics still unchecked on tracking issue model-checking#1163. Why the existing placeholders could not simply be un-gated ---------------------------------------------------------- The two copy intrinsics already had arms, but behind `unstable_codegen!`: Intrinsic::VolatileCopyMemory => unstable_codegen!(codegen_intrinsic_copy!(Memmove)), Intrinsic::VolatileCopyNonOverlappingMemory => { unstable_codegen!(codegen_intrinsic_copy!(Memcpy)) } `unstable_codegen!` is declared `($($tt:tt)*)` and never expands its tokens -- it unconditionally emits a codegen_unimplemented_expr. Those bodies were therefore never type-checked, and they have since rotted: codegen_intrinsic_copy! no longer exists anywhere in kani-compiler. Removing the gate alone does not compile, so both arms are rewritten rather than un-gated. `volatile_set_memory` had no Intrinsic variant at all and fell through to the unsupported catch-all. Argument order -------------- The intrinsics take the destination first: volatile_copy_memory<T>(dst: *mut T, src: *const T, count: usize) volatile_copy_nonoverlapping_memory<T>(dst: *mut T, src: *const T, count: usize) whereas codegen_copy consumes source first (two successive fargs.remove(0), src then dst) and indexes farg_types[0]/farg_types[1] for the src and dst alignment assertions. Both fargs and farg_types are therefore swapped before delegating. Swapping only one silently applies each alignment check to the wrong pointer -- a defect no test using two equally-aligned buffers would catch. volatile_set_memory(dst, val, count) matches write_bytes(dst, val, count), so no swap is needed there. Soundness of reusing the non-volatile codegen --------------------------------------------- Volatility constrains what the optimizer may do -- it must not elide, duplicate or reorder the access. It is not a memory-safety property: the UB conditions for these three intrinsics are exactly those of copy, copy_nonoverlapping and write_bytes. Kani performs no optimization that volatility is meant to suppress, so there is nothing additional to model. This mirrors the existing treatment of volatile_load/volatile_store. Points-to analysis ------------------ points_to_analysis.rs matches exhaustively over Intrinsic with a terminal unimplemented!() wildcard; write_bytes is protected from it via is_identity_aliasing_intrinsic. Before this change volatile_set_memory reached that pass as Intrinsic::Unimplemented, which is explicitly handled as a no-op. Introducing the new variant would have routed it to the panicking wildcard, so it is listed alongside write_bytes in the same way. The two check_uninit visitors have non-panicking fallbacks and are unchanged. Tests ----- Layout follows model-checking#1347, which added volatile_load support. tests/kani/Intrinsics/Volatile/copy.rs both copy intrinsics move the expected bytes, including an overlapping case for volatile_copy_memory (memmove semantics) that distinguishes it from the non-overlapping variant tests/kani/Intrinsics/Volatile/set.rs volatile_set_memory fills the destination, full and partial tests/expected/intrinsics/volatile_copy/overlapping/ volatile_copy_nonoverlapping_memory on overlapping ranges fails, matching the existing copy-nonoverlapping test for the non-volatile intrinsic tests/expected/intrinsics/volatile_copy/unaligned/ a misaligned pointer fails the alignment check The unaligned pair is deliberately left out: unaligned_volatile_load's gated body is a plain dereference, which does not model the unaligned access itself, and unaligned_volatile_store has no codegen at all. Those need a separate decision about how unaligned accesses should be represented.
`docs/src/rust-feature-support/intrinsics.md` listed all three as `No`. They are now supported, so the table has to move. `Partial` rather than `Yes`, matching the neighbouring `volatile_load` and `volatile_store` rows and keeping the same Concurrency note. The memory-safety semantics are modelled exactly (they are those of the non-volatile counterparts), but the volatile guarantees themselves -- that the access is not elided or reordered, and really does touch memory -- are not, since Kani assumes sequential execution. Claiming `Yes` would overstate that and would disagree with how the two already-supported volatile intrinsics are documented.
89ad219 to
68437b9
Compare
|
Marking this as a draft until #4672 is reviewed and lands. Nothing has changed in the content — this is just to make the ordering explicit, since GitHub cannot base a cross-fork PR on a fork branch and this diff therefore currently shows #4672's commits as well. Once #4672 merges I will rebase this down to its own two commits and mark it ready. |
…s comment
Two review findings on the previous commit.
1. The compiler matches on `Intrinsic` in three places -- codegen, the points-to
analysis, and the memory-initialization visitor -- and only the first two were
updated. `volatile_set_memory` therefore fell to the visitor's catch-all and
produced a spurious "Kani does not support reasoning about memory
initialization" failure under -Z uninit-checks, while the codegen comment
claimed it behaves exactly like `write_bytes`. It now shares that arm: same
argument shape (dst, val, count), same initialization effect.
2. The comment overclaimed. "The UB conditions are exactly those of the
non-volatile counterpart" is false as a generalisation: the volatile family's
docs additionally permit pointers outside any Rust allocation (the MMIO
carve-out on ptr::read_volatile), which the non-volatile ops do not. The
modelling is still sound, but for a different reason than the comment gave --
reusing codegen_copy/codegen_write_bytes checks AT LEAST the documented UB
conditions, so it is conservative rather than exact: a legal MMIO access can
be rejected by --pointer-check, but real UB is never missed. The comments now
say that, and anchor on what these intrinsics' own docs state ("consistent
with copy_nonoverlapping" / "consistent with write_bytes") rather than on a
blanket claim about volatility.
Also adds tests/expected/intrinsics/volatile_set/out-of-bounds, mirroring the
existing write_bytes test. The happy-path test alone did not pin that reusing
codegen_write_bytes actually carries its safety checks across.
Completes the volatile intrinsic family on tracking issue model-checking#1163, on top of the volatile copy/set support. With these two, `unstable_codegen!` has no remaining uses and is removed: these were the last gated intrinsics in the codegen match. Modelling --------- Neither intrinsic has an alignment requirement, so neither emits an alignment assertion -- tolerating a misaligned pointer is the whole point of the `unaligned_*` variants. `unaligned_volatile_load` was already sketched (behind the gate) as a plain dereference; `unaligned_volatile_store` had no codegen and no Intrinsic variant at all, and is added mirroring `volatile_store` minus the alignment check. Dereferenceability is still checked by --pointer-check, as for the aligned variants. The question this leaves is whether a misaligned typed dereference is modelled byte-precisely rather than silently assuming alignment. The tests answer it directly rather than assuming: each proof compares the accessed value against a byte-wise oracle at a deliberately misaligned offset, so an implementation that quietly touched the *aligned* word would fail rather than pass. Reading a u32 at byte offset 1 of a known pattern, the only correct little-endian answer is 0x55443322; an alignment-assuming read from offset 0 would give 0x44332211. A third proof leaves the offset symbolic and compares against u32::from_le_bytes at every offset, so the result cannot be reached by constant folding. Tests ----- tests/kani/Intrinsics/Volatile/unaligned.rs byte-precise load at a misaligned offset; byte-precise store checking the neighbouring bytes are untouched; symbolic-offset load against a byte-wise oracle. tests/kani/VolatileIntrinsics/core_intrinsics.rs was marked kani-verify-fail; that expectation existed only because unaligned_volatile_store (and volatile_set_memory) were unsupported. With both implemented the proof verifies -- 0 of 120 failed -- so the kani-verify-fail header is removed and it becomes a passing test. Note: tests/kani/VolatileIntrinsics/main_fixme.rs is left untouched (it is skipped as a fixme test). Its test_copy_volatile names its arguments as though volatile_copy_memory were (src, dst, count); the assertion it makes happens to hold under either argument order, so it neither catches nor is broken by the ordering. Worth a separate look if that suite is ever revived.
…ort table Same reasoning as for the copy/set trio: the table listed both as `No`, and `Partial` matches the neighbouring volatile rows. The memory-safety semantics are modelled -- including the byte-precise misaligned access these two exist for -- but the volatile guarantees are not, since Kani assumes sequential execution.
…rop endianness assumption
Review findings on the previous commit. The first is a regression the commit
introduced and is the reason for this one.
1. ICE. Adding the `UnalignedVolatileStore` variant without teaching the
points-to analysis about it routed it into that pass's terminal
`unimplemented!()`. Before this feature the intrinsic resolved to
`Intrinsic::Unimplemented`, which the pass handles gracefully -- so the
feature turned a clean "unsupported" into a compiler panic for anyone running
-Z uninit-checks over code calling it. It now shares the `VolatileStore` arm;
the aliasing semantics (`*a = b`) are identical. The asymmetry was visible in
the diff: the *load* was handled, the store was not.
2. Same omission in the memory-initialization visitor: `unaligned_volatile_store`
fell to the catch-all and reported "unsupported" rather than marking its
destination initialized. It now shares the `VolatileStore` arm there too.
Both were the same root cause -- the compiler matches on `Intrinsic` in three
places (codegen, points-to, uninit visitor) and they must be updated as a set.
3. ZST. `unaligned_volatile_store` dereferenced unconditionally, while
`codegen_volatile_store` guards exactly that case ("do not attempt to
dereference (and assign) a ZST"). A ZST pointer may legally be
dangling-but-aligned, so the path is reachable. Guard replicated and a
regression test added.
4. Tests no longer assume little-endian. The oracles now use
`u32::from_ne_bytes` / `to_ne_bytes`, which is equally byte-precise and
discriminates the same way -- an alignment-assuming read of bytes 0..4 differs
from the correct 1..5 under either endianness -- without pinning the suite to
one target.
Verified: -Z uninit-checks over code calling both `unaligned_volatile_store` and
`volatile_set_memory` now verifies successfully, with no panic and no spurious
memory-initialization failure.
68437b9 to
ebfdfb8
Compare
|
Pushed a follow-up commit after a review pass. One item was a regression this PR itself introduced, so flagging it explicitly rather than folding it in quietly. ICE. Adding the The same omission existed in ZST guard. Endianness. The oracle tests no longer assume little-endian — they use Verified: The soundness comment was also reworded — see the note on #4672; the same conservative-direction correction applies here. |
Completes the volatile intrinsic family on the tracking issue #1163, on top of #4672
(volatile copy/set).
With these two,
unstable_codegen!has no remaining uses anywhere inkani-compilerandis removed — these five volatile/unaligned entries were the last gated intrinsics in the
codegen match.
Modelling
Neither intrinsic has an alignment requirement, so neither emits an alignment assertion —
tolerating a misaligned pointer is the entire purpose of the
unaligned_*variants.unaligned_volatile_loadwas already sketched behind the gate as a plain dereference;unaligned_volatile_storehad no codegen and noIntrinsicvariant at all, and is addedmirroring
volatile_storeminus the alignment check. Dereferenceability is still checkedby
--pointer-check, as for the aligned variants.That leaves one question worth being explicit about: is a misaligned typed dereference
modelled byte-precisely, or does it quietly assume alignment? The tests answer it rather
than assume it. Each proof compares the accessed value against a byte-wise oracle at a
deliberately misaligned offset, so an implementation that touched the aligned word instead
would fail rather than silently pass:
u32at byte offset 1 of a known pattern, the only correct little-endiananswer is
0x55443322; an alignment-assuming read from offset 0 would give0x44332211.shows up as a clobbered neighbour.
u32::from_le_bytesatevery offset, so the result cannot be reached by constant folding.
Tests
tests/kani/Intrinsics/Volatile/unaligned.rs— the three proofs above.tests/kani/VolatileIntrinsics/core_intrinsics.rs— this was markedkani-verify-fail,but that expectation existed only because
unaligned_volatile_store(andvolatile_set_memory) were unsupported. With both implemented the proof verifies,0 of 120 failed, so the header is removed and it becomes a passing test.
Note
tests/kani/VolatileIntrinsics/main_fixme.rsis left untouched (it is skipped as a fixmetest). Its
test_copy_volatilenames its arguments as thoughvolatile_copy_memorywere(src, dst, count); the assertion it makes happens to hold under either argument order, soit neither catches nor is broken by the ordering. Worth a separate look if that suite is
ever revived.
By submitting this pull request, I confirm that my contribution is made under the terms of
the Apache 2.0 and MIT licenses.