Skip to content
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

[RFC, V2] exclude packed attr for types that contain aligned types when possible #2770

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bertschingert
Copy link
Contributor

NOTE: I know this can use some cleanups and more documentation / comments, etc. but for now I'm just uploading it as an RFC to see if you like the general approach. If you do, then I'll clean it up, but if not I won't bother.


This patch aims to work around a limitation of rustc in which it will not compile types with a packed attr that contain types with an align(N) attr. If the packed attr on the outer/parent type is redundant, and can be removed without changing the type's layout, then this patch will remove that attr so that the type can be compiled under rustc. If the type's packed attr cannot be removed without changing the layout, then it will be left alone.

Handling this correctly requires a change to when bindgen determines whether a type requires an align attr. Currently, this happens during the code generation phase. However, we cannot in general assume anything about the order in which code is generated for types, and in particular codegen may occur for a parent type before codegen for all child types contained in that parent. However, in order to strip the packed attr in cases where we want to do so, we already need to know about the alignment of all children.

In order to make that possible, this patch moves the logic to check if a type requires an explicit align attr earlier. This now happens before codegen starts for the first type, so we can use information about the alignment of all child types to determine whether to place a packed attr on any given type.

This patch aims to work around a limitation of rustc in which it will
not compile types with a `packed` attr that contain types with an
`align(N)` attr. If the `packed` attr on the outer/parent type is
redundant, and can be removed without changing the type's layout, then
this patch will remove that attr so that the type can be compiled under
rustc. If the type's `packed` attr cannot be removed without changing
the layout, then it will be left alone.

Handling this correctly requires a change to when bindgen determines
whether a type requires an `align` attr. Currently, this happens during
the code generation phase. However, we cannot in general assume anything
about the order in which code is generated for types, and in particular
codegen may occur for a parent type before codegen for all child types
contained in that parent. However, in order to strip the `packed` attr
in cases where we want to do so, we already need to know about the
alignment of all children.

In order to make that possible, this patch moves the logic to check if a
type requires an explicit `align` attr earlier. This now happens before
codegen starts for the first type, so we can use information about the
alignment of all child types to determine whether to place a `packed`
attr on any given type.
@bertschingert
Copy link
Contributor Author

An optimization opportunity for this patch could be to recursively visit the children of each type during BindgenContext::compute_alignment_of_compound_types() and flag each type as whether any of its children have an alignment attribute. Then, each type only needs to be visited/checked for alignment once and the extra tree-walk of the type tree of packed types in CompInfo::contains_aligned_type() could be eliminated and replaced with a check of a boolean flag.

@ojeda ojeda added the rust-for-linux Issues relevant to the Rust for Linux project label Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rust-for-linux Issues relevant to the Rust for Linux project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants