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

amy-kwan · 2025-01-15T19:56:53Z

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.

rustbot · 2025-01-15T19:57:03Z

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

amy-kwan · 2025-01-15T20:05:41Z

This PR is related to the discussion that was previously started by @mustartt: https://internals.rust-lang.org/t/repr-c-aix-struct-alignment/21594

FYI @daltenty

workingjubilee

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.

workingjubilee · 2025-01-15T20:26:35Z

compiler/rustc_lint/src/types.rs

+    ///   - 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


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

workingjubilee · 2025-01-15T20:28:26Z

compiler/rustc_lint/src/types.rs

+        //   - 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


Suggested change

// - 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

workingjubilee · 2025-01-15T20:28:35Z

compiler/rustc_lint/src/types.rs

+            && !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


Suggested change

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

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

compiler/rustc_lint/src/types.rs

workingjubilee · 2025-01-15T20:32:26Z

compiler/rustc_lint/src/types.rs

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


Suggested change

/// 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

amy-kwan · 2025-01-20T21:13:53Z

@rustbot review

workingjubilee

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?

compiler/rustc_lint/src/types.rs

workingjubilee · 2025-01-21T00:26:26Z

compiler/rustc_lint/src/types.rs

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


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

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

workingjubilee · 2025-01-21T00:29:59Z

tests/ui/layout/reprc-power-alignment.rs

+    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


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

Suggested change

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

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

compiler/rustc_lint/src/types.rs

workingjubilee · 2025-01-21T00:33:02Z

compiler/rustc_lint/src/types.rs

+    /// 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)


This link should probably come first, actually.

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. :-)

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

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

tests/ui/layout/reprc-power-alignment.rs

workingjubilee · 2025-01-21T01:18:32Z

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

workingjubilee · 2025-01-21T01:31:52Z

compiler/rustc_lint/src/types.rs

+    /// 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


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

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

amy-kwan · 2025-01-21T16:58:25Z

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.

amy-kwan · 2025-01-21T16:59:14Z

@rustbot review

workingjubilee · 2025-01-21T20:07:35Z

tests/ui/layout/reprc-power-alignment.rs

@@ -0,0 +1,146 @@
+//@ check-pass
+//@ revisions: aix


this doesn't need a revision

Suggested change

//@ revisions: aix

unless IBM does in fact have other platforms which use this bizarre alignment rule and is thinking about adding them to rustc, in which case I suppose I have no objection to keeping a single revision in place.

workingjubilee · 2025-01-21T20:09:43Z

tests/ui/layout/reprc-power-alignment.rs

+//@[aix] compile-flags: --target powerpc64-ibm-aix
+//@[aix] needs-llvm-components: powerpc


This test should use our minicore.

Suggested change

//@[aix] compile-flags: --target powerpc64-ibm-aix

//@[aix] needs-llvm-components: powerpc

//@ compile-flags: --target powerpc64-ibm-aix

//@ needs-llvm-components: powerpc

//@ add-core-stubs

#![feature(no_core)]

#![no_core]

#![no_std]

workingjubilee · 2025-01-21T23:03:22Z

compiler/rustc_lint/src/types.rs

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

-declare_lint_pass!(ImproperCTypesDefinitions => [IMPROPER_CTYPES_DEFINITIONS]);
+declare_lint! {
+    /// In it's platform C ABI, AIX uses the "power" (as in PowerPC) alignment


Suggested change

/// In it's platform C ABI, AIX uses the "power" (as in PowerPC) alignment

/// In its platform C ABI, AIX uses the "power" (as in PowerPC) alignment

compiler/rustc_lint/src/types.rs

workingjubilee

one nit, and with CI fixed then this will be good

tgross35 · 2025-01-21T23:18:03Z

(+history cleanup / squash pls)

amy-kwan · 2025-01-22T15:14:45Z

compiler/rustc_lint/src/types.rs

@@ -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)`


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.

This seems fine to me!

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

workingjubilee · 2025-01-22T19:02:18Z

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

@bors r+ rollup

bors · 2025-01-22T19:02:21Z

📌 Commit cd2ecc4 has been approved by workingjubilee

It is now in the queue for this repository.

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

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

…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

rustbot assigned fee1-dead Jan 15, 2025

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.

Sign in to view

workingjubilee requested changes Jan 15, 2025

View reviewed changes

workingjubilee assigned workingjubilee and unassigned fee1-dead Jan 15, 2025

workingjubilee requested changes Jan 15, 2025

View reviewed changes

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

amy-kwan requested a review from workingjubilee January 20, 2025 21:13

This comment has been minimized.

Sign in to view

workingjubilee requested changes Jan 21, 2025

View reviewed changes

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

workingjubilee reviewed Jan 21, 2025

View reviewed changes

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

amy-kwan requested a review from workingjubilee January 21, 2025 16:59

This comment has been minimized.

Sign in to view

workingjubilee reviewed Jan 21, 2025

View reviewed changes

workingjubilee approved these changes Jan 21, 2025

View reviewed changes

workingjubilee 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

amy-kwan force-pushed the amy-kwan/reprc-struct-diagnostic-power-alignment branch from b48eb01 to 646c280 Compare January 22, 2025 05:20

This comment has been minimized.

Sign in to view

amy-kwan force-pushed the amy-kwan/reprc-struct-diagnostic-power-alignment branch from 646c280 to 1d184a1 Compare January 22, 2025 15:12

amy-kwan commented Jan 22, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

amy-kwan force-pushed the amy-kwan/reprc-struct-diagnostic-power-alignment branch from 1d184a1 to 2d26033 Compare January 22, 2025 16:33

This comment has been minimized.

Sign in to view

amy-kwan force-pushed the amy-kwan/reprc-struct-diagnostic-power-alignment branch from 2d26033 to 3f73f85 Compare January 22, 2025 16:45

This comment has been minimized.

Sign in to view

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

cd2ecc4

amy-kwan force-pushed the amy-kwan/reprc-struct-diagnostic-power-alignment branch from 3f73f85 to cd2ecc4 Compare January 22, 2025 17:06

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 mentioned this pull request Jan 23, 2025

Rollup of 10 pull requests #135915

Closed

matthiaskrgr mentioned this pull request Jan 23, 2025

Rollup of 9 pull requests #135921

Merged

bors merged commit 9b40bd7 into rust-lang:master Jan 23, 2025
6 checks passed

rustbot added this to the 1.86.0 milestone Jan 23, 2025

	// 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
	/// A warning should be produced for the above struct as the first field

		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

		/// 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)

		//@[aix] compile-flags: --target powerpc64-ibm-aix
		//@[aix] needs-llvm-components: powerpc

-//@[aix] compile-flags: --target powerpc64-ibm-aix
-//@[aix] needs-llvm-components: powerpc
+//@ compile-flags: --target powerpc64-ibm-aix
+//@ needs-llvm-components: powerpc
+//@ add-core-stubs
+#![feature(no_core)]
+#![no_core]
+#![no_std]

	/// In it's platform C ABI, AIX uses the "power" (as in PowerPC) alignment
	/// In its platform C ABI, AIX uses the "power" (as in PowerPC) alignment

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

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

Conversation

amy-kwan commented Jan 15, 2025 • edited Loading

rustbot commented Jan 15, 2025

amy-kwan commented Jan 15, 2025

This comment has been minimized.

workingjubilee left a comment • edited Loading

Choose a reason for hiding this comment

workingjubilee Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-kwan commented Jan 20, 2025

This comment has been minimized.

This comment has been minimized.

workingjubilee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee commented Jan 21, 2025 • edited Loading

workingjubilee Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amy-kwan commented Jan 21, 2025

amy-kwan commented Jan 21, 2025

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

workingjubilee left a comment

Choose a reason for hiding this comment

tgross35 commented Jan 21, 2025

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

workingjubilee commented Jan 22, 2025

bors commented Jan 22, 2025

amy-kwan commented Jan 15, 2025 •

edited

Loading

workingjubilee left a comment •

edited

Loading

workingjubilee Jan 15, 2025 •

edited

Loading

workingjubilee Jan 21, 2025 •

edited

Loading

workingjubilee commented Jan 21, 2025 •

edited

Loading

workingjubilee Jan 21, 2025 •

edited

Loading