Expose bit-field position and width in variable reflection - #1106
Open
ipburbank wants to merge 2 commits into
Open
Expose bit-field position and width in variable reflection#1106ipburbank wants to merge 2 commits into
ipburbank wants to merge 2 commits into
Conversation
A bit-field data member is indistinguishable from an ordinary one through the reflection API: there is no way to ask whether a variable is a bit-field, nor how many bits it declares. A downstream binding layer therefore treats a bit-field as a whole-width member and reads or writes the entire storage unit that contains it. Expose clang's FieldDecl::isBitField() and getBitWidthValue() so callers can identify bit-fields and size them. Both return a false / -1 sentinel for a non-bit-field or a non-FieldDecl, so any variable can be queried without checking its kind first.
GetVariableOffset converts a field's offset with toCharUnitsFromBits,
which truncates to whole bytes and so discards the sub-byte position a
bit-field needs. Recover it without duplicating the record-layout walk:
bit_offset = 8 * GetVariableOffset(var, parent) + getFieldOffset(FD) % 8
This is exact. GetVariableOffset already accumulates anonymous
struct/union nesting and non-virtual base-class offsets, and every level
of that accumulation apart from the innermost field is a record-typed
member or a base subobject, both of which are byte-aligned; only the
innermost FieldDecl can sit at a sub-byte offset. The residue is in fact
frame-independent: the two frames differ by a sum of exact multiples of
8 bits, and residues mod 8 are invariant under adding those.
Delegating also makes floor(GetVariableBitOffset(v, p) / 8) equal to
GetVariableOffset(v, p) by construction, so a caller's byte offset and
its shift can never disagree. An independent second walk would not have
that property, and a disagreement between the two would corrupt reads
silently.
A field in a virtual base subobject is not supported: the underlying
byte offset comes from ASTRecordLayout::getBaseClassOffset, which
consults non-virtual bases only. The limitation is documented on the API.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1106 +/- ##
==========================================
+ Coverage 87.74% 87.77% +0.02%
==========================================
Files 23 23
Lines 6429 6452 +23
==========================================
+ Hits 5641 5663 +22
- Misses 788 789 +1
🚀 New features to boost your workflow:
|
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.
A bit-field data member is indistinguishable from an ordinary one through the reflection API: nothing reports whether a variable is a bit-field or how many bits it declares, and
GetVariableOffsetconverts the field offset withtoCharUnitsFromBits, which truncates to whole bytes and discards the sub-byte position. A binding layer is therefore forced to treat a bit-field as a whole-width member and read or write the entire storage unit containing it — returning the neighbouring fields on a read and clobbering them on a write.IsBitFieldVariableandGetVariableBitWidthexposeFieldDecl::isBitField()andgetBitWidthValue(). Both return afalse/-1sentinel for a non-bit-field or a non-FieldDecl, so any variable can be queried without first checking its kind.GetVariableBitOffsetrecovers the discarded position without duplicating the record-layout walk:This is exact.
GetVariableOffsetalready accumulates anonymous struct/union nesting and non-virtual base-class offsets, and every level of that accumulation apart from the innermost field is a record-typed member or a base subobject, both of which are byte-aligned; only the innermostFieldDeclcan sit at a sub-byte offset. The residue is in fact frame-independent:getFieldOffsetis relative toFD->getParent()while the accumulated byte offset is relative toparent, but the two frames differ by a sum of exact multiples of 8 bits, and residues mod 8 are invariant under adding those.Delegating rather than walking the layout a second time also makes
floor(GetVariableBitOffset(v, p) / 8)equalGetVariableOffset(v, p)by construction, so a caller's byte offset and its shift can never disagree. An independent walk would not have that property, and a disagreement between the two is precisely the silent corruption these APIs exist to prevent — so the delegation is deliberate rather than an economy.A field in a virtual base subobject is not supported, and the limitation is documented on the API. The byte offset ultimately comes from
ASTRecordLayout::getBaseClassOffset, which consultsBaseOffsets— non-virtual bases only — so an assertions-off build reads a default-insertedCharUnits(0)and also pollutes the cached layout. That is pre-existing inGetVariableOffset(the byte offset is wrong for the same field today) and is left alone here to keep this change atomic; it deserves its own issue and fix, presumably viagetVBaseClassOffset.Tested in
VariableReflectionTestfor the true and false cases, declared widths, offsets both at and away from a byte boundary, a bit-field reached through multiple inheritance (where the base offset is non-zero, so the base-class traversal genuinely runs), and two-level anonymous struct-in-union nesting. The suite also asserts thefloor(bit_offset / 8) == GetVariableOffsetinvariant over every bit-field in every fixture, which is ABI-independent and therefore the strongest available check. The few ABI-dependent absolute offsets live in a separateVariableReflection_BitFieldOffsetItaniumABItest, skipped early on Windows, because the MS ABI starts a fresh allocation unit for a bit-field that follows a non-bit-field.This is the CppJIT-side equivalent of wlav/cppyy#57. The fixes for that issue against the cppyy stack were wlav/CPyCppyy#69, wlav/cppyy#332 and wlav/cppyy-backend#39; rather than merge them, the issue's maintainer directed the work upstream to compiler-research, noting a new backend would need a different placement. The four-hop cling chain those PRs added (
TClingDataMemberInfo→TCling→TDataMember→Cppyy::→ C API) collapses into the three APIs here.The downstream consumer is compiler-research/cppjit#73, which uses these three APIs to fix Python-level bit-field reads and writes; it cannot build until this lands and its
CPPINTEROP_GIT_TAGis bumped.