Repository navigation
Ignore min_length of a constant index into an array - #4974
srivatsansamraj wants to merge 3 commits into
Conversation
|
@CYJ904 @Tianshu-Huang @wodex1nhaoIeng @acearyanarun for review. At the same optimisation level, rustc can rewrite an array index that is always out of bounds into a form that broke an assertion inside Kani, so the compiler aborted. Kani now ignores that field the way rustc's own codegen does, and the out-of-bounds check is reported as an ordinary failure. The standard-library runs never hit this, so their numbers don't change. |
6f24d88 to
7af26c8
Compare
Tianshu-Huang
left a comment
There was a problem hiding this comment.
LGTM, just one comment update
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
1 open issue
What changed in this PR
Fixes Kani codegen for MIR ConstantIndex on arrays by ignoring min_length (which can become inconsistent after GVN at -Zmir-opt-level=2), preventing compile-time aborts and ensuring out-of-bounds checks are reported as expected.
Changes:
- Update array
ConstantIndexcodegen to ignoremin_lengthand drop the now-unused parameter. - Add a regression test for issue #2732 and its expected output at
-Zmir-opt-level=2. - Keep slice handling consistent with rustc by not relying on
min_length.
| File | Description |
|---|---|
| tests/expected/issue-2732/issue_2732.rs | Adds a regression test harness covering always-failing and unreachable out-of-bounds indexing. |
| tests/expected/issue-2732/issue_2732.expected | Captures expected Kani results (FAILURE and UNREACHABLE) for the new regression test. |
| kani-compiler/src/codegen_cprover_gotoc/codegen/place.rs | Ignores min_length for array ConstantIndex and removes the parameter from codegen. |
Review effort: Lite
| // Like rustc's codegen, ignore `min_length`: it can exceed `length` after a bounds | ||
| // check that always fails, where GVN turns `a[i]` into `a[i of i+1]`. An | ||
| // out-of-bounds index that is reached fails CBMC's array bounds check. | ||
| let idx = if from_end { length - offset } else { offset }; |
There was a problem hiding this comment.
Done in 3d531c8bd: the index uses wrapping_sub, so an out-of-bounds index reaches CBMC's bounds check instead of panicking.
feliperodri
left a comment
There was a problem hiding this comment.
Dropping the length >= min_length assert follows rustc's codegen, and the explanation of the always-failing bounds check is clear.
Before merge:
length - offsetcan panic in the compiler whenfrom_endis set andoffset > length, on a path behind an always-failing bounds check. Usechecked_sub(orsaturating_sub) and handle the unreachable case explicitly.- Nine jobs are cancelled. Please re-run them.
GVN rewrites an `Index` with a known value into `ConstantIndex { offset,
min_length: offset + 1 }` without comparing it with the array length, so
for `C[10]` on a 5-element array the `length >= min_length` assertion in
`codegen_constant_index` aborted Kani at `-Zmir-opt-level=2` (model-checking#2732).
Like rustc's codegen, Kani now ignores `min_length` for arrays and drops
the unused parameter; an out-of-bounds index that is reached fails CBMC's
array bounds check.
With `min_length` ignored, an offset from the end can exceed the array length on the same paths that are never taken, and `length - offset` would then panic in the compiler. A wrapping subtraction gives an out-of-bounds index instead, which CBMC's bounds check reports if the access is reached.
a5244bd to
3d531c8
Compare

codegen_constant_indexassertedlength >= min_lengthfor arrays, following the MIR documentation, which saysmin_lengthis the exact length for arrays. GVN does not keep that: it rewrites anIndexwith a known value intoConstantIndex { offset, min_length: offset + 1 }without comparing it with the array length. ForC[10]on a 5-element array, the bounds check becomesassert(const false)and the unreachable access after it becomesC[10 of 11], so Kani aborted at compile time (#2732). rustc uses mir-opt-level 2 wheneveropt-levelis above 0, so a crate withopt-level = 1in its dev profile reached this with plaincargo kani.rustc's codegen ignores
min_length(rustc_codegen_ssa/src/mir/place.rs), and so does Kani's slice arm. This ignores it for arrays too and removes the parameter that becomes unused.The issue's program now reports the always-failing bounds check instead of aborting. Kani emits that check as assert then assume, so the access after it is unreachable; were such an access reached, CBMC's
array_boundscheck would fail. With--cbmc-args --no-assumptions,test.array_bounds.1goes from SUCCESS to FAILURE.The new test
expected/issue-2732(-Zmir-opt-level=2) calls the issue's function directly, expecting the bounds-check failure, and behind a branch that is never taken, expecting it unreachable. It panics onmain.Resolves #2732
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.