Skip to content

[cpyrt] Access bit-field data members by masked read-modify-write - #73

Open
ipburbank wants to merge 2 commits into
compiler-research:mainfrom
ipburbank:fix/bitfields
Open

[cpyrt] Access bit-field data members by masked read-modify-write#73
ipburbank wants to merge 2 commits into
compiler-research:mainfrom
ipburbank:fix/bitfields

Conversation

@ipburbank

Copy link
Copy Markdown

A bit-field data member was treated as an ordinary one: its offset was a plain byte offset and access was a full-width load or store. A read returned the whole containing storage word, and a write clobbered every sibling bit-field sharing it. For struct S { int a : 3; int b : 5; S() : a(1), b(2) {} } both s.a and s.b returned the same word rather than 1 and 2. This is wlav/cppyy#57 on the CppJIT side.

CPPDataMember::Set now resolves the layout once and caches it in flags, so the attribute-access path never takes the interop lock, and dm_get/dm_set read and write only the bytes the field actually occupies — shift = bit_offset % 8, nbytes = (shift + bit_width + 7) / 8, masked to the declared width, with writes as a read-modify-write over that span so neighbouring bits survive. Deriving the span from (bit_offset, bit_width) rather than from the declared type's width is what keeps typedefs, enum-typed bit-fields and packed structs correct: a type-width read runs past the end of a packed struct's trailing member. Signedness comes from IsIntegerType rather than the type's spelling, so uint32_t and uint64_t bit-fields are not sign-extended, and bool bit-fields read back as bool and reject non-boolean writes, matching cpyrt_PyLong_AsBool.

Fields wider than 64 bits are refused rather than masked and fall through to the converter, because a 128-bit field at a non-zero bit offset spans 17 bytes and would overrun the accumulator. The accumulator is unsigned __int128 because a legitimate span reaches nine bytes (unsigned long long b : 64 at bit offset 7 in a packed struct); it carries an #error guard, as it is the first use of __int128 in src/ and there is no MSVC or 32-bit fallback.

Known behaviour worth flagging, unchanged by this PR: a char-typed bit-field returns an int where a plain char member returns a one-character str; an out-of-range write truncates silently (bf : 4 = 2**100 gives 0) where a plain member raises; and a bit-field declared through a typedef of bool reads back as 0/1 rather than False/True, since there is no IsBoolType query and IsIntegerType reports bool as unsigned. Separately, numba_ext still lowers a member access as a load of the declared type at the byte offset, so a bit-field read inside a jitted function continues to return the containing word — pre-existing, out of scope here, but newly inconsistent with the Python path.

Sixteen tests in test_datatypes.py cover unsigned, signed, typedef'd, bool and enum-typed fields, multi-unit spans, a packed struct's trailing field, anonymous and named unions, inherited fields, zero-width separators, full-width signed and unsigned fields, const fields, and the nine-byte span. Note the over-read the byte-span design prevents cannot be caught by any assertion: at shift == 0 a wider read extracts the same masked value and a masked read-modify-write restores the out-of-bounds bytes unchanged, so that property is guarded only by the vg: true valgrind cells in CI — narrowing them silently removes the check. Verified locally at 566 passed / 35 skipped / 16 xfailed / 0 failed, and clean under valgrind (0 errors from 0 contexts).

Depends-On: compiler-research/CppInterOp#1106, which adds the IsBitFieldVariable, GetVariableBitOffset and GetVariableBitWidth reflection APIs this builds on. CI here will fail until that lands and CPPINTEROP_GIT_TAG is bumped past 9802d61, which does not contain them; the pin bump is deliberately not part of this PR. The equivalent fixes against the old cppyy stack were wlav/CPyCppyy#69, wlav/cppyy#332 and wlav/cppyy-backend#39, which the cppyy maintainer directed upstream to compiler-research rather than merging.

A bit-field was treated as an ordinary data member: its offset was a plain
byte offset and access was a full-width load or store. A read returned the
entire containing storage word, and a write clobbered every sibling
bit-field sharing it. For

    struct S { int a : 3; int b : 5; S() : a(1), b(2) {} };

both s.a and s.b returned the same word rather than 1 and 2.

Resolve the layout once when the descriptor is built, then read and write
only the bytes the field actually occupies:

    shift  = bit_offset % 8
    nbytes = (shift + bit_width + 7) / 8

Deriving the span from (bit_offset, bit_width) rather than from the
declared type's width is what keeps typedefs, enum-typed bit-fields and
packed structs correct: a type-width read runs past the end of a packed
struct's trailing member. Writes are a read-modify-write over that same
span, so neighbouring bits survive.

Signedness comes from a type query rather than the type's spelling, which
is why uint32_t and uint64_t bit-fields are not sign-extended. bool
bit-fields read back as bool and reject non-boolean writes, matching
cpyrt_PyLong_AsBool. Fields wider than 64 bits are refused rather than
masked, since their span would exceed the accumulator.
Covers the shapes where a naive implementation reads past the end of an
object or disturbs a neighbour: fields spanning multiple storage units, a
packed struct's trailing field, anonymous and named unions, inherited
fields, zero-width separators, full-width signed and unsigned fields,
const fields, and a nine-byte span.

The nine-byte case is what justifies the 128-bit accumulator: a 64-bit
field at bit offset 7 spans nine bytes, which a 64-bit accumulator would
silently truncate.

Note the over-read these tests are named for cannot be caught by any
assertion. At shift == 0 a wider read extracts the same masked value, and
a masked read-modify-write restores the out-of-bounds bytes unchanged.
That property is guarded by the valgrind cells in CI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant