[AIX] Lint on structs that have a different alignment in AIX's C ABI by amy-kwan · Pull Request #135552 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation55 Commits1 Checks6 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

This PR adds a linting diagnostic on AIX for repr(C) structs that are required to follow
the power alignment rule. A repr(C) struct needs to follow the power alignment rule if
the struct:

Has a floating-point data type (greater than 4-bytes) as its first member, or
The first member of the struct is an aggregate, whose recursively first member is a
floating-point data type (greater than 4-bytes).

The power alignment rule for eligible structs is currently unimplemented, so a linting
diagnostic is produced when such a struct is encountered.

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @fee1-dead (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

Jan 15, 2025

This comment has been minimized.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please give an actual description of the "power alignment rule" for the lint. It must not use undefined terms like "natural alignment". Otherwise the lint is not implementing a spec.

/// - floating-point data type as its first member, or
/// - first member being an aggregate whose recursively first member is a
/// floating-point data type
/// must have its natural alignment. This is currently unimplemented within

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"natural alignment"?

What does "natural alignment" mean?

The alignment of f64 is 8 bytes, so the "natural alignment" of the struct should be 8 bytes, and the produced struct will be 8 bytes, but the alignment to 4 bytes described here by @mustartt is not a "natural alignment".

Comment on lines 1578 to 1580

// - the first argument of the struct is a floating-point type, or
// - the first argument of the struct is an aggregate whose
// recursively first member is a floating-point type

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// - the first argument of the struct is a floating-point type, or
// - the first argument of the struct is an aggregate whose
// recursively first member is a floating-point type
// - the first field of the struct is a floating-point type, or
// - the first field of the struct is an aggregate whose
// recursively first field is a floating-point type

&& !adt_def.all_fields().next().is_none()
{
let struct_variant_data = item.expect_struct().0;
// Analyze only the first member of the struct to see if it

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Analyze only the first member of the struct to see if it
// Analyze only the first field of the struct to see if it

/// }
/// ```
///
/// A warning should be produced for the above struct as the first member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// A warning should be produced for the above struct as the first member
/// A warning should be produced for the above struct as the first field

rustbot added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Jan 15, 2025

workingjubilee changed the title~~[AIX] Emit linting diagnositc to detect structs that follow the power alignment rule under repr(C)~~ [AIX] Lint on structs that have a different alignment in AIX's C ABI

Jan 15, 2025

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Jan 20, 2025

This comment has been minimized.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a strange rule. Thank you, that is a much better description. A few more edits and this should be good.

It seems we would match the alignment produced if -qalign=natural is used, wouldn't we? Of course, such C code is incompatible with -qalign=power, which as I understand it is the default?

Comment on lines +1621 to +1638

if adt_def.repr().c()
&& !adt_def.repr().packed()
&& cx.tcx.sess.target.os == "aix"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please check this first in both functions, so that the code doesn't really run for most targets.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. I will add an AIX check in the other function, too.

Comment on lines +58 to +66

y: FloatAgg2, //~ WARNING repr(C) does not follow the power alignment rule. This may affect platform C ABI compatibility for this type
z: FloatAgg2, //~ WARNING repr(C) does not follow the power alignment rule. This may affect platform C ABI compatibility for this type

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if included in FloatAgg3, FloatAgg2 is given a different layout...?

y: FloatAgg2, //~ WARNING repr(C) does not follow the power alignment rule. This may affect platform C ABI compatibility for this type
z: FloatAgg2, //~ WARNING repr(C) does not follow the power alignment rule. This may affect platform C ABI compatibility for this type
// NOTE: the "power" alignment rule is infectious to nested struct fields
y: FloatAgg2, //~ WARNING repr(C) does not follow the power alignment rule. This may affect platform C ABI compatibility for this type
z: FloatAgg2, //~ WARNING repr(C) does not follow the power alignment rule. This may affect platform C ABI compatibility for this type

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding the comment here makes sense.
Currently, in Rust, we would get something like:

offset_of!(FloatAgg3, x) == 0
offset_of!(FloatAgg3, y) == 40
offset_of!(FloatAgg3, z) == 80

Whereas power alignment would expect it to be more like:

offset_of!(FloatAgg3, x) == 0
offset_of!(FloatAgg3, y) == 32
offset_of!(FloatAgg3, z) == 64

Comment on lines 740 to 741

/// In this case, "natural alignment" (detailed in
/// https://www.ibm.com/docs/en/xl-c-and-cpp-aix/16.1?topic=data-using-alignment-modes#alignment)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This link should probably come first, actually.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the silly question, but just wanted to check if I understand the suggestion. Do you mean that the link should be in the paragraph above instead and we should remove this section (since your suggestion above relates to having to avoid describing natural alignment)?

Please do correct me if I misunderstood. :-)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, it should probably be on the first line (which my suggestion currently doesn't capture, but please feel free to edit things as you see fit).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. Thanks for clarifying! I tried to make it closer to the top/first line, so hopefully you are okay with its placement.

rustbot added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Jan 21, 2025

uthe relevant part of the error:

tidy check
tidy error: /checkout/compiler/rustc_lint/messages.ftl: message lint_uses_power_alignment appears before lint_unused_comparisons, but is alphabetically later than it
run ./x.py test tidy --bless to sort the file correctly

/// is an aggregate whose recursively "first" field is a floating-point
/// type greater than 4-bytes.
/// The alignment for subsequent fields of the associated structs use an
/// alignment value where the floating-point type is aligned on a 4-byte

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an important question: how would the XLC compiler handle this case?

#pragma align(power) typedef struct Lol { float criminal_field; uint16_t filler; _Float16 half_as_bad; } Lmao;

Does the _Float16 get aligned to 4, even though its natural alignment would be 2? Does the compiler even support a half type of some kind? (please substitute whatever it does support, if so).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using XLC, it appears that half precision floating point was never supported in C on the AIX platform. Thus, the ABI is currently unknown.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.

What a strange rule. Thank you, that is a much better description. A few more edits and this should be good.

It seems we would match the alignment produced if -qalign=natural is used, wouldn't we? Of course, such C code is incompatible with -qalign=power, which as I understand it is the default?

Yup, you and I both have the same understanding. Power alignment is the default, which is why they differ.

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Jan 21, 2025

@@ -727,7 +728,50 @@ declare_lint! {
"proper use of libc types in foreign item definitions"
}

declare_lint_pass!(ImproperCTypesDefinitions => [IMPROPER_CTYPES_DEFINITIONS]);
declare_lint! {
/// The `uses_power_alignment` lint detects specific `repr(C)`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @workingjubilee! Just tagging you here in case you had any opinions on this first sentence I added, since I realized I needed this line when building the lint docs. I added a more general first sentence here as we go into describing power alignment below. Hope this is fine, but if not, I am open to suggestions.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to me!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! Thank you so much for your thorough review! I really appreciate it!

This comment has been minimized.

This should only affect the AIX target so it's safe to

@bors r+ rollup

📌 Commit cd2ecc4 has been approved by workingjubilee

It is now in the queue for this repository.

bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Jan 22, 2025

jhpratt added a commit to jhpratt/rust that referenced this pull request

Jan 23, 2025

…gnostic-power-alignment, r=workingjubilee

[AIX] Lint on structs that have a different alignment in AIX's C ABI

This PR adds a linting diagnostic on AIX for repr(C) structs that are required to follow the power alignment rule. A repr(C) struct needs to follow the power alignment rule if the struct:

Has a floating-point data type (greater than 4-bytes) as its first member, or
The first member of the struct is an aggregate, whose recursively first member is a floating-point data type (greater than 4-bytes).

The power alignment rule for eligible structs is currently unimplemented, so a linting diagnostic is produced when such a struct is encountered.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 23, 2025

Rollup of 10 pull requests

Successful merges:

rust-lang#134746 (Don't ICE in coerce when autoderef fails to structurally normalize non-WF type in new solver)
rust-lang#135552 ([AIX] Lint on structs that have a different alignment in AIX's C ABI)
rust-lang#135764 (Fix tests on LLVM 20)
rust-lang#135779 (CI: free disk on linux arm runner)
rust-lang#135790 (Update windows-gnu targets to set DebuginfoKind::DWARF)
rust-lang#135879 (fix outdated file path ref in llvm)
rust-lang#135883 (Remove erroneous unsafe in BTreeSet::upper_bound_mut)
rust-lang#135884 (remove implied end of slice)
rust-lang#135887 (improvements on build_steps::test implementation)
rust-lang#135898 (rustdoc-json-types: Finalize dyn compatibility renaming)

r? @ghost @rustbot modify labels: rollup

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jan 23, 2025

…iaskrgr

Rollup of 9 pull requests

Successful merges:

rust-lang#134746 (Don't ICE in coerce when autoderef fails to structurally normalize non-WF type in new solver)
rust-lang#135552 ([AIX] Lint on structs that have a different alignment in AIX's C ABI)
rust-lang#135779 (CI: free disk on linux arm runner)
rust-lang#135790 (Update windows-gnu targets to set DebuginfoKind::DWARF)
rust-lang#135879 (fix outdated file path ref in llvm)
rust-lang#135883 (Remove erroneous unsafe in BTreeSet::upper_bound_mut)
rust-lang#135884 (remove implied end of slice)
rust-lang#135887 (improvements on build_steps::test implementation)
rust-lang#135898 (rustdoc-json-types: Finalize dyn compatibility renaming)

r? @ghost @rustbot modify labels: rollup

Comment on lines +1642 to +1647

for (index, ..) in struct_variant_data.fields().iter().enumerate() {
// Struct fields (after the first field) are checked for the
// power alignment rule, as fields after the first are likely
// to be the fields that are misaligned.
if index != 0 {
let first_field_def = struct_variant_data.fields()[index];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI this is unnecessarily complicated; the first_field_def here is exactly computing what you are throwing away with the .. above.

#139059 cleans this up a little.

Zalathar added a commit to Zalathar/rust that referenced this pull request

Apr 30, 2025

…agisa

uses_power_alignment: wording tweaks

Slightly improves the wording introduced with rust-lang#135552.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request

Apr 30, 2025

…agisa

uses_power_alignment: wording tweaks

Slightly improves the wording introduced with rust-lang#135552.

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Apr 30, 2025

Rollup merge of rust-lang#139059 - RalfJung:uses_power_alignment, r=nagisa

uses_power_alignment: wording tweaks

Slightly improves the wording introduced with rust-lang#135552.

github-actions bot pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request

May 1, 2025

uses_power_alignment: wording tweaks

Slightly improves the wording introduced with rust-lang/rust#135552.

github-actions bot pushed a commit to rust-lang/miri that referenced this pull request

May 1, 2025

uses_power_alignment: wording tweaks

Slightly improves the wording introduced with rust-lang/rust#135552.

Labels

O-aix

OS: Big Blue's Advanced Interactive eXecutive..

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.