Skip to content

error out when repr(align) exceeds COFF limit #142638

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nthery
Copy link

@nthery nthery commented Jun 17, 2025

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
Copy link
Collaborator

rustbot commented Jun 17, 2025

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 rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) 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 Jun 17, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 17, 2025

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

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

cc @jdonszelmann

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jieyouxu jieyouxu added the A-align Area: alignment control (`repr(align(N))` and so on) label Jun 17, 2025
@workingjubilee
Copy link
Member

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

@folkertdev
Copy link
Contributor

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
Copy link
Member

@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
Copy link
Member

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

@workingjubilee
Copy link
Member

fbbth.

@bors
Copy link
Collaborator

bors commented Jun 18, 2025

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

@nthery
Copy link
Author

nthery commented Jun 20, 2025

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.

@rustbot

This comment has been minimized.

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 21, 2025
@rust-log-analyzer

This comment has been minimized.

@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 21, 2025
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

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

nthery commented Jun 23, 2025

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

@bors
Copy link
Collaborator

bors commented Jun 23, 2025

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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-align Area: alignment control (`repr(align(N))` and so on) A-attributes Area: Attributes (`#[…]`, `#![…]`) 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE of SIGILL/STATUS_ILLEGAL_INSTRUCTION when compiling static variables with 2MB alignment targeting Windows
9 participants