Repository: rust-lang/rust-bindgen
Pull request #2525 (2023-05-17 11:42:16)

Field visibility callback
Add a callback mechanism for determining field visibility. This PR includes 2 other related (breaking) behavior changes in the prior commits:

* *Use default visibility for padding fields* Previously, padding fields were always pub. Now, they follow the default visibility for the type they're in.
* *Compute visibility of bitfield unit based on actual field visibility* A bitfield unit field and its related functions now have their visibility determined based on the minimum of the default visibility and the actual visibility of the bitfields within the unit. private < pub(crate) < pub.

amanjeev: Hi @jethrogb thank you for this PR.  Could you please add a couple of tests showing the interaction of this work with the flag `--enable-cxx-namespaces`.  In our understanding that should take precedence and showing it does not break is nice, when used along with this one.  What do you think?
jethrogb: I assume you mean `respect-cxx-access-specs` instead of `enable-cxx-namespaces`. If you don't, could you please provide a code example of what you'd like tested?

I'm not sure what you mean by “that should take precedence and showing it does not break”, the callback takes precendence over whatever is inferred from the type, just like the annotations already do. I've added some tests in private_fields.hpp.
pvdrz: > I assume you mean `respect-cxx-access-specs` instead of `enable-cxx-namespaces`. If you don't, could you please provide a code example of what you'd like tested?
> 
> I'm not sure what you mean by “that should take precedence and showing it does not break”, the callback takes precendence over whatever is inferred from the type, just like the annotations already do. I've added some tests in private_fields.hpp.

Hmmm... Right now `default_visibility` is ignored if `respect_cxx_access_specs` is enabled (and if it isn't, we wrote the docs wrong :sweat_smile:). The big question here is if this callback should take precedence over both this options or not. I find two approaches that are reasonable from different points of view:
- `respect_cxx_access_specs` cannot be overriden if enabled: This makes sense because modifying the visibility of fields would let you access items that aren't part of the public API, which sounds like a recipe for bad things to happen.
- This callback can override `respect_cxx_access_specs`: This makes sense as it would be the most powerful option and you could argue that it is not `bindgen`'s business to tell users not to do stuff that could cause trouble.

I'm more inclined towards the first option but I'd be happy to hear an argument about any other option in any case.
jethrogb: As is apparent from https://github.com/rust-lang/rust-bindgen/pull/2525/commits/af66f4a39235a610c776bf459035be1cce9658c6, and as I explained in my comment, the behavior of overriding respect-cxx-access-specs is already there.
pvdrz: Oh OK I missed the annotations part. Yes, so this feature has the same precedence as the annotations. My question is if that's desirable or not because it somehow feels "wrong" to enable `--respect-cxx-access-specs` to just ignore it with this callback.

You could argue that annotations also ignore this but at least annotations are part of the input taken by `bindgen` which means the person who wrote the C code had the explicit intention of overriding the visibility of a field when running `bindgen` for these headers.

I'm not saying this reason is strong enough but it should be properly stated that anyone using this callback when `--respect-cxx-access-specs` is  "on their own" as they aren't protected of accessing fields they should not. Which I imagine is the original intent of `--respect-cxx-access-specs`.

I'll review the actual code in a bit and give you a proper review. Just wanted to clarify this point.
jethrogb: I've made the requested changes.

> it somehow feels "wrong" to enable --respect-cxx-access-specs to just ignore it with this callback.

People generally use these callbacks as a "last resort" when they find bindgen doesn't quite do what they wanted. As such I think it's correct to override the more generic mechanism.
pvdrz: Hey! the test suite broke due to an unrelated issue that has already been fixed in the main branch, could you rebase your PR against it so we can be sure the tests are passing?

Patch:
diff --git a/bindgen/callbacks.rs b/bindgen/callbacks.rs
index 58244668..96c1b198 100644
--- a/bindgen/callbacks.rs
+++ b/bindgen/callbacks.rs
@@ -135,6 +135,17 @@ pub trait ParseCallbacks: fmt::Debug {
     fn process_comment(&self, _comment: &str) -> Option<String> {
         None
     }
+
+    /// Potentially override the visibility of a composite type field.
+    ///
+    /// Caution: This allows overriding standard C++ visibility inferred by
+    /// `respect_cxx_access_specs`.
+    fn field_visibility(
+        &self,
+        _info: FieldInfo<'_>,
+    ) -> Option<crate::FieldVisibilityKind> {
+        None
+    }
 }
 
 /// Relevant information about a type to which new derive attributes will be added using
@@ -176,3 +187,14 @@ pub enum ItemKind {
     /// A Variable
     Var,
 }
+
+/// Relevant information about a field for which visibility can be determined using
+/// [`ParseCallbacks::field_visibility`].
+#[derive(Debug)]
+#[non_exhaustive]
+pub struct FieldInfo<'a> {
+    /// The name of the type.
+    pub type_name: &'a str,
+    /// The name of the field.
+    pub field_name: &'a str,
+}
diff --git a/bindgen/codegen/mod.rs b/bindgen/codegen/mod.rs
index 47f23193..c9cad86b 100644
--- a/bindgen/codegen/mod.rs
+++ b/bindgen/codegen/mod.rs
@@ -19,7 +19,7 @@ use self::struct_layout::StructLayoutTracker;
 
 use super::BindgenOptions;
 
-use crate::callbacks::{DeriveInfo, TypeKind as DeriveTypeKind};
+use crate::callbacks::{DeriveInfo, FieldInfo, TypeKind as DeriveTypeKind};
 use crate::ir::analysis::{HasVtable, Sizedness};
 use crate::ir::annotations::{
     Annotations, FieldAccessorKind, FieldVisibilityKind,
@@ -1291,6 +1291,7 @@ trait FieldCodegen<'a> {
         visibility_kind: FieldVisibilityKind,
         accessor_kind: FieldAccessorKind,
         parent: &CompInfo,
+        parent_item: &Item,
         result: &mut CodegenResult,
         struct_layout: &mut StructLayoutTracker,
         fields: &mut F,
@@ -1310,6 +1311,7 @@ impl<'a> FieldCodegen<'a> for Field {
         visibility_kind: FieldVisibilityKind,
         accessor_kind: FieldAccessorKind,
         parent: &CompInfo,
+        parent_item: &Item,
         result: &mut CodegenResult,
         struct_layout: &mut StructLayoutTracker,
         fields: &mut F,
@@ -1326,6 +1328,7 @@ impl<'a> FieldCodegen<'a> for Field {
                     visibility_kind,
                     accessor_kind,
                     parent,
+                    parent_item,
                     result,
                     struct_layout,
                     fields,
@@ -1339,6 +1342,7 @@ impl<'a> FieldCodegen<'a> for Field {
                     visibility_kind,
                     accessor_kind,
                     parent,
+                    parent_item,
                     result,
                     struct_layout,
                     fields,
@@ -1388,6 +1392,7 @@ impl<'a> FieldCodegen<'a> for FieldData {
         parent_visibility_kind: FieldVisibilityKind,
         accessor_kind: FieldAccessorKind,
         parent: &CompInfo,
+        parent_item: &Item,
         result: &mut CodegenResult,
         struct_layout: &mut StructLayoutTracker,
         fields: &mut F,
@@ -1440,10 +1445,11 @@ impl<'a> FieldCodegen<'a> for FieldData {
             .name()
             .map(|name| ctx.rust_mangle(name).into_owned())
             .expect("Each field should have a name in codegen!");
-        let field_ident = ctx.rust_ident_raw(field_name.as_str());
+        let field_name = field_name.as_str();
+        let field_ident = ctx.rust_ident_raw(field_name);
 
         if let Some(padding_field) =
-            struct_layout.saw_field(&field_name, field_ty, self.offset())
+            struct_layout.saw_field(field_name, field_ty, self.offset())
         {
             fields.extend(Some(padding_field));
         }
@@ -1451,7 +1457,13 @@ impl<'a> FieldCodegen<'a> for FieldData {
         let visibility = compute_visibility(
             ctx,
             self.is_public(),
-            Some(self.annotations()),
+            ctx.options().last_callback(|cb| {
+                cb.field_visibility(FieldInfo {
+                    type_name: &parent_item.canonical_name(ctx),
+                    field_name,
+                })
+            }),
+            self.annotations(),
             parent_visibility_kind,
         );
         let accessor_kind =
@@ -1485,7 +1497,6 @@ impl<'a> FieldCodegen<'a> for FieldData {
         let getter_name = ctx.rust_ident_raw(format!("get_{}", field_name));
         let mutable_getter_name =
             ctx.rust_ident_raw(format!("get_{}_mut", field_name));
-        let field_name = ctx.rust_ident_raw(field_name);
 
         methods.extend(Some(match accessor_kind {
             FieldAccessorKind::None => unreachable!(),
@@ -1493,12 +1504,12 @@ impl<'a> FieldCodegen<'a> for FieldData {
                 quote! {
                     #[inline]
                     pub fn #getter_name(&self) -> & #ty {
-                        &self.#field_name
+                        &self.#field_ident
                     }
 
                     #[inline]
                     pub fn #mutable_getter_name(&mut self) -> &mut #ty {
-                        &mut self.#field_name
+                        &mut self.#field_ident
                     }
                 }
             }
@@ -1506,12 +1517,12 @@ impl<'a> FieldCodegen<'a> for FieldData {
                 quote! {
                     #[inline]
                     pub unsafe fn #getter_name(&self) -> & #ty {
-                        &self.#field_name
+                        &self.#field_ident
                     }
 
                     #[inline]
                     pub unsafe fn #mutable_getter_name(&mut self) -> &mut #ty {
-                        &mut self.#field_name
+                        &mut self.#field_ident
                     }
                 }
             }
@@ -1519,7 +1530,7 @@ impl<'a> FieldCodegen<'a> for FieldData {
                 quote! {
                     #[inline]
                     pub fn #getter_name(&self) -> & #ty {
-                        &self.#field_name
+                        &self.#field_ident
                     }
                 }
             }
@@ -1604,27 +1615,28 @@ fn access_specifier(
 fn compute_visibility(
     ctx: &BindgenContext,
     is_declared_public: bool,
-    annotations: Option<&Annotations>,
+    callback_override: Option<FieldVisibilityKind>,
+    annotations: &Annotations,
     default_kind: FieldVisibilityKind,
 ) -> FieldVisibilityKind {
-    match (
-        is_declared_public,
-        ctx.options().respect_cxx_access_specs,
-        annotations.and_then(|e| e.visibility_kind()),
-    ) {
-        (true, true, annotated_visibility) => {
-            // declared as public, cxx specs are respected
-            annotated_visibility.unwrap_or(FieldVisibilityKind::Public)
-        }
-        (false, true, annotated_visibility) => {
-            // declared as private, cxx specs are respected
-            annotated_visibility.unwrap_or(FieldVisibilityKind::Private)
-        }
-        (_, false, annotated_visibility) => {
-            // cxx specs are not respected, declaration does not matter.
-            annotated_visibility.unwrap_or(default_kind)
-        }
-    }
+    callback_override
+        .or_else(|| annotations.visibility_kind())
+        .unwrap_or_else(|| {
+            match (is_declared_public, ctx.options().respect_cxx_access_specs) {
+                (true, true) => {
+                    // declared as public, cxx specs are respected
+                    FieldVisibilityKind::Public
+                }
+                (false, true) => {
+                    // declared as private, cxx specs are respected
+                    FieldVisibilityKind::Private
+                }
+                (_, false) => {
+                    // cxx specs are not respected, declaration does not matter.
+                    default_kind
+                }
+            }
+        })
 }
 
 impl<'a> FieldCodegen<'a> for BitfieldUnit {
@@ -1636,6 +1648,7 @@ impl<'a> FieldCodegen<'a> for BitfieldUnit {
         visibility_kind: FieldVisibilityKind,
         accessor_kind: FieldAccessorKind,
         parent: &CompInfo,
+        parent_item: &Item,
         result: &mut CodegenResult,
         struct_layout: &mut StructLayoutTracker,
         fields: &mut F,
@@ -1695,7 +1708,7 @@ impl<'a> FieldCodegen<'a> for BitfieldUnit {
         // the 32 items limitation.
         let mut generate_ctor = layout.size <= RUST_DERIVE_IN_ARRAY_LIMIT;
 
-        let mut all_fields_declared_as_public = true;
+        let mut unit_visibility = visibility_kind;
         for bf in self.bitfields() {
             // Codegen not allowed for anonymous bitfields
             if bf.name().is_none() {
@@ -1708,19 +1721,27 @@ impl<'a> FieldCodegen<'a> for BitfieldUnit {
                 continue;
             }
 
-            all_fields_declared_as_public &= bf.is_public();
             let mut bitfield_representable_as_int = true;
+            let mut bitfield_visibility = visibility_kind;
             bf.codegen(
                 ctx,
                 visibility_kind,
                 accessor_kind,
                 parent,
+                parent_item,
                 result,
                 struct_layout,
                 fields,
                 methods,
-                (&unit_field_name, &mut bitfield_representable_as_int),
+                (
+                    &unit_field_name,
+                    &mut bitfield_representable_as_int,
+                    &mut bitfield_visibility,
+                ),
             );
+            if bitfield_visibility < unit_visibility {
+                unit_visibility = bitfield_visibility;
+            }
 
             // Generating a constructor requires the bitfield to be representable as an integer.
             if !bitfield_representable_as_int {
@@ -1740,13 +1761,7 @@ impl<'a> FieldCodegen<'a> for BitfieldUnit {
             ctor_impl = bf.extend_ctor_impl(ctx, param_name, ctor_impl);
         }
 
-        let visibility_kind = compute_visibility(
-            ctx,
-            all_fields_declared_as_public,
-            None,
-            visibility_kind,
-        );
-        let access_spec = access_specifier(visibility_kind);
+        let access_spec = access_specifier(unit_visibility);
 
         let field = quote! {
             #access_spec #unit_field_ident : #field_ty ,
@@ -1787,7 +1802,7 @@ fn bitfield_setter_name(
 }
 
 impl<'a> FieldCodegen<'a> for Bitfield {
-    type Extra = (&'a str, &'a mut bool);
+    type Extra = (&'a str, &'a mut bool, &'a mut FieldVisibilityKind);
 
     fn codegen<F, M>(
         &self,
@@ -1795,11 +1810,16 @@ impl<'a> FieldCodegen<'a> for Bitfield {
         visibility_kind: FieldVisibilityKind,
         _accessor_kind: FieldAccessorKind,
         parent: &CompInfo,
+        parent_item: &Item,
         _result: &mut CodegenResult,
         struct_layout: &mut StructLayoutTracker,
         _fields: &mut F,
         methods: &mut M,
-        (unit_field_name, bitfield_representable_as_int): (&'a str, &mut bool),
+        (unit_field_name, bitfield_representable_as_int, bitfield_visibility): (
+            &'a str,
+            &mut bool,
+            &'a mut FieldVisibilityKind,
+        ),
     ) where
         F: Extend<proc_macro2::TokenStream>,
         M: Extend<proc_macro2::TokenStream>,
@@ -1833,13 +1853,22 @@ impl<'a> FieldCodegen<'a> for Bitfield {
         let offset = self.offset_into_unit();
         let width = self.width() as u8;
 
-        let visibility_kind = compute_visibility(
+        let override_visibility = self.name().and_then(|field_name| {
+            ctx.options().last_callback(|cb| {
+                cb.field_visibility(FieldInfo {
+                    type_name: &parent_item.canonical_name(ctx),
+                    field_name,
+                })
+            })
+        });
+        *bitfield_visibility = compute_visibility(
             ctx,
             self.is_public(),
-            Some(self.annotations()),
+            override_visibility,
+            self.annotations(),
             visibility_kind,
         );
-        let access_spec = access_specifier(visibility_kind);
+        let access_spec = access_specifier(*bitfield_visibility);
 
         if parent.is_union() && !struct_layout.is_rust_union() {
             methods.extend(Some(quote! {
@@ -1933,8 +1962,17 @@ impl CodeGenerator for CompInfo {
         // the parent too.
         let is_opaque = item.is_opaque(ctx, &());
         let mut fields = vec![];
-        let mut struct_layout =
-            StructLayoutTracker::new(ctx, self, ty, &canonical_name);
+        let visibility = item
+            .annotations()
+            .visibility_kind()
+            .unwrap_or(ctx.options().default_visibility);
+        let mut struct_layout = StructLayoutTracker::new(
+            ctx,
+            self,
+            ty,
+            &canonical_name,
+            visibility,
+        );
 
         if !is_opaque {
             if item.has_vtable_ptr(ctx) {
@@ -1983,10 +2021,6 @@ impl CodeGenerator for CompInfo {
 
         let mut methods = vec![];
         if !is_opaque {
-            let visibility = item
-                .annotations()
-                .visibility_kind()
-                .unwrap_or(ctx.options().default_visibility);
             let struct_accessor_kind = item
                 .annotations()
                 .accessor_kind()
@@ -1997,6 +2031,7 @@ impl CodeGenerator for CompInfo {
                     visibility,
                     struct_accessor_kind,
                     self,
+                    item,
                     result,
                     &mut struct_layout,
                     &mut fields,
diff --git a/bindgen/codegen/struct_layout.rs b/bindgen/codegen/struct_layout.rs
index cca4a59b..56730603 100644
--- a/bindgen/codegen/struct_layout.rs
+++ b/bindgen/codegen/struct_layout.rs
@@ -6,6 +6,7 @@ use crate::ir::comp::CompInfo;
 use crate::ir::context::BindgenContext;
 use crate::ir::layout::Layout;
 use crate::ir::ty::{Type, TypeKind};
+use crate::FieldVisibilityKind;
 use proc_macro2::{self, Ident, Span};
 use std::cmp;
 
@@ -26,6 +27,7 @@ pub(crate) struct StructLayoutTracker<'a> {
     latest_field_layout: Option<Layout>,
     max_field_align: usize,
     last_field_was_bitfield: bool,
+    visibility: FieldVisibilityKind,
 }
 
 /// Returns a size aligned to a given value.
@@ -88,6 +90,7 @@ impl<'a> StructLayoutTracker<'a> {
         comp: &'a CompInfo,
         ty: &'a Type,
         name: &'a str,
+        visibility: FieldVisibilityKind,
     ) -> Self {
         let known_type_layout = ty.layout(ctx);
         let is_packed = comp.is_packed(ctx, known_type_layout.as_ref());
@@ -97,6 +100,7 @@ impl<'a> StructLayoutTracker<'a> {
             name,
             ctx,
             comp,
+            visibility,
             is_packed,
             known_type_layout,
             is_rust_union,
@@ -397,8 +401,10 @@ impl<'a> StructLayoutTracker<'a> {
 
         self.max_field_align = cmp::max(self.max_field_align, layout.align);
 
+        let vis = super::access_specifier(self.visibility);
+
         quote! {
-            pub #padding_field_name : #ty ,
+            #vis #padding_field_name : #ty ,
         }
     }
 
diff --git a/bindgen/ir/annotations.rs b/bindgen/ir/annotations.rs
index 423f6c4b..d085f5c5 100644
--- a/bindgen/ir/annotations.rs
+++ b/bindgen/ir/annotations.rs
@@ -9,7 +9,7 @@ use std::str::FromStr;
 use crate::clang;
 
 /// What kind of visibility modifer should be used for a struct or field?
-#[derive(Copy, PartialEq, Eq, Clone, Debug)]
+#[derive(Copy, PartialEq, Eq, PartialOrd, Ord, Clone, Debug)]
 pub enum FieldVisibilityKind {
     /// Fields are marked as private, i.e., struct Foo {bar: bool}
     Private,


Test patch:
diff --git a/bindgen-tests/tests/expectations/tests/default_visibility_private_respects_cxx_access_spec.rs b/bindgen-tests/tests/expectations/tests/default_visibility_private_respects_cxx_access_spec.rs
index d38790f3..453a3054 100644
--- a/bindgen-tests/tests/expectations/tests/default_visibility_private_respects_cxx_access_spec.rs
+++ b/bindgen-tests/tests/expectations/tests/default_visibility_private_respects_cxx_access_spec.rs
@@ -93,7 +93,7 @@ pub struct Point {
 #[derive(Debug, Default, Copy, Clone)]
 pub struct Color {
     _bitfield_align_1: [u8; 0],
-    pub _bitfield_1: __BindgenBitfieldUnit<[u8; 1usize]>,
+    _bitfield_1: __BindgenBitfieldUnit<[u8; 1usize]>,
 }
 impl Color {
     #[inline]
@@ -130,7 +130,7 @@ impl Color {
         }
     }
     #[inline]
-    pub fn new_bitfield_1(
+    fn new_bitfield_1(
         r: ::std::os::raw::c_char,
         g: ::std::os::raw::c_char,
         b: ::std::os::raw::c_char,
diff --git a/bindgen-tests/tests/expectations/tests/field-visibility-callback.rs b/bindgen-tests/tests/expectations/tests/field-visibility-callback.rs
new file mode 100644
index 00000000..9411ccc7
--- /dev/null
+++ b/bindgen-tests/tests/expectations/tests/field-visibility-callback.rs
@@ -0,0 +1,166 @@
+#![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)]
+#[repr(C)]
+#[derive(Copy, Clone, Debug, Default, Eq, Hash, Ord, PartialEq, PartialOrd)]
+pub struct __BindgenBitfieldUnit<Storage> {
+    storage: Storage,
+}
+impl<Storage> __BindgenBitfieldUnit<Storage> {
+    #[inline]
+    pub const fn new(storage: Storage) -> Self {
+        Self { storage }
+    }
+}
+impl<Storage> __BindgenBitfieldUnit<Storage>
+where
+    Storage: AsRef<[u8]> + AsMut<[u8]>,
+{
+    #[inline]
+    pub fn get_bit(&self, index: usize) -> bool {
+        debug_assert!(index / 8 < self.storage.as_ref().len());
+        let byte_index = index / 8;
+        let byte = self.storage.as_ref()[byte_index];
+        let bit_index = if cfg!(target_endian = "big") {
+            7 - (index % 8)
+        } else {
+            index % 8
+        };
+        let mask = 1 << bit_index;
+        byte & mask == mask
+    }
+    #[inline]
+    pub fn set_bit(&mut self, index: usize, val: bool) {
+        debug_assert!(index / 8 < self.storage.as_ref().len());
+        let byte_index = index / 8;
+        let byte = &mut self.storage.as_mut()[byte_index];
+        let bit_index = if cfg!(target_endian = "big") {
+            7 - (index % 8)
+        } else {
+            index % 8
+        };
+        let mask = 1 << bit_index;
+        if val {
+            *byte |= mask;
+        } else {
+            *byte &= !mask;
+        }
+    }
+    #[inline]
+    pub fn get(&self, bit_offset: usize, bit_width: u8) -> u64 {
+        debug_assert!(bit_width <= 64);
+        debug_assert!(bit_offset / 8 < self.storage.as_ref().len());
+        debug_assert!(
+            (bit_offset + (bit_width as usize)) / 8 <= self.storage.as_ref().len()
+        );
+        let mut val = 0;
+        for i in 0..(bit_width as usize) {
+            if self.get_bit(i + bit_offset) {
+                let index = if cfg!(target_endian = "big") {
+                    bit_width as usize - 1 - i
+                } else {
+                    i
+                };
+                val |= 1 << index;
+            }
+        }
+        val
+    }
+    #[inline]
+    pub fn set(&mut self, bit_offset: usize, bit_width: u8, val: u64) {
+        debug_assert!(bit_width <= 64);
+        debug_assert!(bit_offset / 8 < self.storage.as_ref().len());
+        debug_assert!(
+            (bit_offset + (bit_width as usize)) / 8 <= self.storage.as_ref().len()
+        );
+        for i in 0..(bit_width as usize) {
+            let mask = 1 << i;
+            let val_bit_is_set = val & mask == mask;
+            let index = if cfg!(target_endian = "big") {
+                bit_width as usize - 1 - i
+            } else {
+                i
+            };
+            self.set_bit(index + bit_offset, val_bit_is_set);
+        }
+    }
+}
+#[repr(C)]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct my_struct {
+    pub a: ::std::os::raw::c_int,
+    private_b: ::std::os::raw::c_int,
+    _bitfield_align_1: [u8; 0],
+    _bitfield_1: __BindgenBitfieldUnit<[u8; 1usize]>,
+    __bindgen_padding_0: [u8; 3usize],
+}
+#[test]
+fn bindgen_test_layout_my_struct() {
+    const UNINIT: ::std::mem::MaybeUninit<my_struct> = ::std::mem::MaybeUninit::uninit();
+    let ptr = UNINIT.as_ptr();
+    assert_eq!(
+        ::std::mem::size_of:: < my_struct > (), 12usize, concat!("Size of: ",
+        stringify!(my_struct))
+    );
+    assert_eq!(
+        ::std::mem::align_of:: < my_struct > (), 4usize, concat!("Alignment of ",
+        stringify!(my_struct))
+    );
+    assert_eq!(
+        unsafe { ::std::ptr::addr_of!((* ptr).a) as usize - ptr as usize }, 0usize,
+        concat!("Offset of field: ", stringify!(my_struct), "::", stringify!(a))
+    );
+    assert_eq!(
+        unsafe { ::std::ptr::addr_of!((* ptr).private_b) as usize - ptr as usize },
+        4usize, concat!("Offset of field: ", stringify!(my_struct), "::",
+        stringify!(private_b))
+    );
+}
+impl my_struct {
+    #[inline]
+    pub fn c(&self) -> ::std::os::raw::c_int {
+        unsafe { ::std::mem::transmute(self._bitfield_1.get(0usize, 1u8) as u32) }
+    }
+    #[inline]
+    pub fn set_c(&mut self, val: ::std::os::raw::c_int) {
+        unsafe {
+            let val: u32 = ::std::mem::transmute(val);
+            self._bitfield_1.set(0usize, 1u8, val as u64)
+        }
+    }
+    #[inline]
+    fn private_d(&self) -> ::std::os::raw::c_int {
+        unsafe { ::std::mem::transmute(self._bitfield_1.get(1usize, 1u8) as u32) }
+    }
+    #[inline]
+    fn set_private_d(&mut self, val: ::std::os::raw::c_int) {
+        unsafe {
+            let val: u32 = ::std::mem::transmute(val);
+            self._bitfield_1.set(1usize, 1u8, val as u64)
+        }
+    }
+    #[inline]
+    fn new_bitfield_1(
+        c: ::std::os::raw::c_int,
+        private_d: ::std::os::raw::c_int,
+    ) -> __BindgenBitfieldUnit<[u8; 1usize]> {
+        let mut __bindgen_bitfield_unit: __BindgenBitfieldUnit<[u8; 1usize]> = Default::default();
+        __bindgen_bitfield_unit
+            .set(
+                0usize,
+                1u8,
+                {
+                    let c: u32 = unsafe { ::std::mem::transmute(c) };
+                    c as u64
+                },
+            );
+        __bindgen_bitfield_unit
+            .set(
+                1usize,
+                1u8,
+                {
+                    let private_d: u32 = unsafe { ::std::mem::transmute(private_d) };
+                    private_d as u64
+                },
+            );
+        __bindgen_bitfield_unit
+    }
+}
diff --git a/bindgen-tests/tests/expectations/tests/field-visibility.rs b/bindgen-tests/tests/expectations/tests/field-visibility.rs
new file mode 100644
index 00000000..b4e06208
--- /dev/null
+++ b/bindgen-tests/tests/expectations/tests/field-visibility.rs
@@ -0,0 +1,179 @@
+#![allow(dead_code, non_snake_case, non_camel_case_types, non_upper_case_globals)]
+#[repr(C)]
+#[derive(Copy, Clone, Debug, Default, Eq, Hash, Ord, PartialEq, PartialOrd)]
+pub struct __BindgenBitfieldUnit<Storage> {
+    storage: Storage,
+}
+impl<Storage> __BindgenBitfieldUnit<Storage> {
+    #[inline]
+    pub const fn new(storage: Storage) -> Self {
+        Self { storage }
+    }
+}
+impl<Storage> __BindgenBitfieldUnit<Storage>
+where
+    Storage: AsRef<[u8]> + AsMut<[u8]>,
+{
+    #[inline]
+    pub fn get_bit(&self, index: usize) -> bool {
+        debug_assert!(index / 8 < self.storage.as_ref().len());
+        let byte_index = index / 8;
+        let byte = self.storage.as_ref()[byte_index];
+        let bit_index = if cfg!(target_endian = "big") {
+            7 - (index % 8)
+        } else {
+            index % 8
+        };
+        let mask = 1 << bit_index;
+        byte & mask == mask
+    }
+    #[inline]
+    pub fn set_bit(&mut self, index: usize, val: bool) {
+        debug_assert!(index / 8 < self.storage.as_ref().len());
+        let byte_index = index / 8;
+        let byte = &mut self.storage.as_mut()[byte_index];
+        let bit_index = if cfg!(target_endian = "big") {
+            7 - (index % 8)
+        } else {
+            index % 8
+        };
+        let mask = 1 << bit_index;
+        if val {
+            *byte |= mask;
+        } else {
+            *byte &= !mask;
+        }
+    }
+    #[inline]
+    pub fn get(&self, bit_offset: usize, bit_width: u8) -> u64 {
+        debug_assert!(bit_width <= 64);
+        debug_assert!(bit_offset / 8 < self.storage.as_ref().len());
+        debug_assert!(
+            (bit_offset + (bit_width as usize)) / 8 <= self.storage.as_ref().len()
+        );
+        let mut val = 0;
+        for i in 0..(bit_width as usize) {
+            if self.get_bit(i + bit_offset) {
+                let index = if cfg!(target_endian = "big") {
+                    bit_width as usize - 1 - i
+                } else {
+                    i
+                };
+                val |= 1 << index;
+            }
+        }
+        val
+    }
+    #[inline]
+    pub fn set(&mut self, bit_offset: usize, bit_width: u8, val: u64) {
+        debug_assert!(bit_width <= 64);
+        debug_assert!(bit_offset / 8 < self.storage.as_ref().len());
+        debug_assert!(
+            (bit_offset + (bit_width as usize)) / 8 <= self.storage.as_ref().len()
+        );
+        for i in 0..(bit_width as usize) {
+            let mask = 1 << i;
+            let val_bit_is_set = val & mask == mask;
+            let index = if cfg!(target_endian = "big") {
+                bit_width as usize - 1 - i
+            } else {
+                i
+            };
+            self.set_bit(index + bit_offset, val_bit_is_set);
+        }
+    }
+}
+#[repr(C)]
+#[repr(align(4))]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct my_struct1 {
+    _bitfield_align_1: [u8; 0],
+    _bitfield_1: __BindgenBitfieldUnit<[u8; 1usize]>,
+    __bindgen_padding_0: [u8; 3usize],
+}
+#[test]
+fn bindgen_test_layout_my_struct1() {
+    assert_eq!(
+        ::std::mem::size_of:: < my_struct1 > (), 4usize, concat!("Size of: ",
+        stringify!(my_struct1))
+    );
+    assert_eq!(
+        ::std::mem::align_of:: < my_struct1 > (), 4usize, concat!("Alignment of ",
+        stringify!(my_struct1))
+    );
+}
+impl my_struct1 {
+    #[inline]
+    fn a(&self) -> ::std::os::raw::c_int {
+        unsafe { ::std::mem::transmute(self._bitfield_1.get(0usize, 1u8) as u32) }
+    }
+    #[inline]
+    fn set_a(&mut self, val: ::std::os::raw::c_int) {
+        unsafe {
+            let val: u32 = ::std::mem::transmute(val);
+            self._bitfield_1.set(0usize, 1u8, val as u64)
+        }
+    }
+    #[inline]
+    fn new_bitfield_1(a: ::std::os::raw::c_int) -> __BindgenBitfieldUnit<[u8; 1usize]> {
+        let mut __bindgen_bitfield_unit: __BindgenBitfieldUnit<[u8; 1usize]> = Default::default();
+        __bindgen_bitfield_unit
+            .set(
+                0usize,
+                1u8,
+                {
+                    let a: u32 = unsafe { ::std::mem::transmute(a) };
+                    a as u64
+                },
+            );
+        __bindgen_bitfield_unit
+    }
+}
+#[repr(C)]
+#[repr(align(4))]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct my_struct2 {
+    pub _bitfield_align_1: [u8; 0],
+    pub _bitfield_1: __BindgenBitfieldUnit<[u8; 1usize]>,
+    pub __bindgen_padding_0: [u8; 3usize],
+}
+#[test]
+fn bindgen_test_layout_my_struct2() {
+    assert_eq!(
+        ::std::mem::size_of:: < my_struct2 > (), 4usize, concat!("Size of: ",
+        stringify!(my_struct2))
+    );
+    assert_eq!(
+        ::std::mem::align_of:: < my_struct2 > (), 4usize, concat!("Alignment of ",
+        stringify!(my_struct2))
+    );
+}
+impl my_struct2 {
+    #[inline]
+    pub fn a(&self) -> ::std::os::raw::c_int {
+        unsafe { ::std::mem::transmute(self._bitfield_1.get(0usize, 1u8) as u32) }
+    }
+    #[inline]
+    pub fn set_a(&mut self, val: ::std::os::raw::c_int) {
+        unsafe {
+            let val: u32 = ::std::mem::transmute(val);
+            self._bitfield_1.set(0usize, 1u8, val as u64)
+        }
+    }
+    #[inline]
+    pub fn new_bitfield_1(
+        a: ::std::os::raw::c_int,
+    ) -> __BindgenBitfieldUnit<[u8; 1usize]> {
+        let mut __bindgen_bitfield_unit: __BindgenBitfieldUnit<[u8; 1usize]> = Default::default();
+        __bindgen_bitfield_unit
+            .set(
+                0usize,
+                1u8,
+                {
+                    let a: u32 = unsafe { ::std::mem::transmute(a) };
+                    a as u64
+                },
+            );
+        __bindgen_bitfield_unit
+    }
+}
diff --git a/bindgen-tests/tests/expectations/tests/private_fields.rs b/bindgen-tests/tests/expectations/tests/private_fields.rs
index 3408a9a3..120ed858 100644
--- a/bindgen-tests/tests/expectations/tests/private_fields.rs
+++ b/bindgen-tests/tests/expectations/tests/private_fields.rs
@@ -483,3 +483,113 @@ impl Default for WithAnonUnion {
         }
     }
 }
+#[repr(C)]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct Override {
+    pub a: ::std::os::raw::c_uint,
+    /// <div rustbindgen private></div>
+    b: ::std::os::raw::c_uint,
+    private_c: ::std::os::raw::c_uint,
+    pub _bitfield_align_1: [u8; 0],
+    _bitfield_1: __BindgenBitfieldUnit<[u8; 2usize]>,
+    pub __bindgen_padding_0: u16,
+}
+#[test]
+fn bindgen_test_layout_Override() {
+    const UNINIT: ::std::mem::MaybeUninit<Override> = ::std::mem::MaybeUninit::uninit();
+    let ptr = UNINIT.as_ptr();
+    assert_eq!(
+        ::std::mem::size_of:: < Override > (), 16usize, concat!("Size of: ",
+        stringify!(Override))
+    );
+    assert_eq!(
+        ::std::mem::align_of:: < Override > (), 4usize, concat!("Alignment of ",
+        stringify!(Override))
+    );
+    assert_eq!(
+        unsafe { ::std::ptr::addr_of!((* ptr).a) as usize - ptr as usize }, 0usize,
+        concat!("Offset of field: ", stringify!(Override), "::", stringify!(a))
+    );
+    assert_eq!(
+        unsafe { ::std::ptr::addr_of!((* ptr).b) as usize - ptr as usize }, 4usize,
+        concat!("Offset of field: ", stringify!(Override), "::", stringify!(b))
+    );
+    assert_eq!(
+        unsafe { ::std::ptr::addr_of!((* ptr).private_c) as usize - ptr as usize },
+        8usize, concat!("Offset of field: ", stringify!(Override), "::",
+        stringify!(private_c))
+    );
+}
+impl Override {
+    #[inline]
+    pub fn bf_a(&self) -> ::std::os::raw::c_uint {
+        unsafe { ::std::mem::transmute(self._bitfield_1.get(0usize, 4u8) as u32) }
+    }
+    #[inline]
+    pub fn set_bf_a(&mut self, val: ::std::os::raw::c_uint) {
+        unsafe {
+            let val: u32 = ::std::mem::transmute(val);
+            self._bitfield_1.set(0usize, 4u8, val as u64)
+        }
+    }
+    #[inline]
+    fn bf_b(&self) -> ::std::os::raw::c_uint {
+        unsafe { ::std::mem::transmute(self._bitfield_1.get(4usize, 4u8) as u32) }
+    }
+    #[inline]
+    fn set_bf_b(&mut self, val: ::std::os::raw::c_uint) {
+        unsafe {
+            let val: u32 = ::std::mem::transmute(val);
+            self._bitfield_1.set(4usize, 4u8, val as u64)
+        }
+    }
+    #[inline]
+    fn private_bf_c(&self) -> ::std::os::raw::c_uint {
+        unsafe { ::std::mem::transmute(self._bitfield_1.get(8usize, 4u8) as u32) }
+    }
+    #[inline]
+    fn set_private_bf_c(&mut self, val: ::std::os::raw::c_uint) {
+        unsafe {
+            let val: u32 = ::std::mem::transmute(val);
+            self._bitfield_1.set(8usize, 4u8, val as u64)
+        }
+    }
+    #[inline]
+    fn new_bitfield_1(
+        bf_a: ::std::os::raw::c_uint,
+        bf_b: ::std::os::raw::c_uint,
+        private_bf_c: ::std::os::raw::c_uint,
+    ) -> __BindgenBitfieldUnit<[u8; 2usize]> {
+        let mut __bindgen_bitfield_unit: __BindgenBitfieldUnit<[u8; 2usize]> = Default::default();
+        __bindgen_bitfield_unit
+            .set(
+                0usize,
+                4u8,
+                {
+                    let bf_a: u32 = unsafe { ::std::mem::transmute(bf_a) };
+                    bf_a as u64
+                },
+            );
+        __bindgen_bitfield_unit
+            .set(
+                4usize,
+                4u8,
+                {
+                    let bf_b: u32 = unsafe { ::std::mem::transmute(bf_b) };
+                    bf_b as u64
+                },
+            );
+        __bindgen_bitfield_unit
+            .set(
+                8usize,
+                4u8,
+                {
+                    let private_bf_c: u32 = unsafe {
+                        ::std::mem::transmute(private_bf_c)
+                    };
+                    private_bf_c as u64
+                },
+            );
+        __bindgen_bitfield_unit
+    }
+}
diff --git a/bindgen-tests/tests/headers/field-visibility-callback.h b/bindgen-tests/tests/headers/field-visibility-callback.h
new file mode 100644
index 00000000..d2fe3ace
--- /dev/null
+++ b/bindgen-tests/tests/headers/field-visibility-callback.h
@@ -0,0 +1,9 @@
+// bindgen-flags: --default-visibility private
+// bindgen-parse-callbacks: field-visibility-default-private
+
+struct my_struct {
+    int a;
+    int private_b;
+    int c: 1;
+    int private_d: 1;
+};
diff --git a/bindgen-tests/tests/headers/field-visibility.h b/bindgen-tests/tests/headers/field-visibility.h
new file mode 100644
index 00000000..adb73159
--- /dev/null
+++ b/bindgen-tests/tests/headers/field-visibility.h
@@ -0,0 +1,10 @@
+// bindgen-flags: --default-visibility private --no-doc-comments
+
+struct my_struct1 {
+    int a: 1;
+};
+
+/** <div rustbindgen private="false"></div> */
+struct my_struct2 {
+    int a: 1;
+};
diff --git a/bindgen-tests/tests/headers/private_fields.hpp b/bindgen-tests/tests/headers/private_fields.hpp
index 9d55ebca..965acde9 100644
--- a/bindgen-tests/tests/headers/private_fields.hpp
+++ b/bindgen-tests/tests/headers/private_fields.hpp
@@ -1,4 +1,6 @@
 // bindgen-flags: --respect-cxx-access-specs
+// bindgen-parse-callbacks: field-visibility-default-public
+
 class PubPriv {
   public:
     int x;
@@ -41,4 +43,21 @@ class WithAnonStruct {
 
 class WithAnonUnion {
   union {};
-};
\ No newline at end of file
+};
+
+class Override {
+ public:
+  unsigned int a;
+  // override with annotation
+  /** <div rustbindgen private></div> */
+  unsigned int b;
+  // override with callback
+  unsigned int private_c;
+
+  unsigned int bf_a : 4;
+  // override with annotation
+  /** <div rustbindgen private></div> */
+  unsigned int bf_b : 4;
+  // override with callback
+  unsigned int private_bf_c : 4;
+};
diff --git a/bindgen-tests/tests/parse_callbacks/mod.rs b/bindgen-tests/tests/parse_callbacks/mod.rs
index 85082b63..fe4c4e5c 100644
--- a/bindgen-tests/tests/parse_callbacks/mod.rs
+++ b/bindgen-tests/tests/parse_callbacks/mod.rs
@@ -1,4 +1,5 @@
 use bindgen::callbacks::*;
+use bindgen::FieldVisibilityKind;
 
 #[derive(Debug)]
 pub struct RemovePrefixParseCallback {
@@ -86,25 +87,65 @@ impl ParseCallbacks for BlocklistedTypeImplementsTrait {
     }
 }
 
+#[derive(Debug)]
+struct FieldVisibility {
+    default: FieldVisibilityKind,
+}
+
+/// Implements the `field_visibility` function of the trait by checking if the
+/// field name starts with `private_`. If it does it makes it private, if it
+/// doesn't it makes it public, taking into account the default visibility.
+impl ParseCallbacks for FieldVisibility {
+    fn field_visibility(
+        &self,
+        FieldInfo { field_name, .. }: FieldInfo,
+    ) -> Option<FieldVisibilityKind> {
+        match (self.default, field_name.starts_with("private_")) {
+            (FieldVisibilityKind::Private, false) => {
+                Some(FieldVisibilityKind::Public)
+            }
+            (FieldVisibilityKind::Public, true) => {
+                Some(FieldVisibilityKind::Private)
+            }
+            (FieldVisibilityKind::PublicCrate, _) => unimplemented!(),
+            _ => None,
+        }
+    }
+}
+
 pub fn lookup(cb: &str) -> Box<dyn ParseCallbacks> {
+    fn try_strip_prefix<'a>(s: &'a str, prefix: &str) -> Option<&'a str> {
+        if s.starts_with(prefix) {
+            Some(&s[prefix.len()..])
+        } else {
+            None
+        }
+    }
+
     match cb {
         "enum-variant-rename" => Box::new(EnumVariantRename),
         "blocklisted-type-implements-trait" => {
             Box::new(BlocklistedTypeImplementsTrait)
         }
         call_back => {
-            if call_back.starts_with("remove-function-prefix-") {
-                let prefix = call_back
-                    .split("remove-function-prefix-")
-                    .last()
-                    .to_owned();
-                let lnopc = RemovePrefixParseCallback::new(prefix.unwrap());
+            if let Some(prefix) =
+                try_strip_prefix(call_back, "remove-function-prefix-")
+            {
+                let lnopc = RemovePrefixParseCallback::new(prefix);
                 Box::new(lnopc)
-            } else if call_back.starts_with("prefix-link-name-") {
-                let prefix =
-                    call_back.split("prefix-link-name-").last().to_owned();
-                let plnpc = PrefixLinkNameParseCallback::new(prefix.unwrap());
+            } else if let Some(prefix) =
+                try_strip_prefix(call_back, "prefix-link-name-")
+            {
+                let plnpc = PrefixLinkNameParseCallback::new(prefix);
                 Box::new(plnpc)
+            } else if let Some(default) =
+                try_strip_prefix(call_back, "field-visibility-default-")
+            {
+                Box::new(FieldVisibility {
+                    default: default.parse().expect(
+                        "unable to parse field-visibility-default callback",
+                    ),
+                })
             } else {
                 panic!("Couldn't find name ParseCallbacks: {}", cb)
             }