boolbv_let: pass lowered value to record_array_let_binding#9017
Open
tautschnig wants to merge 1 commit into
Open
boolbv_let: pass lowered value to record_array_let_binding#9017tautschnig wants to merge 1 commit into
tautschnig wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a latent bug in boolbvt::convert_let where the byte-operator-lowered expression lowered_value was computed but not actually passed to record_array_let_binding, which would have caused a DATA_INVARIANT failure in collect_arrays if a let-bound array value contained a byte_update.
Changes:
- Pass
lowered_value(instead of the un-loweredpair.second) torecord_array_let_bindingso the array theory sees an expression free of byte operators.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #9017 +/- ##
========================================
Coverage 80.60% 80.60%
========================================
Files 1711 1711
Lines 189466 189487 +21
Branches 73 73
========================================
+ Hits 152719 152737 +18
- Misses 36747 36750 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
convert_let computes 'lowered_value' (the let's bound expression with
byte_update / byte_extract operators rewritten away by
lower_byte_operators) but then dropped it on the floor and passed
the *un*-lowered 'pair.second' to record_array_let_binding. If the
let-bound array value happens to contain a byte_update, the array
theory's collect_arrays will see it and trip
DATA_INVARIANT(false,
"byte_update should be removed before collect_arrays")
Fix: pass 'lowered_value' so the value handed to the array theory
matches the lowering already done at the call site.
The bug was introduced by commit 14105d5 ("Connect let-bound
arrays to originals in array theory"), which added the byte-operator
lowering computation but forgot to thread the result through to
record_array_let_binding. That commit shipped in cbmc-6.9.0, so this is
a release-shipped latent bug.
No standard CBMC entry point currently funnels a byte-operator-bearing
array expression into an array-typed let_exprt (SMT2 input has no
byte_update; value_set_dereference produces pointer-typed lets;
goto-symex's lift_let hoists lets out before they reach boolbvt), so
there is no end-to-end regression test. The fix is pinned instead by a
unit test in unit/solvers/flattening/boolbv.cpp that drives boolbvt
directly with a let binding an unbounded-array symbol to a byte_update
expression: it trips the DATA_INVARIANT above without this change and
converts cleanly with it.
While here, document record_array_let_binding's now load-bearing
precondition (its value must be free of byte_update operators) in
arrays.h.
Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
2d40c05 to
7858db3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
convert_let computes 'lowered_value' (the let's bound expression with byte_update / byte_extract operators rewritten away by lower_byte_operators) but then dropped it on the floor and passed the un-lowered 'pair.second' to record_array_let_binding. If the let-bound array value happens to contain a byte_update, the array theory's collect_arrays will see it and trip
DATA_INVARIANT(false,
"byte_update should be removed before collect_arrays")
Fix: pass 'lowered_value' so the value handed to the array theory matches the lowering already done at the call site.
The bug was introduced by commit 14105d5 ("Connect let-bound arrays to originals in array theory"), which added the byte-operator lowering computation but forgot to thread the result through to record_array_let_binding.
Note on regression coverage: I attempted to construct a test case that triggers the DATA_INVARIANT, but could not. The set of CBMC input paths that produce an array-typed let_exprt (which is the gate at line 78 above) is currently limited to:
So the bug is currently unreachable through standard CBMC entry points, and the fix is defensive against a future code path that might funnel byte-operator-bearing array expressions into a let. The existing regression/smt2_solver/let-array/let-array.smt2 test continues to exercise the convert_let array-typed branch and passes unchanged with the fix.