-
Notifications
You must be signed in to change notification settings - Fork 156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support POD types with bitfields #1478
Open
tchebb
wants to merge
4
commits into
google:main
Choose a base branch
from
tchebb:support-bitfields
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains 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
This makes it usable in more places, like when the segment we care about isn't the first. Soon to be used for __BindgenBitfieldUnit detection.
Although bindgen has supported bitfields[1] for a long time, autocxx currently forbids them[2] in POD types. The main obstacle to support is the way bindgen represents bitfields, using a custom struct (emitted alongside the bindings) named __BindgenBitfieldUnit<T>, where T is the underlying storage type (e.g. [u8; 4usize] for a uint32_t bitfield). Because bitfields are structs on the Rust side types, autocxx wants __BindgenBitfieldUnit to be POD before it'll let any struct that includes one be POD. But there are a few reasons we shouldn't/can't mark __BindgenBitfieldUnit as a POD type: 1. It's not semantically correct: POD is a property of types from C++, but __BindgenBitfieldUnit is purely a Rust-side abstraction. 2. The double underscore at the beginning of __BindgenBitfieldUnit causes our validate_ident_ok_for_cxx() to reject it, meaning it never gets added to apis when we parse bindgen's output. 3. For the same reason, cxx rejects __BindgenBitfieldUnit as a type in the bridge module. 4. Even ignoring all that, our existing struct analysis can't handle structs with type parameters at all, and our convert_type() can't handle array-type type parameters. We can't easily fix these issues, but we can sidestep them! Like google#1 says, it doesn't make sense to treat __BindgenBitfieldUnit as a C++ type at all. So instead of trying to analyze it as one, we can treat it as what it is--a primitive type that happens to have a somewhat odd spelling on the Rust side. This commit adds a few special cases to make analyze_pod_apis() "see through" fields of type __BindgenBitfieldUnit, treating them like the underlying storage type (taken from the type parameter) instead of like structs. And that's all we need: the (now-deleted) error message that implies cxx can't handle types with __BindgenBitfieldUnit members turns out to be wrong: cxx doesn't care at all[3] about the names of fields inside ExternType structs, so it needs no changes. Interestingly, we unwrap the bitfield type in exactly the same way we unwrap the bindgen marker types from commit 97d7fa9 ("Switch to using new version of autocxx-bindgen"). We even use the same code! [1] https://rust-lang.github.io/rust-bindgen/using-bitfields.html [2] google#1313 [3] https://github.com/dtolnay/cxx/blob/33084bae28e7b690f977b27d7bdab11f3bab8ce4/syntax/check.rs#L318-L354
These are broken in bindgen[1]. If/when upstream fixes them, we can re-enable the assertions. [1] rust-lang/rust-bindgen#1160
tchebb
commented
Mar 17, 2025
@@ -345,6 +345,7 @@ impl IncludeCppEngine { | |||
.clang_args(make_clang_args(inc_dirs, extra_clang_args)) | |||
.derive_copy(false) | |||
.derive_debug(false) | |||
.derive_default(true) // Needed to safely construct POD structs with bitfields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if you think this'll have detrimental code size or correctness effects elsewhere. As per bindgen's documentation, it's the only ergonomic way to construct types with bitfields, but I can leave it out if it breaks other things.
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.
Although bindgen has supported bitfields for a long time, autocxx currently forbids them in POD types (issue #1303). The main obstacle to support is the way bindgen represents bitfields, using a custom struct (emitted alongside the bindings) named
__BindgenBitfieldUnit<T>
, whereT
is the underlying storage type (e.g.[u8; 4usize]
for auint32_t
bitfield).Because bitfields are structs on the Rust side types, autocxx wants
__BindgenBitfieldUnit
to be POD before it'll let any struct that includes one be POD. But there are a few reasons we shouldn't/can't mark__BindgenBitfieldUnit
as a POD type:__BindgenBitfieldUnit
is purely a Rust-side abstraction.__BindgenBitfieldUnit
causes ourvalidate_ident_ok_for_cxx()
to reject it, meaning it never gets added toapis
when we parse bindgen's output.__BindgenBitfieldUnit
as a type in the bridge module.convert_type()
can't handle array-type type parameters.We can't easily fix these issues, but we can sidestep them! Like point 1 says, it doesn't make sense to treat
__BindgenBitfieldUnit
as a C++ type at all. So instead of trying to analyze it as one, we can treat it as what it is—a primitive type that happens to have a somewhat odd spelling on the Rust side.This commit adds a few special cases to make
analyze_pod_apis()
"see through" fields of type__BindgenBitfieldUnit
, treating them like the underlying storage type (taken from the type parameter) instead of like structs. And that's all we need: the (now-deleted) error message that implies cxx can't handle types with__BindgenBitfieldUnit
members turns out to be wrong: cxx doesn't care at all about the names of fields inside ExternType structs, so it needs no changes.Interestingly, we unwrap the bitfield type in exactly the same way we unwrap the bindgen marker types from #1442. We even use the same code!
This PR depends on adetaylor/rust-bindgen#15. Without it, things will mostly work, but some types that end with bitfields (including the one in the new integration tests) will fail to generate due to the opaque padding field.
If you merge this PR, please don't squash it. The commits are already split appropriately.