Skip to content

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

Open
ipburbank wants to merge 14 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 14 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.

aaronj0 and others added 14 commits August 25, 2026 15:17
…er-research#41)

* [cpyrt] Improve error reporting for method calls without C++ object

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* [test] Add test for method calls on an instance without a C++ object

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Grigori Rybkine <Grigori.Rybkine@cern.ch>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…esearch#40)

* [cpyrt] Penalize void* arguments in overload priority as intended

* [test] Add regression test for void* overload priority

---------

Co-authored-by: Emery Conrad <emery.conrad@chicagotrading.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…rch#42)

Fixes the long-standing header duplication (previously `Cppyy.h` and `cpp_cppyy.h`) between cpyrt and interop, so that there is only a single source of definitions for the `cppjit::interop` API and types.
Changed definition of a function to the test it is actually used, and added a missing import
…er-research#50)

Drops wheel sizes by about half. Previously the entire install tree of CppInterOp was staged that included duplicate shared libs due to versioning. This is fixed by adding a stripped shared-lib option in CppInterOp, leveraged in this patch.
…ch#52)

[cpyrt] Clear the error indicator only where a call failed

A debug build of CPython asserts when a C API call is made with the
error indicator already set, so the __cpp_cross__ annotation, the
meta_setattro fallthrough to tp_setattro, and the VectorData alias need
it cleared. Clear it only on the failing path: AddToClass reports
failure, and the two CPPScope sites can test the value they just
produced, so an error raised elsewhere still propagates.

meta_getattro returns a new reference; release it instead of leaking it.
* [test] Find eigen and boost under the Homebrew and MacPorts prefixes

* [ci] Build and test manylinux and macOS arm64 wheels
…esearch#54)

SetArg() created an element converter per call and appended it to
fConverters, but Clear() frees only fBuffer, so the vector grew without
bound across repeated std::initializer_list conversions. Create each
element converter once, on first use of its index, and reuse it.
…iler-research#51)

* Serialize and atomize test dictionary builds

* Force loadfile scheduling for distributed test runs

* Normalize the xfail marker keyword order

* Correct the xfail markers and add missing reasons

* Add the --run-crashing-xfails collection option

* Enable strict xfail

* Drop xfail markers that no longer fail on macOS and cling

* Make the span tests include their own header
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.

3 participants