error out when repr(align) exceeds COFF limit

[nthery]

The PE-COFF binary format limits section alignment to 8192 bytes. Emit error when alignment exceeds this limit to avoid crash in llvm.

Closes #142386.

[rustbot]

r? @WaffleLapkin

rustbot has assigned @WaffleLapkin.
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]

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

[workingjubilee]

It only needs to error if we generate a static based on this.

[folkertdev]

Could you include testing alignment on functions here in this PR (if relevant, but I suspect it is). I think you can just add it to the test files you already modify.

#![feature(fn_align)]

// ...

// and then you can annotate a function with `#[repr(align(N))]`
#[repr(align(16))]
fn foo() {}

[workingjubilee]

@nthery This simple version that rejects repr(align(0x4000)) will need to be cratered. Nonetheless please finish writing up the PR so we can do so.

[workingjubilee]

...wait, we can't crater for Windows.

[workingjubilee]

fbbth.

[bors]

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

[nthery]

Could you include testing alignment on functions here in this PR (if relevant, but I suspect it is). I think you can just add it to the test files you already modify.

#![feature(fn_align)]

// ...

// and then you can annotate a function with `#[repr(align(N))]`
#[repr(align(16))]
fn foo() {}

It is relevant indeed. I reproduced the original bug with a function alignment exceeding 0x2000 bytes. Thanks for spotting this. I will add some test points.

[nthery]

I reckon I've addressed all points in the latest force-push.

[bors]

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

[workingjubilee]

As noted by @wesleywiser on Aug 28:

Wesley Wiser: We could do this eagerly like this PR currently implements but then it will perhaps trigger more than strictly necessary if the over-aligned static is never actually codegen'd.
Wesley Wiser: We could also delay emitting the error until codegen time (as @workingjubilee mentioned) which makes this more like a post-mono error
Wesley Wiser: Clearly there needs to be an error here, this is a clear-cut case of target/platform limitations and rustc should generate an error when you try to violate them.
Wesley Wiser: So I guess this is more "T-lang do you have a preference for which way we do this?"

So, in the spirit of executing that:

Thanks for this PR, @nthery, and sorry about the holdup. We probably want to have a vibe check from T-lang on whether they have a strong feeling on whether this error should occur eagerly or lazily or what.

@rustbot label: +I-lang-nominated +T-lang

[scottmcm]

(My first instinct, not speaking for the team)

Given that this same place is doing other tcx.sess.target-based checks, it seems plausible to put more here. There's some niceness to "well if it's not codegen'd it doesn't matter", but I think overall that's worse because it reintroduces all the questions about "well, what does used really even mean?" and the risk of unintentional breakage across compiler version updates for something that was formerly not considered used being considered used in the new version.

If the limit was gratuitously low, like align(32), then I'd probably think differently, but hopefully 8192 is high enough that it's unlikely to cause spurious failures.

[folkertdev]

the limit was gratuitously low, like align(32), then I'd probably think differently

For function alignment on wasm we'll need to impose a maximum alignment of 1: the function address is kind of meaningless for wasm, so an alignment of 1 is all we can reasonably guarantee.

The solution that is chosen here still seems like the correct one to me, but there are, or may be, platforms where the limits are substantially lower in some cases.

[Jules-Bertholet]

Even if we reject the maximally permissive “only error if it’s codegened” option, that still leaves a choice between “error at the type declaration site” (most restrictive) and “error at the static item declaration site” (middle ground).

[scottmcm]

If this is specific to the static then that does sound nice, but I don't know that it's strictly necessary to do that.

Any thoughts from people who need extra alignment on things? Are they needed at high-ish numbers across platforms?

[RalfJung]

Note that it's not just statics, it's also promoteds and consts that can give rise to global allocations that end up in the binary. So checking specifically only statics is insufficient.

#[repr(align(16384))]
enum E5 { A, B }

let x = &E5::A; // this gets promoted to a global, so this must also error

[traviscross]

We should document this in the Reference, in:

• https://doc.rust-lang.org/1.90.0/reference/type-layout.html#r-layout.repr.alignment

@nthery, would you perhaps be interested in putting up a PR for that?

[joshtriplett]

Seems like a reasonable restriction to add. FCPing for lang:

@rfcbot merge

[rust-rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[nikomatsakis]

@rfcbot reviewed

[traviscross]

In the lang call today, we talked about this again. In discussion, @scottmcm raised again his question:

(My first instinct, not speaking for the team)

Given that this same place is doing other tcx.sess.target-based checks, it seems plausible to put more here. There's some niceness to "well if it's not codegen'd it doesn't matter", but I think overall that's worse because it reintroduces all the questions about "well, what does used really even mean?" and the risk of unintentional breakage across compiler version updates for something that was formerly not considered used being considered used in the new version.

If the limit was gratuitously low, like align(32), then I'd probably think differently, but hopefully 8192 is high enough that it's unlikely to cause spurious failures.

It'd be good to answer this in the Reference PR, as it may be observable.

I'm going to wait to see the Reference PR before checking a box here.

[scottmcm]

I'm hopeful that 8k is high enough that checking the declaration is good; worst come to worst I think we could change if people show up really needing something else. (I don't think people do align(1048576) for huge-page types or anything, but if people do then we might end up wanting to reconsider.)

@rfcbot reviewed

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔