Skip to content

Support POD types with bitfields #1478

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions engine/src/conversion/analysis/pod/byvalue_checker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use crate::{
conversion::{
analysis::tdef::TypedefPhase,
api::{Api, TypedefKind},
type_helpers::unwrap_bitfield,
},
types::{Namespace, QualifiedName},
};
Expand Down Expand Up @@ -158,12 +159,6 @@ impl ByValueChecker {
));
break;
}
None if ty_id.get_final_item() == "__BindgenBitfieldUnit" => {
field_safety_problem = PodState::UnsafeToBePod(format!(
"Type {tyname} could not be POD because it is a bitfield"
));
break;
}
None => {
field_safety_problem = PodState::UnsafeToBePod(format!(
"Type {tyname} could not be POD because its dependent type {ty_id} isn't known"
Expand Down Expand Up @@ -258,6 +253,10 @@ impl ByValueChecker {
for f in &def.fields {
let fty = &f.ty;
if let Type::Path(p) = fty {
if unwrap_bitfield(p).is_some() {
// Bitfields are POD even though bindgen represents them as structs.
continue;
}
results.push(QualifiedName::from_type_path(p));
}
// TODO handle anything else which bindgen might spit out, e.g. arrays?
Expand Down Expand Up @@ -358,4 +357,19 @@ mod tests {
bvc.ingest_struct(&t, &Namespace::new());
assert!(bvc.satisfy_requests(vec![t_id]).is_err());
}

#[test]
fn test_with_bitfield() {
let mut bvc = ByValueChecker::new();
let t: ItemStruct = parse_quote! {
struct Foo {
a: root::__BindgenBitfieldUnit<[u8; 4usize]>,
b: i64,
}
};
let t_id = ty_from_ident(&t.ident);
bvc.ingest_struct(&t, &Namespace::new());
bvc.satisfy_requests(vec![t_id.clone()]).unwrap();
assert!(bvc.is_pod(&t_id));
}
}
9 changes: 8 additions & 1 deletion engine/src/conversion/analysis/type_converter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ use crate::{
api::{AnalysisPhase, Api, ApiName, NullPhase, TypedefKind, UnanalyzedApi},
apivec::ApiVec,
codegen_cpp::type_to_cpp::CppNameMap,
type_helpers::{unwrap_has_opaque, unwrap_has_unused_template_param, unwrap_reference},
type_helpers::{
unwrap_bitfield, unwrap_has_opaque, unwrap_has_unused_template_param, unwrap_reference,
},
ConvertErrorFromCpp,
},
known_types::{known_types, CxxGenericType},
Expand Down Expand Up @@ -197,6 +199,11 @@ impl<'a> TypeConverter<'a> {
self.convert_type(ty.clone(), ns, ctx)
} else if let Some(ty) = unwrap_has_opaque(&typ) {
self.convert_type(ty.clone(), ns, ctx)
} else if let Some(ty) = unwrap_bitfield(&typ) {
// Bindgen bitfields are of type __BindgenBitfieldUnit, which is a convenience struct
// that's guaranteed to match the layout of the underlying bitfield. Pretend it's the
// underlying storage type to avoid referencing it in the cxx bridge.
self.convert_type(ty.clone(), ns, ctx)
} else if let Some(ptr) = unwrap_reference(&typ, false) {
// LValue reference
let mutability = ptr.mutability;
Expand Down
61 changes: 41 additions & 20 deletions engine/src/conversion/type_helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,31 +69,33 @@ pub(crate) fn type_is_reference(ty: &syn::Type, search_for_rvalue: bool) -> bool
matches_bindgen_marker(ty, marker_for_reference(search_for_rvalue))
}

fn matches_bindgen_marker(ty: &syn::Type, marker_name: &str) -> bool {
fn matches_bindgen_marker(ty: &syn::Type, type_name: &str) -> bool {
matches!(&ty, Type::Path(TypePath {
path: Path { segments, .. },..
}) if segments.first().map(|seg| seg.ident == marker_name).unwrap_or_default())
}) if segments.first().map(|seg| seg.ident == type_name).unwrap_or_default())
}

fn unwrap_bindgen_marker<'a>(ty: &'a TypePath, marker_name: &str) -> Option<&'a syn::Type> {
ty.path
.segments
.first()
.filter(|seg| seg.ident == marker_name)
.and_then(|seg| match seg.arguments {
PathArguments::AngleBracketed(ref angle_bracketed_args) => {
angle_bracketed_args.args.first()
}
_ => None,
})
.and_then(|generic_argument| match generic_argument {
GenericArgument::Type(ty) => Some(ty),
_ => None,
})
fn unwrap_newtype<'a>(seg: &'a PathSegment, marker_name: &str) -> Option<&'a syn::Type> {
if seg.ident != marker_name {
return None;
}

let PathArguments::AngleBracketed(ref angle_bracketed_args) = seg.arguments else {
return None;
};

if let GenericArgument::Type(ty) = angle_bracketed_args.args.first()? {
Some(ty)
} else {
None
}
}

pub(crate) fn unwrap_reference(ty: &TypePath, search_for_rvalue: bool) -> Option<&syn::TypePtr> {
match unwrap_bindgen_marker(ty, marker_for_reference(search_for_rvalue)) {
match unwrap_newtype(
ty.path.segments.first()?,
marker_for_reference(search_for_rvalue),
) {
// Our behavior here if we see __bindgen_marker_Reference <something that isn't a pointer>
// is to ignore the type. This should never happen.
Some(Type::Ptr(typ)) => Some(typ),
Expand All @@ -102,9 +104,28 @@ pub(crate) fn unwrap_reference(ty: &TypePath, search_for_rvalue: bool) -> Option
}

pub(crate) fn unwrap_has_unused_template_param(ty: &TypePath) -> Option<&syn::Type> {
unwrap_bindgen_marker(ty, "__bindgen_marker_UnusedTemplateParam")
unwrap_newtype(
ty.path.segments.first()?,
"__bindgen_marker_UnusedTemplateParam",
)
}

pub(crate) fn unwrap_has_opaque(ty: &TypePath) -> Option<&syn::Type> {
unwrap_bindgen_marker(ty, "__bindgen_marker_Opaque")
unwrap_newtype(ty.path.segments.first()?, "__bindgen_marker_Opaque")
}

pub(crate) fn unwrap_bitfield(ty: &TypePath) -> Option<&syn::Type> {
let mut segs = ty.path.segments.iter();

if segs.next()?.ident != "root" {
return None;
}

let inner = unwrap_newtype(segs.next()?, "__BindgenBitfieldUnit");

if segs.next().is_some() {
return None;
}

inner
}
1 change: 1 addition & 0 deletions engine/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

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.

.default_enum_style(bindgen::EnumVariation::Rust {
non_exhaustive: false,
})
Expand Down
10 changes: 3 additions & 7 deletions engine/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -236,10 +236,8 @@ impl std::fmt::Debug for QualifiedName {
/// cxx.
#[derive(Error, Clone, Debug)]
pub enum InvalidIdentError {
#[error("Union are not supported by autocxx (and their bindgen names have __ so are not acceptable to cxx)")]
#[error("Union are not supported by autocxx")]
Union,
#[error("Bitfields are not supported by autocxx (and their bindgen names have __ so are not acceptable to cxx)")]
Bitfield,
#[error("Names containing __ are reserved by C++ so not acceptable to cxx")]
TooManyUnderscores,
#[error("bindgen decided to call this type _bindgen_ty_N because it couldn't deduce the correct name for it. That means we can't generate C++ bindings to it.")]
Expand All @@ -255,10 +253,8 @@ pub enum InvalidIdentError {
/// where code will be output as part of the `#[cxx::bridge]` mod.
pub fn validate_ident_ok_for_cxx(id: &str) -> Result<(), InvalidIdentError> {
validate_str_ok_for_rust(id)?;
// Provide a couple of more specific diagnostics if we can.
if id.starts_with("__BindgenBitfieldUnit") {
Err(InvalidIdentError::Bitfield)
} else if id.starts_with("__BindgenUnionField") {
// Provide a more specific union diagnostic.
if id.starts_with("__BindgenUnionField") {
Err(InvalidIdentError::Union)
} else if id.contains("__") && !id.starts_with("__bindgen_marker") {
Err(InvalidIdentError::TooManyUnderscores)
Expand Down
88 changes: 88 additions & 0 deletions integration-tests/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12578,6 +12578,94 @@ fn test_opaque_directive() {
);
}

#[test]
fn test_take_bitfield() {
let cxx = indoc! {"
bool take_bitfield(Bitfieldy x) {
return (
x.unsigned3 == 2 &&
x.signed3 == -3 &&
x.bool1 == false &&
x.separator == 424242 &&
x.unsigned12 == 3000 &&
x.signed4 == -1
);
}
"};
let hdr = indoc! {"
#include <cstdint>
struct Bitfieldy {
uint8_t unsigned3 : 3;
int8_t signed3 : 3;
bool bool1 : 1;

uint32_t separator;

uint32_t unsigned12 : 12;
int32_t signed4 : 4;
};
bool take_bitfield(Bitfieldy);
"};
let rs = quote! {
let mut bitfield = ffi::Bitfieldy {
_bitfield_1: ffi::Bitfieldy::new_bitfield_1(2, -3, false),
separator: 424242,
..Default::default()
};

bitfield.set_unsigned12(3000);
bitfield.set_signed4(-1);

assert_eq!(ffi::take_bitfield(bitfield), true);
};
run_test(cxx, hdr, rs, &["take_bitfield"], &["Bitfieldy"]);
}

#[test]
fn test_give_bitfield() {
let cxx = indoc! {"
Bitfieldy give_bitfield() {
return {
.unsigned3 = 2,
.signed3 = -3,
.bool1 = false,

.separator = 424242,

.unsigned12 = 3000,
.signed4 = -1,
};
}
"};
let hdr = indoc! {"
#include <cstdint>
struct Bitfieldy {
uint8_t unsigned3 : 3;
int8_t signed3 : 3;
bool bool1 : 1;

uint32_t separator;

uint32_t unsigned12 : 12;
int32_t signed4 : 4;
};
Bitfieldy give_bitfield();
"};
let rs = quote! {
let bitfield = ffi::give_bitfield();

assert_eq!(bitfield.unsigned3(), 2);
// FIXME: Disabled until https://github.com/rust-lang/rust-bindgen/issues/1160 is fixed
//assert_eq!(bitfield.signed3(), -3);
assert_eq!(bitfield.bool1(), false);
assert_eq!(bitfield.separator, 424242);
assert_eq!(bitfield.unsigned12(), 3000);
// FIXME: Disabled until https://github.com/rust-lang/rust-bindgen/issues/1160 is fixed
//assert_eq!(bitfield.signed4(), -1);
};
run_test(cxx, hdr, rs, &["give_bitfield"], &["Bitfieldy"]);
}

// Yet to test:
// - Ifdef
// - Out param pointers
Expand Down
Loading