Repository: rust-lang/rust-bindgen
Pull request #2734 (2024-01-26 00:40:01)

try to avoid `#[repr(packed)]` when `align` is needed
This is for https://github.com/rust-lang/rust-bindgen/issues/2725

This patch handles the "type has both packed and align attrs" case, but it doesn't do anything for the "packed type transitively contains aligned type" case.

The test header intentionally generates types that won't compile. If this is a problem, and the test output rust source needs to compile, let me know and I'll remove those types...

I'm still pretty new to Rust, so a thorough review would be much appreciated! 

-------

Currently rustc forbids compound types from having both a `packed` and `align` attribute.

When a source type has both attributes, this may mean it cannot be represented with the current rustc. Often, though, one or both of these attributes is redundant and can be safely removed from the generated Rust code.

Previously, bindgen avoided placing the `align` attribute when it is not needed. However, it would always place the `packed` attribute if the source type has it, even when it is redundant because the source type is "naturally packed".

With this change, bindgen avoids placing `packed` on a type if the `packed` is redundant and the type needs an `align` attribute. If the type does not have an "align" attribute, then bindgen will still place `packed` so as to avoid changing existing working behavior.

bertschingert: > The test header intentionally generates types that won't compile. If this is a problem, and the test output rust source needs to compile, let me know and I'll remove those types...

OK, based on the test failures from these types not compiling, I take it I should fix this
bertschingert: pushed v2:
- fixed formatting issues from rustfmt
- removed the intentionally non-compiling types form the test header
workingjubilee: The combination of bitfields and `packed` is quite simply not well-defined in C, at all, and it only becomes less well-defined with the addition of the `aligned(N)` attribute, so I do not think we should try to apply the reasoning in this patch to such cases.
workingjubilee: Since rustc accepts `packed(N)`, wouldn't it be preferable to merge `packed` + `aligned(N)` into `packed(N)` when the combined directives would have that meaning?
bertschingert: > Since rustc accepts packed(N), wouldn't it be preferable to merge packed + aligned(N) into packed(N) when the combined directives would have that meaning?

Bindgen should already be doing this. It works when `N` is <= the struct's natural alignment. My patch shouldn't affect this case (this is what the check for `!explicit_align.is_some()` is meant to do). It won't work if `N` is > the struct's natural alignment since `packed` can only decrease alignment, and this is the only case that this patch should affect, if I did everything right.

> The combination of bitfields and packed is quite simply not well-defined in C, at all, and it only becomes less well-defined with the addition of the aligned(N) attribute, so I do not think we should try to apply the reasoning in this patch to such cases.

Hmm. Do you have any examples of cases where problems could arise here? I'll admit I am not familiar with all the rules for bitfields especially across different C compilers. I'll try to research it more myself too but if you can give an example of a problem case that would be helpful. If this patch exposes problem cases it'll need to be reworked, but if it doesn't I think it'd be nice to go ahead with it and handle more cases in bindgen.

Ultimately, not handling this case in bindgen isn't the end of the world since users can construct the Rust bindings by hand, but it would be nice to have bindgen handle as many cases as possible.
workingjubilee: > Bindgen should already be doing this. It works when N is <= the struct's natural alignment

Ah, okay. I am "blessed" to work on a codebase that uses bindgen but the target C codebase only has one instance of using the combined `packed` + `aligned(N)` attributes, and then only to exhort rare C compilers to strip padding in a particularly unusual case they should already be removing padding from: the Rust `repr(C)` layout in that one case is identical to the layout arrived at after the annotations are considered.

...they otherwise prefer to manually pack things, byte by byte, in code. Charming, really.

> Hmm. Do you have any examples of cases where problems could arise here? I'll admit I am not familiar with all the rules for bitfields especially across different C compilers. I'll try to research it more myself too but if you can give an example of a problem case that would be helpful.

Even for the "same ABI", alignment for the "storage allocation" of the bitfield types can have different default alignments, and this deviates further when `packed` is invoked. For this reason [it is actually considered invalid to expose these types in public headers in the AArch64 ABI](https://github.com/ARM-software/abi-aa/blob/b0f1111afbd7d0aa8f2b160dd63971e69c99955a/aapcs64/aapcs64.rst?plain=1#L2707):

> The AAPCS64 **does not allow exported interfaces to contain packed structures or bit-fields**. However a scheme for laying out packed bit-fields can be achieved by reducing the alignment, A, in the above rules to below that of the natural container type. ARMCC uses an alignment of A=8 in these cases, but GCC uses an alignment of A=1.

Emphasis mine.

It should be noted that AArch64 has one of the most _well-defined_ ABIs in this case, and they still mumble something that leaves at least a few things up in the air and then just ban it in public interfaces. You cannot truly coherently reason about the allocation sizes of bitfields for most ABIs, because the C Standard is essentially mute on how they work, and then psABIs only constrain it just-enough. Then we inject `packed` into the mix, and psABIs say nothing about that, which compilers interpret as license to remake the already-vague rules wholesale for the packed-bitfields case.
bertschingert: > I also wonder if this should just be inside the is_packed logic, rather than as another walk through the fields, but maybe this way is simpler to reason about.

This is a good point; but after looking into this some it's not actually obvious to me what the best way to handle this is. Doing this probably adds complexity and might not skip that much work in many cases.

First, this patch shouldn't result in any extra work in the common cases since the new method will only be called for types that are both packed and need `explicit_align`, which should be pretty rare.
However, I think adding this logic into `is_packed()` could result in more walks through the field in some cases (when the `packed` attr is present) since we don't yet know if `explicit_align` is needed, when `is_packed()` is called.

I realized `is_packed()` is currently called 3 times for each compound type. I figured we could reduce the amount of repetitive work being done by storing the result of the check the first time and just querying the stored value later. However it turns out this doesn't work with the current code structure because `is_packed()` can get different results before and after `CompInfo::compute_bitfield_units()` runs, and it seems like some current behavior depends on this.

In the revision I just uploaded, I take out one extra call to `is_packed()` from `StructLayoutTracker::new()` since this one should really be redundant. Going even further and reworking `is_packed()` to only need to walk through the fields once -- and incorporating the `already_packed` check into that -- may be possible, but I think it could require some reworking of the logic to account for the bitfield allocation issue noted above. Given that, I figured I would see what you think of all this before I keep going down this rabbit hole.
bertschingert: @workingjubilee -- thanks for posting the additional info about bitfields (and thanks for reviewing this patch)! That aarch64 ABI doc was helpful.

I don't think this patch should change anything about how bindgen lays out structs with bitfields--in particular, any bindgen-created code that previously compiled under rustc _should_ be unaffected (I intentionally tried to write this patch to not affect any existing working behavior). But I'll keep thinking about this in case there any problem scenarios I've missed...

Patch:
diff --git a/bindgen/codegen/mod.rs b/bindgen/codegen/mod.rs
index c84320ce..b2f0f151 100644
--- a/bindgen/codegen/mod.rs
+++ b/bindgen/codegen/mod.rs
@@ -1977,6 +1977,7 @@ impl CodeGenerator for CompInfo {
             ty,
             &canonical_name,
             visibility,
+            packed,
         );
 
         if !is_opaque {
@@ -2196,7 +2197,14 @@ impl CodeGenerator for CompInfo {
         if let Some(comment) = item.comment(ctx) {
             attributes.push(attributes::doc(comment));
         }
-        if packed && !is_opaque {
+
+        // if a type has both a "packed" attribute and an "align(N)" attribute, then check if the
+        // "packed" attr is redundant, and do not include it if so.
+        if packed &&
+            !is_opaque &&
+            !(explicit_align.is_some() &&
+                self.already_packed(ctx).map_or(false, |t| t))
+        {
             let n = layout.map_or(1, |l| l.align);
             assert!(ctx.options().rust_features().repr_packed_n || n == 1);
             let packed_repr = if n == 1 {
diff --git a/bindgen/codegen/struct_layout.rs b/bindgen/codegen/struct_layout.rs
index a62da695..f4596a19 100644
--- a/bindgen/codegen/struct_layout.rs
+++ b/bindgen/codegen/struct_layout.rs
@@ -91,9 +91,9 @@ impl<'a> StructLayoutTracker<'a> {
         ty: &'a Type,
         name: &'a str,
         visibility: FieldVisibilityKind,
+        is_packed: bool,
     ) -> Self {
         let known_type_layout = ty.layout(ctx);
-        let is_packed = comp.is_packed(ctx, known_type_layout.as_ref());
         let (is_rust_union, can_copy_union_fields) =
             comp.is_rust_union(ctx, known_type_layout.as_ref(), name);
         StructLayoutTracker {
diff --git a/bindgen/ir/comp.rs b/bindgen/ir/comp.rs
index a5d06fa5..fed6ba8a 100644
--- a/bindgen/ir/comp.rs
+++ b/bindgen/ir/comp.rs
@@ -1642,6 +1642,26 @@ impl CompInfo {
         false
     }
 
+    /// Return true if a compound type is "naturally packed". This means we can exclude the
+    /// "packed" attribute without changing the layout.
+    /// This is useful for types that need an "align(N)" attribute since rustc won't compile
+    /// structs that have both of those attributes.
+    pub(crate) fn already_packed(&self, ctx: &BindgenContext) -> Option<bool> {
+        let mut total_size: usize = 0;
+
+        for field in self.fields().iter() {
+            let layout = field.layout(ctx)?;
+
+            if layout.align != 0 && total_size % layout.align != 0 {
+                return Some(false);
+            }
+
+            total_size += layout.size;
+        }
+
+        Some(true)
+    }
+
     /// Returns true if compound type has been forward declared
     pub(crate) fn is_forward_declaration(&self) -> bool {
         self.is_forward_declaration


Test patch:
diff --git a/bindgen-tests/tests/expectations/tests/redundant-packed-and-align.rs b/bindgen-tests/tests/expectations/tests/redundant-packed-and-align.rs
new file mode 100644
index 00000000..b5feaa14
--- /dev/null
+++ b/bindgen-tests/tests/expectations/tests/redundant-packed-and-align.rs
@@ -0,0 +1,369 @@
+#![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(8))]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct redundant_packed {
+    pub a: u32,
+    pub b: u32,
+}
+#[test]
+fn bindgen_test_layout_redundant_packed() {
+    const UNINIT: ::std::mem::MaybeUninit<redundant_packed> = ::std::mem::MaybeUninit::uninit();
+    let ptr = UNINIT.as_ptr();
+    assert_eq!(
+        ::std::mem::size_of::<redundant_packed>(),
+        8usize,
+        concat!("Size of: ", stringify!(redundant_packed)),
+    );
+    assert_eq!(
+        ::std::mem::align_of::<redundant_packed>(),
+        8usize,
+        concat!("Alignment of ", stringify!(redundant_packed)),
+    );
+    assert_eq!(
+        unsafe { ::std::ptr::addr_of!((*ptr).a) as usize - ptr as usize },
+        0usize,
+        concat!("Offset of field: ", stringify!(redundant_packed), "::", stringify!(a)),
+    );
+    assert_eq!(
+        unsafe { ::std::ptr::addr_of!((*ptr).b) as usize - ptr as usize },
+        4usize,
+        concat!("Offset of field: ", stringify!(redundant_packed), "::", stringify!(b)),
+    );
+}
+#[repr(C)]
+#[repr(align(8))]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct redundant_packed_bitfield {
+    pub a: [u8; 3usize],
+    pub _bitfield_align_1: [u8; 0],
+    pub _bitfield_1: __BindgenBitfieldUnit<[u8; 1usize]>,
+    pub c: u32,
+}
+#[test]
+fn bindgen_test_layout_redundant_packed_bitfield() {
+    const UNINIT: ::std::mem::MaybeUninit<redundant_packed_bitfield> = ::std::mem::MaybeUninit::uninit();
+    let ptr = UNINIT.as_ptr();
+    assert_eq!(
+        ::std::mem::size_of::<redundant_packed_bitfield>(),
+        8usize,
+        concat!("Size of: ", stringify!(redundant_packed_bitfield)),
+    );
+    assert_eq!(
+        ::std::mem::align_of::<redundant_packed_bitfield>(),
+        8usize,
+        concat!("Alignment of ", stringify!(redundant_packed_bitfield)),
+    );
+    assert_eq!(
+        unsafe { ::std::ptr::addr_of!((*ptr).a) as usize - ptr as usize },
+        0usize,
+        concat!(
+            "Offset of field: ",
+            stringify!(redundant_packed_bitfield),
+            "::",
+            stringify!(a),
+        ),
+    );
+    assert_eq!(
+        unsafe { ::std::ptr::addr_of!((*ptr).c) as usize - ptr as usize },
+        4usize,
+        concat!(
+            "Offset of field: ",
+            stringify!(redundant_packed_bitfield),
+            "::",
+            stringify!(c),
+        ),
+    );
+}
+impl redundant_packed_bitfield {
+    #[inline]
+    pub fn b0(&self) -> u8 {
+        unsafe { ::std::mem::transmute(self._bitfield_1.get(0usize, 1u8) as u8) }
+    }
+    #[inline]
+    pub fn set_b0(&mut self, val: u8) {
+        unsafe {
+            let val: u8 = ::std::mem::transmute(val);
+            self._bitfield_1.set(0usize, 1u8, val as u64)
+        }
+    }
+    #[inline]
+    pub fn b1(&self) -> u8 {
+        unsafe { ::std::mem::transmute(self._bitfield_1.get(1usize, 1u8) as u8) }
+    }
+    #[inline]
+    pub fn set_b1(&mut self, val: u8) {
+        unsafe {
+            let val: u8 = ::std::mem::transmute(val);
+            self._bitfield_1.set(1usize, 1u8, val as u64)
+        }
+    }
+    #[inline]
+    pub fn new_bitfield_1(b0: u8, b1: u8) -> __BindgenBitfieldUnit<[u8; 1usize]> {
+        let mut __bindgen_bitfield_unit: __BindgenBitfieldUnit<[u8; 1usize]> = Default::default();
+        __bindgen_bitfield_unit
+            .set(
+                0usize,
+                1u8,
+                {
+                    let b0: u8 = unsafe { ::std::mem::transmute(b0) };
+                    b0 as u64
+                },
+            );
+        __bindgen_bitfield_unit
+            .set(
+                1usize,
+                1u8,
+                {
+                    let b1: u8 = unsafe { ::std::mem::transmute(b1) };
+                    b1 as u64
+                },
+            );
+        __bindgen_bitfield_unit
+    }
+}
+#[repr(C)]
+#[repr(align(16))]
+#[derive(Copy, Clone)]
+pub union redundant_packed_union {
+    pub a: u64,
+    pub b: u32,
+}
+#[test]
+fn bindgen_test_layout_redundant_packed_union() {
+    const UNINIT: ::std::mem::MaybeUninit<redundant_packed_union> = ::std::mem::MaybeUninit::uninit();
+    let ptr = UNINIT.as_ptr();
+    assert_eq!(
+        ::std::mem::size_of::<redundant_packed_union>(),
+        16usize,
+        concat!("Size of: ", stringify!(redundant_packed_union)),
+    );
+    assert_eq!(
+        ::std::mem::align_of::<redundant_packed_union>(),
+        16usize,
+        concat!("Alignment of ", stringify!(redundant_packed_union)),
+    );
+    assert_eq!(
+        unsafe { ::std::ptr::addr_of!((*ptr).a) as usize - ptr as usize },
+        0usize,
+        concat!(
+            "Offset of field: ",
+            stringify!(redundant_packed_union),
+            "::",
+            stringify!(a),
+        ),
+    );
+    assert_eq!(
+        unsafe { ::std::ptr::addr_of!((*ptr).b) as usize - ptr as usize },
+        0usize,
+        concat!(
+            "Offset of field: ",
+            stringify!(redundant_packed_union),
+            "::",
+            stringify!(b),
+        ),
+    );
+}
+impl Default for redundant_packed_union {
+    fn default() -> Self {
+        let mut s = ::std::mem::MaybeUninit::<Self>::uninit();
+        unsafe {
+            ::std::ptr::write_bytes(s.as_mut_ptr(), 0, 1);
+            s.assume_init()
+        }
+    }
+}
+#[repr(C)]
+#[repr(align(2))]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct inner {
+    pub a: u8,
+}
+#[test]
+fn bindgen_test_layout_inner() {
+    const UNINIT: ::std::mem::MaybeUninit<inner> = ::std::mem::MaybeUninit::uninit();
+    let ptr = UNINIT.as_ptr();
+    assert_eq!(
+        ::std::mem::size_of::<inner>(),
+        2usize,
+        concat!("Size of: ", stringify!(inner)),
+    );
+    assert_eq!(
+        ::std::mem::align_of::<inner>(),
+        2usize,
+        concat!("Alignment of ", stringify!(inner)),
+    );
+    assert_eq!(
+        unsafe { ::std::ptr::addr_of!((*ptr).a) as usize - ptr as usize },
+        0usize,
+        concat!("Offset of field: ", stringify!(inner), "::", stringify!(a)),
+    );
+}
+#[repr(C)]
+#[repr(align(8))]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct outer_redundant_packed {
+    pub a: [inner; 2usize],
+    pub b: u32,
+}
+#[test]
+fn bindgen_test_layout_outer_redundant_packed() {
+    const UNINIT: ::std::mem::MaybeUninit<outer_redundant_packed> = ::std::mem::MaybeUninit::uninit();
+    let ptr = UNINIT.as_ptr();
+    assert_eq!(
+        ::std::mem::size_of::<outer_redundant_packed>(),
+        8usize,
+        concat!("Size of: ", stringify!(outer_redundant_packed)),
+    );
+    assert_eq!(
+        ::std::mem::align_of::<outer_redundant_packed>(),
+        8usize,
+        concat!("Alignment of ", stringify!(outer_redundant_packed)),
+    );
+    assert_eq!(
+        unsafe { ::std::ptr::addr_of!((*ptr).a) as usize - ptr as usize },
+        0usize,
+        concat!(
+            "Offset of field: ",
+            stringify!(outer_redundant_packed),
+            "::",
+            stringify!(a),
+        ),
+    );
+    assert_eq!(
+        unsafe { ::std::ptr::addr_of!((*ptr).b) as usize - ptr as usize },
+        4usize,
+        concat!(
+            "Offset of field: ",
+            stringify!(outer_redundant_packed),
+            "::",
+            stringify!(b),
+        ),
+    );
+}
+#[repr(C)]
+#[repr(align(4))]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct redundant_pragma_packed {
+    pub a: u8,
+    pub b: u16,
+}
+#[test]
+fn bindgen_test_layout_redundant_pragma_packed() {
+    const UNINIT: ::std::mem::MaybeUninit<redundant_pragma_packed> = ::std::mem::MaybeUninit::uninit();
+    let ptr = UNINIT.as_ptr();
+    assert_eq!(
+        ::std::mem::size_of::<redundant_pragma_packed>(),
+        4usize,
+        concat!("Size of: ", stringify!(redundant_pragma_packed)),
+    );
+    assert_eq!(
+        ::std::mem::align_of::<redundant_pragma_packed>(),
+        4usize,
+        concat!("Alignment of ", stringify!(redundant_pragma_packed)),
+    );
+    assert_eq!(
+        unsafe { ::std::ptr::addr_of!((*ptr).a) as usize - ptr as usize },
+        0usize,
+        concat!(
+            "Offset of field: ",
+            stringify!(redundant_pragma_packed),
+            "::",
+            stringify!(a),
+        ),
+    );
+    assert_eq!(
+        unsafe { ::std::ptr::addr_of!((*ptr).b) as usize - ptr as usize },
+        2usize,
+        concat!(
+            "Offset of field: ",
+            stringify!(redundant_pragma_packed),
+            "::",
+            stringify!(b),
+        ),
+    );
+}
diff --git a/bindgen-tests/tests/headers/redundant-packed-and-align.h b/bindgen-tests/tests/headers/redundant-packed-and-align.h
new file mode 100644
index 00000000..75e15e41
--- /dev/null
+++ b/bindgen-tests/tests/headers/redundant-packed-and-align.h
@@ -0,0 +1,42 @@
+typedef unsigned char uint8_t;
+typedef unsigned short uint16_t;
+typedef unsigned int uint32_t;
+typedef unsigned long long uint64_t;
+
+struct redundant_packed {
+    uint32_t a;
+    uint32_t b;
+} __attribute__((packed, aligned(8)));
+
+struct redundant_packed_bitfield {
+    uint8_t a[3];
+    uint8_t b0:1;
+    uint8_t b1:1;
+    uint32_t c;
+} __attribute__((packed, aligned(8)));
+
+
+union redundant_packed_union {
+    uint64_t a;
+    uint32_t b;
+} __attribute__((packed, aligned(16)));
+
+
+struct inner {
+    uint8_t a;
+} __attribute__((packed, aligned(2)));
+
+struct outer_redundant_packed {
+    struct inner a[2];
+    uint32_t b;
+} __attribute__((packed, aligned(8)));
+
+
+#pragma pack(2)
+
+struct redundant_pragma_packed {
+    uint8_t a;
+    uint16_t b;
+} __attribute__((aligned(4)));
+
+#pragma pack()