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, V1] try to exclude packed attr for types that contain aligned types #2769

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 handles some (but not all) cases of types with a packed attribute that contain a type with an align(N) attribute.

This uses information available in the Clang IR about types' layout to determine if a given type is likely to have an alignment attribute placed on it during the code generation phase. This is just a heuristic; the decision to actually place an alignment attribute depends on information that is not known until code generation, so the logic here may result in both false positives and false negatives.

Using the real information from codegen on whether an alignment attribute was placed on a child type is not possible in general, because the order in which types are generated is not guaranteed. In some cases code may be generated for parent types before code is generated for child types contained in that parent. Then, it will be impossible to make an accurate decision regarding whether to remove the packed attribute from the parent type.

The impact of a false negative is that an outer type may have a packed attr even when an inner type as an align attr. Such a type would not compile under rustc, but it already would not compile so this has no harmful impact.

The impact of a false positive is that an outer type may have its packed attr stripped needlessly, because no inner types actually have an align attr. Because we only remove the packed attr when we can confirm that there will be no change to the type's layout, this also should have no harmful impact.

This patch handles some (but not all) cases of types with a `packed`
attribute that contain a type with an `align(N)` attribute.

This uses information available in the Clang IR about types' layout to
determine if a given type is likely to have an alignment attribute
placed on it during the code generation phase. This is just a heuristic;
the decision to actually place an alignment attribute depends on
information that is not known until code generation, so the logic here
may result in both false positives and false negatives.

Using the real information from codegen on whether an alignment
attribute was placed on a child type is not possible in general, because
the order in which types are generated is not guaranteed. In some cases
code may be generated for parent types before code is generated for
child types contained in that parent. Then, it will be impossible to
make an accurate decision regarding whether to remove the `packed`
attribute from the parent type.

The impact of a false negative is that an outer type may have a `packed`
attr even when an inner type as an `align` attr. Such a type would not
compile under rustc, but it already would not compile so this has no
harmful impact.

The impact of a false positive is that an outer type may have its
`packed` attr stripped needlessly, because no inner types actually have
an `align` attr. Because we only remove the `packed` attr when we can
confirm that there will be no change to the type's layout, this also
should have no harmful impact.
@bors-servo
Copy link

☔ The latest upstream changes (presumably 7e90434) made this pull request unmergeable. Please resolve the merge conflicts.

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.

3 participants