-
Notifications
You must be signed in to change notification settings - Fork 1.6k
RFC: Add an attribute for raising the alignment of various items #3806
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
75d8fc6
to
f6f7686
Compare
f6f7686
to
3a4f5ac
Compare
800e69d
to
eca25f7
Compare
eca25f7
to
4badbc9
Compare
text/3806-align-attr.md
Outdated
The `align` attribute is a new inert, built-in attribute that can be applied to | ||
ADT fields, `static` items, function items, and local variable declarations. The | ||
attribute accepts a single required parameter, which must be a power-of-2 | ||
integer literal from 1 up to 2<sup>29</sup>. (This is the same as | ||
`#[repr(align(…))]`.) |
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.
The 2^29 limit is way too high. The consistency with #[repr(align(..))]
is a good default but alignments larger than a page or two have never worked properly in local variables (rust-lang/rust#70143) and in statics (rust-lang/rust#70022, rust-lang/rust#70144). While there are some use cases for larger alignments on types (if they're heap allocated) and an error on putting such a type in a local or static is ugly, for this new attribute we could just avoid the problem from the start.
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.
For a struct field, both GCC and clang supported _Alignas(N)
for N ≤ 228 (268435456).
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.
The bug with local variables (rust-lang/rust#70143) seems to have been fixed everywhere except Windows, and just waiting on someone to fix it there as well in LLVM. (And even on Windows where the issue is not fixed, the only effect is to break the stack overflow protection, bringing it down to the same level as many Tier 2 targets.)
So the only remaining issue is with statics, where it looks like a target-specific max alignment might be necessary. Once implemented, that solution can be used to address align
as well.
Overall, I don't think any of this is sufficient motivation to impose a stricter maximum on #[align]
.
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.
Note that fixing the soundness issue for locals just means that putting a local with huge alignment in a stack frame is very likely to trigger the stack overflow check and abort the program. There is no use case for such massively over-aligned locals or statics, which is why those soundness issues been mostly theoretical problems and why the only progress toward fixing them over many years has been side effects of unrelated improvements (inline stack checks).
The only reason why the repr(align(..))
limit is so enormous is because it’s plausibly useful for heap allocations. Adding a second , lower limit for types put in statics and locals nowadays is somewhat tricky to design and drive to consensus (e.g., there’s theoretical backwards compatibility concerns) and not a priority for anyone, so who knows when it’ll happen. For #[align]
we have the benefit of hindsight and could just mostly side-step the whole mess. I don’t see this as “needlessly restricting the new feature” but rather as “not pointlessly expanding upon an existing soundness issue for no reason”.
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.
There is no use case for such massively over-aligned locals or statics
one use case I can think of is having a massive array that is faster because it's properly aligned so the OS can use huge pages (on x86_64, those require alignment static
s or heap-allocated/mmap
-ed memory.
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.
To use huge pages for static data, you'd want to align the ELF segment containing the relevant sections (or equivalent in other formats), so the right tool there is a linker script or similar platform-specific mechanism. Over-aligning individual static
s is a bad alternative:
- It risks wasting a lot more (physical!) memory than expected if you end up with multiple
static
s in the program doing it and there's not enough other data to fill the padding required between them or they go in different sections. - If the linker/loader ignores the requested section alignment then that leads to UB if you used Rust-level
#[align(N)]
/#[repr(align(N))]
and the code was optimized under that assumption. - While aligning statics appears easier and more portable than linker scripts, the reality is that platform/toolchain support for this is spotty anyway, so you really ought to carefully consider when and where to apply this trick.
In any case, I'm sure I'm technically wrong to claim that nobody could ever come up with a use case for massively over-aligned statics. But there's a reason why Linux and glibc have only started supporting it at all in the last few years, and other environments like musl-based Linux and Windows apparently doesn't support it at all (see discussion in aforementioned issues).
text/3806-align-attr.md
Outdated
In Rust, a type’s size is always a multiple of its alignment. However, there are | ||
other languages that can interoperate with Rust, where this is not the case | ||
(WGSL, for example). It’s important for Rust to be able to represent such | ||
structures. |
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.
It's not clear to me how this would work while keeping Rust's "size is multiple of align" rule intact. I guess if it's about individual fields in a larger aggregate that maintains the rule in total? I don't know anything about WGSL so an example would be appreciated.
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.
That’s exactly it. The WSGL example was taken from this comment on Internals: https://internals.rust-lang.org/t/pre-rfc-align-attribute/21004/20
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.
Adding a worked example would indeed help readers of the RFC on this point.
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.
Here is a concrete example of implementing Rust-WGSL compatibility using the #[align]
attribute defined in this RFC. These structs have the same layout, and together demonstrate both inserting required padding (between foo
and bar
), and allowing a following field to be placed where a wrapper type would demand padding (baz
immediately after bar
):
// WGSL
struct Example { // size = 32, alignment = 16
foo: vec3<f32>, // offset = 0, size = 12
bar: vec3<f32>, // offset = 16, size = 12
baz: f32, // offset = 28, size = 4
}
// Rust
#[repr(linear)] // as defined in this RFC; repr(C) in current Rust
#[derive(Debug, Copy, Clone, bytemuck::Pod, bytemuck::Zeroable)]
pub(crate) struct Example {
#[align(16)]
foo: [f32; 3],
// #[align] below causes 4 bytes of padding to be inserted here to satisfy it.
#[align(16)]
bar: [f32; 3],
baz: f32, // If we used a wrapper for bar, this field would be at offset 32, wrongly
}
It is often possible to order structure fields to fill gaps so that no inter-field padding is needed — such as if the fields in this example were declared in the order {foo, baz, bar}
— and this is preferable when possible to avoid wasted memory, but the advantage of using #[align]
in this scenario is that when used systematically, it can imitate WGSL's layout and thus will be correct even if the field ordering is not optimal.
(Please feel free to use any of the above text in the RFC.)
We discussed this in the lang call today. We were feeling generally favorable about this, but all need to read it more closely. |
Also, justify prohibition on fn params.
text/3806-align-attr.md
Outdated
1. What should the syntax be for applying the `align` attribute to `ref`/`ref | ||
mut` bindings? | ||
|
||
- Option A: the attribute goes inside the `ref`/`ref mut`. | ||
|
||
```rust | ||
fn foo(x: &u8) { | ||
let ref #[align(4)] _a = *x; | ||
} | ||
``` | ||
|
||
- Option B: the attribute goes outside the `ref`/`ref mut`. | ||
|
||
```rust | ||
fn foo(x: &u8) { | ||
let #[align(4)] ref _a = *x; | ||
} | ||
``` |
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.
Whatever we do, I'd expect it to be the same as for mut
. So it's probably not worth deferring this question, as we need to handle it there.
As for where to put it, it seems like a bit of a coin toss. Anyone have a good argument for which way it should go?
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.
I’m comfortable deferring it because I see no use-case for it, and I don’t want to hold up the RFC on something with no use case.
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.
Sure, but still, I repeat my question, as we need to answer it for mut
in any case, about whether there are good arguments for on which side of mut
the attribute should appear.
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.
The mut
case does have actual use-cases, so I think we should handle the issue in the context of that, not this 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.
Oh, wait, I think there may be a misunderstanding here. By “the same as for mut
”, are you referring to combining mut
with ref
/ref mut
?
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.
No. This RFC specifies this is allowed (quoting from an example in the RFC):
let (a, #[align(4)] b, #[align(2)] mut c) = (4u8, 2u8, 1u8);
My question is whether there are good arguments about whether we should prefer that, or should instead prefer:
let (a, #[align(4)] b, mut #[align(2)] c) = (4u8, 2u8, 1u8);
The RFC should discuss any reasons we might want to prefer one over the other.
Separately, and secondarily, my feeling is that if we chose
let #[align(..)] mut a = ..;
then we would also choose:
let #[align(..)] ref a = ..;
And if we instead chose
let mut #[align(..)] a = ..;
then we would choose:
let ref #[align(..)] a = ..;
So my feeling is that in settling the question of how to syntactically combine #[align(..)]
and mut
, we are de facto settling the question of how to combine #[align(..)]
with any other binding mode token.
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.
I don’t agree that we would necessarily want to make the same choice in both cases. I actually think it depends on how mut
and ref
/ref mut
should be combined.
If the combination looks like
let ref (mut x) = …;
let ref mut (mut x) = …;
Then we should also do
let ref (#[align(…)] x) = …;
let ref mut (#[align(…)] x) = …;
But if it looks like
let mut ref x = …;
let mut ref mut x = …;
Then we should do
let #[align(…)] ref x = …;
let #[align(…)] ref mut x = …;
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.
In that event, and in your model, that would still leave us deciding between:
let ref (mut #[align(..)] x) = ..; // 1
// vs
let ref (#[align(..)] mut x) = ..; // 2
And between:
let #[align(..)] mut ref x = ..; // 3
// vs
let mut #[align(..)] ref x = ..; // 4
I would estimate that we'd comfortably favor 1, 3 over 2, 4.
There are also, of course, these possibilities:
let #[align(..)] ref (mut x) = ..; // 5
let mut ref #[align(..)] x = ..; // 6
If in this RFC we pick #[align(..) mut x
, that would rule out for me option 1 if we later did ref (mut x)
(and I wouldn't pick option 2 anyway). If we pick mut #[align(..)] x
, that would rule out for me option 3 if we later did mut ref x
(and I wouldn't pick option 4 anyway).
That is, even in this future possibility, I'm going to want to keep all of the binding mode tokens either to the left or to the right of the attribute.
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.
I’ll elaborate in the RFC, but my preference is for 2 or 3.
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.
I’ve added a section on this to the RFC.
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
…e, r=jdonszelmann use `#[align]` attribute for `fn_align` Tracking issue: rust-lang#82232 rust-lang/rfcs#3806 decides to add the `#[align]` attribute for alignment of various items. Right now it's used for functions with `fn_align`, in the future it will get more uses (statics, struct fields, etc.) (the RFC finishes FCP today) r? `@ghost`
Rollup merge of #142507 - folkertdev:fn-align-align-attribute, r=jdonszelmann use `#[align]` attribute for `fn_align` Tracking issue: #82232 rust-lang/rfcs#3806 decides to add the `#[align]` attribute for alignment of various items. Right now it's used for functions with `fn_align`, in the future it will get more uses (statics, struct fields, etc.) (the RFC finishes FCP today) r? `@ghost`
…szelmann use `#[align]` attribute for `fn_align` Tracking issue: rust-lang/rust#82232 rust-lang/rfcs#3806 decides to add the `#[align]` attribute for alignment of various items. Right now it's used for functions with `fn_align`, in the future it will get more uses (statics, struct fields, etc.) (the RFC finishes FCP today) r? `@ghost`
@rfcbot concern specify-behavior-with-traits As discussed in the stabilization of I've asked @Jules-Bertholet to update the language in the RFC for this. I'll file a concern so that, on resolving it after we review this new language in a lang call, that we'll have restarted the FCP clock. |
I’ve updated the RFC in response to comments received after FCP. |
The maximum alignment is the same as `#[repr(align(…))]`: 2<sup>29</sup>. That | ||
being said, some targets may not be able to support very high alignments in all | ||
contexts. In such cases, the compiler must impose a lower limit for those | ||
specific contexts on those specific targets. The compiler may choose to emit a | ||
lint warning for high, non-portable alignment specifiers. |
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.
For background, clang defines this maximum alignment here
uint64_t MaximumAlignment = Sema::MaximumAlignment;
if (Context.getTargetInfo().getTriple().isOSBinFormatCOFF())
MaximumAlignment = std::min(MaximumAlignment, uint64_t(8192));
if (Alignment > MaximumAlignment) {
Diag(AttrLoc, diag::err_attribute_aligned_too_great)
<< MaximumAlignment << E->getSourceRange();
return;
}
/// The maximum alignment, same as in llvm::Value. We duplicate them here
/// because that allows us not to duplicate the constants in clang code,
/// which we must to since we can't directly use the llvm constants.
/// The value is verified against llvm here: lib/CodeGen/CGDecl.cpp
///
/// This is the greatest alignment value supported by load, store, and alloca
/// instructions, and global values.
static const unsigned MaxAlignmentExponent = 32;
static const uint64_t MaximumAlignment = 1ull << MaxAlignmentExponent;
So clang's limit is u32::MAX
, or 8192 when the binary format is COFF.
However, some linkers may not support such high alignments even when the binary format does. However it looks like support is gradually added, see rust-lang/rust#70022 (comment)
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.
That seems off because LLVM has a similar limit but it's expressed in bits (that's why we cap out at 29).
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.
Well this is fun...
https://godbolt.org/z/j4fvnnfec
// In the LLVM IR, this is only aligned to 16
char big_aligned_buffer[4096] __attribute__((aligned((1 << 32))));
// Aligned properly in LLVM IR (and directives) to `1 << 28`
__attribute__((aligned(1 << 28))) void aligned_to_28(void) {}
// Alignment just gets silently ignored
__attribute__((aligned(1 << 29))) void aligned_to_29(void) {}
So apparently for statics LLVM already imposes a limit of 16 (can be higher if the type requires it though), and for functions clang accepts higher alignments than 1 << 28
but then LLVM just silently ignores them.
All of this is to say: specifying the behavior will be tough, and it really does look like LLVM doesn't have our back.
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.
Thanks for looking at that!
That's both hilarious and sad.
Co-authored-by: Jubilee <[email protected]>
The generated implementation for the trait’s `dyn` type will also provide this | ||
alignment. `impl` blocks do not have to repeat the `#[align(n)]` attribute, it | ||
is implicit. (This enables the trait definition to raise `n` without breaking | ||
implementations.) That being said, `impl` blocks are free to specify an | ||
`align(…)` higher than that provided by the trait. (If they specify a *lower* | ||
alignment, it will simply be ignored.) |
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.
This at least resolves the semver hazard (which is an improvement). I'm still not sure what the use-case is for this such that we should allow it, since it mostly seems like it may inhibit optimizations by preventing unification of instances of the code, but in an unreliable way. If that is a use-case, it is possible to prevent code from being unified in more reliable ways. But it does have consistency with #[align]
specified twice at least.
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.
...Indeed it's actually not clear to me that we can prevent optimizations like identical code folding from collapsing code with differing alignments, thus allowing people to rely on alignment of the code but with unsound consequences? I don't know that this will be an issue for free functions, but it seems more likely to happen to traits.
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.
Higher alignments fulfill lower alignments, so when merging together otherwise identical functions, the compiler can just keep the most-aligned one.
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.
I am not sure if that is the semantic LLVM actually implements? https://godbolt.org/z/7xK78v61f Meh, nevermind, got confused again.
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.
@aligned_to_2p8()
is unused and therefore discarded. If you remove @aligned_to_default()
, the output will end up entirely empty
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.
Neat, found it works correctly: https://godbolt.org/z/WzT7c9ex6
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.
I believe the correct semantic, from Rust's perspective, is "it doesn't matter much what address this is at, as long as it is correctly aligned". I have opened an issue about implementing this in LLVM: llvm/llvm-project#145123
I believe there are mitigations we can pursue on our side, but it may come at notable cost.
Port C
alignas
to Rust.Rendered