Skip to content
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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

add -Zmin-function-alignment #134030

Merged
merged 1 commit into from
Jan 11, 2025
Merged

add -Zmin-function-alignment #134030

merged 1 commit into from
Jan 11, 2025
+145 −3

Conversation

folkertdev
Copy link
Contributor

tracking issue: #82232

This PR adds the -Zmin-function-alignment=<align> flag, that specifies a minimum alignment for all* functions.

Motivation

This feature is requested by RfL here:

i.e. the equivalents of -fmin-function-alignment (GCC, Clang does not support it) / -falign-functions (GCC, Clang).

For the Linux kernel, the behavior wanted is that of GCC's -fmin-function-alignment and Clang's -falign-functions, i.e. align all functions, including cold functions.

There is feature(fn_align), but we need to do it globally.

Behavior

The fn_align feature does not have an RFC. It was decided at the time that it would not be necessary, but maybe we feel differently about that now? In any case, here are the semantics of this flag:

  • -Zmin-function-alignment=<align> specifies the minimum alignment of all* functions
  • the #[repr(align(<align>))] attribute can be used to override the function alignment on a per-function basis: when -Zmin-function-alignment is specified, the attribute's value is only used when it is higher than the value passed to -Zmin-function-alignment.
  • the target may decide to use a higher value (e.g. on x86_64 the minimum that LLVM generates is 16)
  • The highest supported alignment in rust is 2^29: I checked a bunch of targets, and they all emit the .p2align 29 directive for targets that align functions at all (some GPU stuff does not have function alignment).

*: Only with build-std would the minimum alignment also be applied to std functions.

cc @ojeda

r? @workingjubilee you were active on the tracking issue

@rustbot 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 Dec 8, 2024
@ojeda ojeda mentioned this pull request Dec 8, 2024
Open
52 tasks
@workingjubilee
Copy link
Member

Yes, the 2^29 alignment is a limit imposed by LLVM.

@folkertdev
Copy link
Contributor Author

Yeah what I can't quite figure out is when targets actually support that (e.g. on embedded targets, you probably don't have enough space for .p2align 29). Anyway I guess we're doing the best we can here, and you could already use repr(align(1 << 29)) on these targets and get a broken program.

Also I just added support for #[naked] functions. They emit their own .balign directive directly and therefore need custom logic and testing.

@workingjubilee
Copy link
Member

workingjubilee commented Dec 16, 2024 •
edited
Loading

Yeah, I don't think we need to worry about that. Using broken alignments on targets without virtual memory mapping is already deeply incorrect. If we figure out how we want to fix that, the fix for #[repr(align(N))] struct is more important, tbh.

@folkertdev
Copy link
Contributor Author

So, then, is there anything else that needs to happen here?

@bors
Copy link
Contributor

bors commented Dec 18, 2024

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

@rustbot
Copy link
Collaborator

rustbot commented Dec 18, 2024

Some changes occurred in src/tools/cargo

cc @ehuss

workingjubilee
workingjubilee requested changes Jan 5, 2025
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Mostly for me to have more bandwidth for doing reviews, apparently!

I suppose this should be added to the "unstable book"? src/doc/unstable-book

@rustbot 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 5, 2025
@folkertdev
Copy link
Contributor Author

@rustbot ready

@rustbot 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 8, 2025
workingjubilee
workingjubilee requested changes Jan 10, 2025
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

The documentation seems to be incorrect.

I'd also just like to see something higher than 16 and 32 for a test (or test revision). Let's get sillier. Have one of the values be outside the range of a u8, say.

src/doc/unstable-book/src/compiler-flags/min-function-alignment.md Outdated Show resolved Hide resolved
tests/codegen/min-function-alignment.rs Outdated Show resolved Hide resolved
@rustbot 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 10, 2025
@folkertdev
Copy link
Contributor Author

@rustbot ready

  • fixed the docs
  • we're now also testing for an alignment of 1024 bytes (well what we're testing is that we pass that on to llvm correctly, and we trust that it generates the right things).
  • we now also test that min-function-alignment overrides a repr(align) if the value passed to the flag is higher than the one provided by the attribute.

@rustbot 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 10, 2025
workingjubilee
workingjubilee approved these changes Jan 11, 2025
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

Much better!

@workingjubilee
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 11, 2025

📌 Commit 47573bf has been approved by workingjubilee

It is now in the queue for this repository.

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 11, 2025
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 11, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 11, 2025
@bors
Auto merge of rust-lang#135370 - matthiaskrgr:rollup-g2w6d5n, r=matth…
eb54a50 
…iaskrgr

Rollup of 4 pull requests

Successful merges:

 - rust-lang#134030 (add `-Zmin-function-alignment`)
 - rust-lang#134776 (Avoid ICE: Account for `for<'a>` types when checking for non-structural type in constant as pattern)
 - rust-lang#135205 (Rename `BitSet` to `DenseBitSet`)
 - rust-lang#135314 (Eagerly collect mono items for non-generic closures)

r? `@ghost`
`@rustbot` modify labels: rollup
@RalfJung
Copy link
Member

To be clear, there's no problem linking two crates that use different values for this? The compiler will not assume that all function pointers it sees are sufficiently aligned; it will merely decide to align the functions emitted in this particular session in the described way?

@folkertdev
Copy link
Contributor Author

Yes that's right. You can think of this flag as annotating all functions for which code is generated with

#[repr(align<value>))]

No assumptions are made about code from other crates, extern fns or anything like that.

@bors bors merged commit b8e230a into rust-lang:master Jan 11, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 11, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 11, 2025
@rust-timer
Unrolled build for rust-lang#134030
892c992 
Rollup merge of rust-lang#134030 - folkertdev:min-fn-align, r=workingjubilee

add `-Zmin-function-alignment`

tracking issue: rust-lang#82232

This PR adds the `-Zmin-function-alignment=<align>` flag, that specifies a minimum alignment for all* functions.

### Motivation

This feature is requested by RfL [here](rust-lang#128830):

> i.e. the equivalents of `-fmin-function-alignment` ([GCC](https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#index-fmin-function-alignment_003dn), Clang does not support it) / `-falign-functions` ([GCC](https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#index-falign-functions), [Clang](https://clang.llvm.org/docs/ClangCommandLineReference.html#cmdoption-clang1-falign-functions)).
>
> For the Linux kernel, the behavior wanted is that of GCC's `-fmin-function-alignment` and Clang's `-falign-functions`, i.e. align all functions, including cold functions.
>
> There is [`feature(fn_align)`](rust-lang#82232), but we need to do it globally.

### Behavior

The `fn_align` feature does not have an RFC. It was decided at the time that it would not be necessary, but maybe we feel differently about that now? In any case, here are the semantics of this flag:

- `-Zmin-function-alignment=<align>` specifies the minimum alignment of all* functions
- the `#[repr(align(<align>))]` attribute can be used to override the function alignment on a per-function basis: when `-Zmin-function-alignment` is specified, the attribute's value is only used when it is higher than the value passed to `-Zmin-function-alignment`.
- the target may decide to use a higher value (e.g. on x86_64 the minimum that LLVM generates is 16)
- The highest supported alignment in rust is `2^29`: I checked a bunch of targets, and they all emit the `.p2align        29` directive for targets that align functions at all (some GPU stuff does not have function alignment).

*: Only with `build-std` would the minimum alignment also be applied to `std` functions.

---

cc `@ojeda`

r? `@workingjubilee` you were active on the tracking issue
@ojeda ojeda mentioned this pull request Jan 12, 2025
Open
90 tasks
@bjorn3 bjorn3 mentioned this pull request Jan 15, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@workingjubilee workingjubilee workingjubilee approved these changes

Assignees

@workingjubilee workingjubilee

Labels
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.
Projects
None yet
Milestone
1.86.0
Development

Successfully merging this pull request may close these issues.
5 participants
@folkertdev @workingjubilee @bors @rustbot @RalfJung