Fix bitfields spanning nine storage bytes - #3490
Conversation
496817f replaced bit-at-a-time access with byte loops that assemble the field in a u64. A valid 64-bit field starting one bit into storage spans nine bytes, so get(1, 64) and set(1, 64, value) shift by 64 on the last iteration. The setter also discards the high bit when aligning the value. The existing full-word regression test covered aligned fields and fields ending at bit 64, leaving this crossing case untested. Handle the ninth byte separately, after shifting the first word on reads and before shifting away the high bits on writes. Share the read and write implementations across runtime, raw-pointer, and const-generic accessors instead of maintaining separate usize and u64 algorithms. The const-generic wrappers pass their constants into the inlineable helpers, and the getters remain usable in constants. Store byte-aligned 64-bit fields directly with write_unaligned. Such fields use native byte order and need no read-modify-write. This keeps AArch64 code generation to a single store instead of vectorizing eight byte extractions into shifts, shuffles, and constant-pool loads. Use byte pointers without creating references to the whole storage, so raw access can leave neighboring bytes uninitialized. Check the field's bounds before accessing storage, including in release builds, to preserve the safe accessors' bounds checking and reject invalid writes before modifying any bytes. Replace comparisons between accessors sharing the same algorithm with bit-at-a-time expected values. Cover narrow and full-width fields at every intra-byte offset, preservation of neighboring bits, constant evaluation, indirect storage, and bounds failures. Enable this coverage on both endiannesses while retaining the older little-endian-specific cases behind their existing condition.
|
This was written by LLM and reviewed by me. The bug was also spotted by multiple LLMs in aya-rs/aya#1730. |
|
Error: Failed to set assignee to
Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip. |
|
r? @emilio |
|
Another note: this diff looks much cleaner with |
| @@ -45,6 +201,7 @@ fn bitfield_unit_get_bit() { | |||
| ); | |||
| } | |||
|
|
|||
| #[cfg(target_endian = "little")] | |||
There was a problem hiding this comment.
Can you explain why these are needed? It seems to me like it should work on big endian too?
There was a problem hiding this comment.
Right, the whole module was previously little-endian-only. The restrictions were moved onto the existing tests so the new regression tests could run on both endiannesses. Several old tests have little-endian-specific expectations, but you're right about this set_bit round-trip test specifically: it doesn't need the restriction.
Would you like a small follow-up removing that unnecessary restriction?
| @@ -11,7 +11,7 @@ pub(crate) mod struct_layout; | |||
| #[cfg(test)] | |||
| #[allow(warnings)] | |||
| pub(crate) mod bitfield_unit; | |||
| #[cfg(all(test, target_endian = "little"))] | |||
There was a problem hiding this comment.
Huh, ok, I guess we weren't testing that in big endian before.
| impl<const N: usize> __BindgenBitfieldUnit<[u8; N]> { | ||
| /// Get a field using const generics for compile-time optimization. | ||
| /// Uses native word size operations when the field fits in usize. |
There was a problem hiding this comment.
I guess we're just giving this away or at least hoping LLVM optimizes it out? Might be worth checking the codegen if you haven't yet?
3 participants
496817f replaced bit-at-a-time access with byte loops that assemble
the field in a u64. A valid 64-bit field starting one bit into storage
spans nine bytes, so get(1, 64) and set(1, 64, value) shift by 64 on the
last iteration. The setter also discards the high bit when aligning the
value. The existing full-word regression test covered aligned fields and
fields ending at bit 64, leaving this crossing case untested.
Handle the ninth byte separately, after shifting the first word on reads
and before shifting away the high bits on writes. Share the read and
write implementations across runtime, raw-pointer, and const-generic
accessors instead of maintaining separate usize and u64 algorithms. The
const-generic wrappers pass their constants into the inlineable helpers,
and the getters remain usable in constants.
Store byte-aligned 64-bit fields directly with write_unaligned. Such
fields use native byte order and need no read-modify-write. This keeps
AArch64 code generation to a single store instead of vectorizing eight
byte extractions into shifts, shuffles, and constant-pool loads.
Use byte pointers without creating references to the whole storage, so
raw access can leave neighboring bytes uninitialized. Check the field's
bounds before accessing storage, including in release builds, to
preserve the safe accessors' bounds checking and reject invalid writes
before modifying any bytes.
Replace comparisons between accessors sharing the same algorithm with
bit-at-a-time expected values. Cover narrow and full-width fields at
every intra-byte offset, preservation of neighboring bits, constant
evaluation, indirect storage, and bounds failures. Enable this coverage
on both endiannesses while retaining the older little-endian-specific
cases behind their existing condition.