-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Stabilize fn_align
: #[align(N)]
on functions and -Zmin-function-alignment
#140261
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.
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
base: master
Are you sure you want to change the base?
Conversation
Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred to the CTFE machinery The Miri subtree was changed cc @rust-lang/miri Some changes occurred in compiler/rustc_passes/src/check_attr.rs |
This comment has been minimized.
This comment has been minimized.
ca73e7d
to
be84cd1
Compare
This comment has been minimized.
This comment has been minimized.
be84cd1
to
3613a1f
Compare
We talked about this at some length in the @rust-lang/lang call today. In particular, we focused on whether to spell this We looked at @Jules-Bertholet's pre-RFC here: https://internals.rust-lang.org/t/pre-rfc-align-attribute/21004/27 We were interested, @Jules-Bertholet, in seeing that made into an RFC, and we'd invite you to submit that and nominate it for us. It does feel to me that We did discuss whether maybe it should just be
For my part, I agree with all of those considerations. We wanted to open it up for the thoughts of others to see what other good ideas or arguments here might emerge. What do you think? |
I’m hesitant to do so at present because I don’t know enough about MSVC to be confident that its alignment-related weirdness won’t be a problem. I don't want the RFC comments to turn into a rehash of the same issues being debated at rust-lang/rfcs#3718. I would want to either see that settled and merged, or for the lang team to agree that they would be interested in the RFC even with those parts left explicitly unresolved. |
Yes, 100%.
Yes, it's mostly embedded and OS projects from what I can tell. These same projects would also get a bunch of mileage out of
Accepting
I really like this framing! Currently the reference for the
The language of this section is in terms of types. That makes sense, and is useful. It can be reworked, but I think including functions and statics here would get messy.
The existing text already has some trouble with There is also https://doc.rust-lang.org/nomicon/other-reprs.html#repralignn, again under the "data layout" heading, but honestly that could probably use some work (an example, talking about addresses, maybe other things). @Jules-Bertholet that's understandable. From my listening in to the conversation, it seemed like what T-lang is looking for is some sort of coherent design for A procedural question, given that resolving these questions around the attribute will likely take a while: what would the process be for just stabilizing the |
In my view, we'd still want lang on that, as it amounts to adding Probably I'd just suggest giving us a little while to see if we can sort this out as-is.
@Jules-Bertholet, like @folkertdev said, we're mostly interested here in whether there's a coherent notion of |
RFC is up: rust-lang/rfcs#3806 |
@rustbot labels -I-lang-nominated +I-lang-radar Let's drop the nomination temporarily to first tackle rust-lang/rfcs#3806. |
☔ The latest upstream changes (presumably #142697) made this pull request unmergeable. Please resolve the merge conflicts. |
3613a1f
to
55341da
Compare
This comment has been minimized.
This comment has been minimized.
55341da
to
9fb0416
Compare
Some changes occurred in compiler/rustc_attr_parsing |
This comment has been minimized.
This comment has been minimized.
9fb0416
to
fe31811
Compare
Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 |
Now that we've agreed on using I've made a reference PR at rust-lang/reference#1866, and split out one final change to an error message into #142715 so that the stabilization PR can be a bit cleaner. |
fn_align
: #[repr(align(N))]
on functions and -Zmin-function-alignment
fn_align
: #[align(N)]
on functions and -Zmin-function-alignment
If you could, let's please answer each of the unresolved questions in the PR description, or link to the answer for each. |
I've provided the answers, linking back to the RFC text/discussion. For the cranelift question I've used my judgement: nobody has objected to going ahead here without cranelift support, that backend has many unsupported features, and support will arrive eventually (i.e. there is no fundamental limitation, it's just not supported yet). |
@Jules-Bertholet: I'm interested in seeing your approving review that everything in this stabilization is what you'd expect given the RFC. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@traviscross LGTM, as far as #[align(…)]
goes. (-Cmin-function-alignment
isn’t part of the RFC, so I have nothing to say about that.)
@wesleywiser / @davidtwco / @apiraino: If you could, I'd be interested in your confirmation that all the necessary steps have been taken to lead up to proposing |
tracking issue: #82232
closes #82232
Request for Stabilization
Summary
The
fn_align
feature has two parts:#[align(N)]
function attribute specifies the alignment of the generated code of a particular function-Zmin-function-alignment
command line flag specifies the minimal alignment of the generated code of all compiled functions.These could be stabilized separately, but share a lot of the same logic in the compiler and are tracked by the same tracking issue.
An example of specifying function alignment:
Specifying the alignment of a specific function occasionally comes up in embedded development. For instance in risc-v vector tables, the function pointers stored in it must be aligned to at least 4 bytes (see #75072).
Specifying a global alignment is more commonly done as a performance optimization. This functionality is used in the linux kernel and this feature flag was requested by Rust for Linux #128830.
Semantics
The
-Zmin-function-alignment
flag is a global mimimum function alignment. It is semantically equivalent to annotating every function with a#[align(N)]
. However, this minimum is only applied to functions that are actually codegened, so without-Zbuild-std
or similar, this minimum alignment is not applied to standard library functions.The function alignment is forwarded to the code generation backend, and behaves as follows:
2.pow(29)
.Documentation
reference PR: rust-lang/reference#1866
Tests
-Zmin-function-alignment
flag and how it interacts with#[align(N)]
.History
repr(align = x)
on inherent methods #110313-Zmin-function-alignment
#134030The
-Zmin-function-alignment
flag was requested by rust-for-linux #128830. It will be used soon (see #t-compiler/help > ✔ Alignment for function addresses).Miri supports function alignment since #140072. In const-eval there is no way to observe the address of a function pointer, so no special attention is needed there (see #t-compiler/const-eval > function address alignment).
In rust-lang/rfcs#3806 it was decided to use
#[align]
for the attribute, rather than the earlier#[repr(align)]
.Notes
Resolved questions
Cranelift support is not required. There are already many features that it does not support, support for this feature will land eventually.
rust-lang/rfcs#3806 specifies that the
#[align]
is applied to the function that returns theFuture
.This was discussed in rust-lang/rfcs#3806 (comment), but the final RFC text says to mirror
#[repr(align(...))]
. If the rules for#[repr(align(...))]
ever change,#[align]
will have to follow suit.unresolved questions
None
r? @traviscross
@rustbot label +I-lang-nominated