compiler: remove AIX's uses_power_alignment lint

[workingjubilee]

In #149880 we changed the data layout for AIX to use the corrected, reduced alignment for f64 (AKA "double"). Because the uses_power_alignment lint was based on the premise that our previous alignment was correct, which is wrong, we should now simply remove it. This greatly simplifies the surrounding code handling types that are "improper" for FFI.

Some repr(C) structs are still not given the proper layout for FFI compatibility. This will involve a future migration plan involving either a new lint or language changes. The current lint remains incorrect and is not very useful to rewrite into possible revised lints that address this need.

[rustbot]

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

These commits modify compiler targets.
(See the Target Tier Policy.)

[RalfJung]

It does not fix upstream

I assume you are referring to llvm/llvm-project#133599 here? Always good to leave some cross-references. :)

Please also add comments in the code referencing that (both in the aix target triple, and the special exception for the data layout consistency check). Right now, just looking at the code after applying this diff, one would have a very hard time figuring out what happens.

[workingjubilee]

aye aye capitan

[RalfJung]

As discussed on IRLO, as-is this patch would make some types more correct and others less correct. Hard to say whether that's overall a net positive...

[workingjubilee]

It does fix every type that doesn't have f64 as its recursive-first-element, so I'm feeling pretty good about it alone.

[RalfJung]

Are there really more types with f64 in some later field vs the first field? Also, for the later field case we at least had a lint...

[beetrees]

Not all types which start with f64 would be affected, just those that also need at least 4 bytes of padding at the end of the struct, all of which could easily be fixed by adding _padding: MaybeUninit<u32> at the end of the relevant struct (a lint could be added for this case with a suggestion if desired). Compared to the #112480-like issues that giving f64 an alignment of 8 causes, I think this PR is a definite improvement.

[workingjubilee]

Are there really more types with f64 in some later field vs the first field?

I think so, when we're considering that it includes all nested aggregates?

Yes, the problem after this patch is now "possible-to-fix-in-bindgen"-tier.

[RalfJung]

all of which could easily be fixed by adding _padding: MaybeUninit at the end of the relevant struct

Ah, that's a good point.

[amy-kwan]

Hi! Thanks for this patch. I wanted to test this on an AIX machine, but if I try to build it, I get an assertion on the LLVM side:

. . .

Assertion failed: Target.isCompatibleDataLayout(getDataLayout()) && "Can't create a MachineFunction using a Module with a " "Target-incompatible DataLayout attached\n", file  rust/src/llvm-project/llvm/lib/CodeGen/MachineFunction.cpp, line 248, void llvm::MachineFunction::init()()
rustc exited with signal: 6 (SIGABRT)

Did not run successfully: signal: 6 (SIGABRT)
error: could not compile `compiler_builtins` (lib)
. . .

Since the datalayout string in LLVM does not match the datalayout string in rustc. If I make the strings match to unblock the build, I see the internal compiler bug message that was added in the patch:

error: internal compiler error: compiler/rustc_codegen_llvm/src/context.rs:223:17: LLVM got fixed, please remove this exception in cg_llvm!

In any case, I tried to work around these issues to test the patch a bit. There are a few concerns from our end:

• This change is not buildable on AIX as-is without the backend change but also not buildable due to the rustc exception.
• As mentioned in the previous comments, some structs that start with f64 would require the extra 4 bytes of padding at the end of the struct so that the size is correct.
• It seems the struct sizing is also affected when we're not using repr(C). For something like the following, this should have a size of 24, but now we would report 20 for this struct:

pub struct Floats {
    a: f64,
    b: u32,
    c: f64,
}

Ideally, we would only want these changes for repr(C) structs and do not want to affect normal Rust structs at all.

FYI @daltenty

[RalfJung]

For something like the following, this should have a size of 24, but now we would report 20 for this struct:

That's a Rust layout struct. Why should it have AIX-specific layout? We use the same (undocumented, can-change-any-day) algorithm for repr(Rust) on all targets and we really want to keep it that way for the sake of everyone's sanity.

[workingjubilee]

This change is not buildable on AIX as-is without the backend change but also not buildable due to the rustc exception.

Ah, sorry. I'll just remove the bug! then.

[workingjubilee]

@amy-kwan New patch to try out, should be less comical to test.

[workingjubilee]

Ideally, we would only want these changes for repr(C) structs and do not want to affect normal Rust structs at all.

While I may make further changes to the layout algorithm that may overalign in some cases for the performance reasons you note, the fundamental detail is that it doesn't matter: once any f64 may be underaligned (read: 4, the ABI alignment on AIX), our codegen will notice they all are at that alignment and our reads and writes of that type will be generated with such an alignment annotation for LLVM. LLVM will only be able to upgrade the alignment as an optimization.

That is not an optimization I believe we should ourselves perform on our LLVMIR. This is because it would be extremely fragile, as repr(Rust) structs and repr(C) structs can be nested within each other and everything must still make sense when we do that. Because of this, we should not use the knowledge we have entered a struct with a certain repr to modify the way we do accesses to types.

This is also partly because any reasoning that we overaligned things depends on specifics of the layout algorithm that we are allowed to undermine. It would be globally embedding a local assumption from another part of the code. So for this:

pub struct Floats {
    a: f64,
    b: u32,
    c: f64,
}

If the true alignment of f64 is 4, then you can neither rely on us choosing this layout:

#[repr(linear)]
pub struct Floats {
    a: f64,
    c: f64,
    b: u32,
}

nor can you rely on us choosing this layout:

#[repr(linear)]
pub struct Floats {
    b: u32,
    a: f64,
    c: f64,
}

We are allowed to pick either actual effective layout. You do not have a "should" you can rely on. And in general neither should we: if we make our code break with our own rules, then we make it harder to update.

[workingjubilee]

Also you could probably have built the previous commit by disabling assertions for LLVM but I can understand not wanting to, for, you know, testing-the-patch purposes. :^)

[workingjubilee]

@nikic as usual is off being a gentleman and a scholar and has opened llvm/llvm-project#144673

[bors]

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

[apiraino]

@nikic as usual is off being a gentleman and a scholar and has opened llvm/llvm-project#144673

can the review of this patch proceed in parallel or do you prefer the LLVM to be first merged?

[workingjubilee]

@amy-kwan @mandlebug @daltenty Hello. The current build of the compiler ICEs when building the standard library due to a missing case regarding unions, see #147348. This patch would remove the ICE if accepted. I suppose I also have to rebase it, but I will also have to rebase it over any fixes of the "power alignment" lint.

[amy-kwan]

@amy-kwan @mandlebug @daltenty Hello. The current build of the compiler ICEs when building the standard library due to a missing case regarding unions, see #147348. This patch would remove the ICE if accepted. I suppose I also have to rebase it, but I will also have to rebase it over any fixes of the "power alignment" lint.

Hi @workingjubilee, I'd like to first apologize in the delay in getting back to this patch. We are aware of the compiler ICEs, and thanks for bringing attention to them.

We have recently been trying to sort out the data layout related issues on our side and have been reviewing and testing the relevant patches again, so we will be trying our best to continue to make progress on this.

I'm able to get this patch through a stage 1 build (thank you!), although a question I thought of when testing this patch is, when llvm/llvm-project#144673 lands, will any more changes be necessary? For example, do we still need the changes within compiler/rustc_codegen_llvm/src/context.rs? I figured I'd check because I ran into the following when I initially tested the two together:

error: data-layout for target `powerpc64-ibm-aix`, `E-m:a-Fi64-i64:64-i128:128-n32:64-S128-v256:256:256-v512:512:512`, differs from LLVM target's `powerpc64-ibm-aix` default layout, `E-m:a-Fi64-i64:64-i128:128-n32:64-f64:32:64-S128-v256:256:256-v512:512:512`

Somewhat separate to this, but I wasn't able to get through a complete stage 2 build due to the assert in https://github.com/BurntSushi/byteorder/blob/1.5.0/src/lib.rs#L1271, which I think makes sense. However, this crate hasn't been updated in awhile, which could be concerning if we wanted to make updates to it to accommodate the AIX data layout changes.

[RalfJung]

Somewhat separate to this, but I wasn't able to get through a complete stage 2 build due to the assert in https://github.com/BurntSushi/byteorder/blob/1.5.0/src/lib.rs#L1271, which I think makes sense. However, this crate hasn't been updated in awhile, which could be concerning if we wanted to make updates to it to accommodate the AIX data layout changes.

That assertion seems to simply not be true on AIX -- there are f64 that are only 4-aligned, but u64 are always 8-aligned. (It should be fascinating to have a chat with whomever decided this and thought it was a good idea.^^)

BurntSushi is still around and maintaining other code, so it may be worth reporting an issue nevertheless. Maybe that function should be simply cfg'd out on AIX.

[workingjubilee]

I'm able to get this patch through a stage 1 build (thank you!), although a question I thought of when testing this patch is, when llvm/llvm-project#144673 lands, will any more changes be necessary?

We do our best to keep our data layout string in sync with LLVM's, so yes some update will likely be required. It may be more minimal, however.

[workingjubilee]

I have opened BurntSushi/byteorder#221

[workingjubilee]

@amy-kwan The reason byteorder is nowhere in the rust-lang/rust Cargo.lock is because I specifically went through every crate that we depended on that also depended on it, PR'd updates to them to use stdlib functions instead, and got those updates published. Eventually we stopped building that dependency entirely. Not out of any animus to byteorder per se, merely a manic devotion to "one less dependency to build rustc!"

I would not worry too much about dependencies not building. They can be solved, one way or another.

I do appreciate you pointing that out, though.

[amy-kwan]

I'd like to apologize for the long delay as I have not been able to continue looking into the power alignment issues near the end of last year. I'm starting to pick this up again and so far, I've rebased this patch locally and am trying to locate the byteorder dependencies in the build. On the bright side, llvm/llvm-project#144673 has since landed, which is a step in the right direction. Once again, my apologies for the extremely long delay in reviewing this patch.

[workingjubilee]

Once again, my apologies for the extremely long delay in reviewing this patch.

We have all been there.

We actually already landed #149880 it seems, which contains the datalayout change.

I will cut this PR down to removing the lint, and will submit a change to byteorder.

I am happy to submit a followup to rust-lang/rust that does more in terms of addressing the remaining mismatch with repr(C), either via linting or layout changes, if we can agree on an approach for doing so. It wasn't clear to me whether we wanted to address that via

• demand people add explicit padding, lint for it, and make rust-bindgen do padding insertion?
• wait until repr(ordered_fields) lands, then implement the fix in repr(C)?
• YOLO the repr(C) fix even if we don't have repr(ordered_fields) yet, despite the risks that poses to ecosystem correctness, on the justification that AIX is a tier 3 target so we can use it as a guinea pig?

[RalfJung]

I think it has to be 1 and/or 2. We shouldn't change what repr(C) does without t-lang involvement and that discussion is/was happening at rust-lang/rfcs#3845 and possibly in other RFCs.

[workingjubilee]

Technically, we have already done so, in the sense that we already fixed the data layout for this target, thus now repr(C) is different across Rust compilers. :^)

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[RalfJung]

We did not change the spec for repr(C), which is in terms of the size and alignment of its fields. We changed the input to the algorithm, but that's a different kind of change. That change still should come with release notes though.