Fix sub-byte bit placement in boolbv convert_byte_update#8960
Fix sub-byte bit placement in boolbv convert_byte_update#8960tautschnig wants to merge 1 commit intodiffblue:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes the bit placement used by boolbvt::convert_byte_update when updating a byte-aligned location with a sub-byte-width value, aligning little-endian behavior with lower_byte_update’s padding/placement semantics. It also adds a new regression test to lock in the expected behavior for a 1-bit update.
Changes:
- Adjust little-endian
convert_byte_updateconstant-offset handling to shift trailing partial-byte bits into the high end of the last byte. - Add a CBMC regression test ensuring a 1-bit store clears the MSB (not LSB) of the first byte on little-endian.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/solvers/flattening/boolbv_byte_update.cpp |
Shifts bits for trailing partial-byte updates in the constant-offset byte_update conversion path. |
regression/cbmc/byte_update_sub_byte1/test.desc |
Adds a regression descriptor to run CBMC with --little-endian --no-simplify and check success. |
regression/cbmc/byte_update_sub_byte1/main.c |
New test program that stores through a __CPROVER_bitvector[1] * and asserts the MSB of the first byte is cleared. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When byte_update writes a sub-byte value (e.g., a 1-bit bitvector)
at a byte-aligned offset, the value must be placed at the high end
of the affected byte, matching the semantics of lower_byte_update
which concatenates {value, remaining_low_bits}.
The boolbv convert_byte_update was placing the value at bit 0 (the
low end) instead. For little-endian, this means the value was
written to the LSB instead of the MSB of the byte. For big-endian,
the endianness map already reverses bits so that bit 0 maps to the
MSB, making the existing code accidentally correct.
Generalise the fix to handle any non-byte-aligned update width: the
trailing partial byte's bits are shifted to the high end for
little-endian, matching lower_byte_update's padding semantics.
Co-authored-by: Kiro <kiro-agent@users.noreply.github.com>
200d911 to
66fe11e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #8960 +/- ##
========================================
Coverage 80.49% 80.49%
========================================
Files 1704 1704
Lines 188789 188795 +6
Branches 73 73
========================================
+ Hits 151967 151973 +6
Misses 36822 36822 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -0,0 +1,13 @@ | |||
| CORE | |||
| main.c | |||
| --little-endian --no-simplify | |||
There was a problem hiding this comment.
We really need to find a better mechanism to test these things that does not involve turning off the simplifier.
There was a problem hiding this comment.
Yes, but this is just another example where the simplifier pretty much does what the back-end would otherwise do. In other words: I was not able to create a test that the simplifier wouldn't already rewrite to an expression that does not have the corner case that's being tackled here.
When byte_update writes a sub-byte value (e.g., a 1-bit bitvector) at a byte-aligned offset, the value must be placed at the high end of the affected byte, matching the semantics of lower_byte_update which concatenates {value, remaining_low_bits}.
The boolbv convert_byte_update was placing the value at bit 0 (the low end) instead. For little-endian, this means the value was written to the LSB instead of the MSB of the byte. For big-endian, the endianness map already reverses bits so that bit 0 maps to the MSB, making the existing code accidentally correct.
Generalise the fix to handle any non-byte-aligned update width: the trailing partial byte's bits are shifted to the high end for little-endian, matching lower_byte_update's padding semantics.