try to avoid #[repr(packed)] when align is needed

[bertschingert]

This is for #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:

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 that aren't Arm's generally simply say nothing about that, which compilers interpret as license to remake the already-vague rules wholesale for the packed-bitfields case.

[emilio]

So this looks somewhat reasonable. I have a couple nits. 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.

[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...

[emilio]

unwrap_or(false)