handle more cases of packed and aligned structs when it's safe/correct

[bertschingert]

When a C struct has both packed and aligned(N) attributes, in some cases bindgen generates a Rust struct with a packed(N) attribute only that has the correct ABI. (Note, I'm assuming GCC semantics for the packed and aligned(N) attributes on the C side.)

In other cases, though, bindgen generates code with both attributes, which doesn't compile under rustc. Many of these cases can be handled correctly by applying only an align(N) attribute; intuitively, this should work if the struct is "naturally packed".

Here is a minimal example:

struct mytest {
        uint32_t a;
        uint32_t b;
} __attribute__((packed, aligned(8)));

# bindgen generates: 

#[repr(C, packed(8))]
#[repr(align(8))]
#[derive(Debug, Copy, Clone)]
pub struct mytest {
    pub a: u32,
    pub b: u32,
}

# but this would be correct and ABI compatible: 

#[repr(C, align(8))]
#[derive(Debug, Copy, Clone)]
pub struct mytest {
    pub a: u32,
    pub b: u32,
}

This comes up for a handful of structs used in bcachefs. I wrote about this in a message to the rust-for-linux email list:
https://lore.kernel.org/rust-for-linux/20240122024711.GC151023@fedora-laptop/T/#m4439be01c0bcfdbaa781c379be3e227358cfab27

From that message:

In general, given a C structure like

struct A {
...
} __packed __aligned(N);

I think a Rust structure can be created with the same size, alignment,
and member offsets in the following cases:

(1) #[repr(packed(N))] will have the same layout if N is <= the
structure's default alignment and all members with a default
alignment >= N naturally have an offset that is a multiple of N.
Also, no member can have an alignment attribute as rustc forbids
this [2].

(2) #[repr(align(N))] will have the same layout if the structure's size
without the packed attribute is equal to the sum of the size of all
its members (i.e., it is "naturally packed"), and the structure's
default alignment is <= N.

It looks like bindgen already mostly handles case (1).

However, bindgen doesn't seem to handle case (2). What do you think of adding the logic to handle that case into bindgen? This would be really helpful for bcachefs with the structs mentioned in the above email.

[emilio]

I'd take a reasonable patch for this, sure :)

[bertschingert]

I'd take a reasonable patch for this, sure :)

Cool. I will get to work :)

[bertschingert]

The patch I've just uploaded (#2734) 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.

What do you think of a follow-up that handles the latter case? There is one example of a "packed" struct in bcachefs where the "packed" is redundant and it has an "aligned" member type: struct btree_node (https://github.com/koverstreet/bcachefs/blob/e7152225f6b401edb0c4dc285ccc9ac5fdfbcd85/fs/bcachefs/bcachefs_format.h#L1968). So handling this case would allow us to use bindgen for this type and not have to code it manually outside of bindgen.

I think this is a little trickier because in bindgen's code generation step we don't have the information easily available of if any child types had the "align" attribute applied to them. The determination to apply that attribute is done in StructLayoutTracker::requires_explicit_align but then it's not clear that it's easy to make that information available to any parent type that may contain the current type.

So I think to support this, it might make sense to try to move the logic to check if "align" is explicitly needed into an earlier stage, maybe somewhere under src/ir/analysis/? Then a field could be added to struct Layout which is accessible to any parent type that contains a type with this layout.

@@ -15,6 +15,8 @@ pub(crate) struct Layout {
     pub(crate) align: usize,
     /// Whether this layout's members are packed or not.
     pub(crate) packed: bool,
+    /// Whether this layout needs an explicit "align(N)" attribute
+    pub(crate) explicit_align: bool,
 }

What do you think of this -- does it make sense? Would you take a patch that does something like this?

[emilio]

So I think to support this, it might make sense to try to move the logic to check if "align" is explicitly needed into an earlier stage, maybe somewhere under src/ir/analysis/? Then a field could be added to struct Layout which is accessible to any parent type that contains a type with this layout.

Makes sense to me. Not sure if a full analysis/ pass would be necessary. It seems it would be feasible to handle it in CompInfo::layout?

[bertschingert]

Makes sense to me. Not sure if a full analysis/ pass would be necessary. It seems it would be feasible to handle it in CompInfo::layout?

Thanks! - I may take a look at implementing this but I probably won't get to it in the immediate future since the other patch was higher priority.

[bertschingert]

Emilio -

I've submitted two patches that take different approaches to handling "packed type transitively contains aligned type".

The first patch (#2769) is simpler but doesn't cover as many cases (for example, types with bitfields).

The second patch (#2770) is a lot more complex but it covers more cases.

I'm sharing both in case you prefer one approach over the other.

Honestly both patches are somewhat ugly and working on this issue has really convinced me that trying to work around this rustc limitation in bindgen is not the way to go. However, I imagine getting rustc enhanced to allow this will take a long time and in the short term handling this in bindgen would be helpful for a handful of types that have this problem in bcachefs.

For what it's worth, the first, simpler patch should be good enough for all the bcachefs types with this problem, but since the second patch is more complete, I figured I would still share it for consideration.