Add alignment to the NPO guarantee #114845
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
scottmcm
added
T-lang
|
r? @m-ou-se (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
scottmcm
commented
Aug 15, 2023
| @@ -119,7 +119,7 @@ | |||
| //! # Representation | |||
| //! | |||
| //! Rust guarantees to optimize the following types `T` such that | |||
| //! [`Option<T>`] has the same size as `T`: | |||
| //! [`Option<T>`] has the same size and alignment as `T`: | |||
There was a problem hiding this comment.
These two words are the core of the change; the other places refer back to here.
There was a problem hiding this comment.
We actually also need them to be function-call-ABI compatible, right? Even types with the same size and alignment can have different ABIs.
There was a problem hiding this comment.
That's already guaranteed for a few types, like https://doc.rust-lang.org/stable/std/num/struct.NonZeroU32.html#layout-1.
That said, I'd like to keep this PR+FCP to just the hopefully-obvious and non-controversial addition of alignment, and something else can add more.
There was a problem hiding this comment.
Hm I see. We should probably standardize terminology at some point, "FFI" isn't really clear that it talks about ABI -- after all, for by-ref FFI, you don't need ABI compatibility.
There was a problem hiding this comment.
And avoiding deciding on the terminology for this is part of why I'm happy keeping this to just something we have a word for already 🙃
scottmcm
added
the
I-lang-nominated
This comment has been minimized.
This comment has been minimized.
As far as I know, this is always true already, but it's not in the text of the Option module docs, so I figured I'd bring this up to FCP it.
|
@rfcbot merge This is intentionally both lang (for the null-pointer optimization) and libs-api (for the specific types involved). |
scottmcm
removed
the
I-lang-nominated
|
Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members: 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. |
rfcbot
added
proposed-final-comment-period
This comment was marked as resolved.
This comment was marked as resolved.
rfcbot
added
final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
rfcbot
added
finished-final-comment-period
|
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. |
WaffleLapkin
approved these changes
Sep 1, 2023
|
@bors r+ As per the completed FCP above. |
bors
added
S-waiting-on-bors
|
@bors rollup |
fmease
added a commit
to fmease/rust
that referenced
this pull request
Sep 2, 2023
Add alignment to the NPO guarantee This PR [changes](rust-lang#114845 (comment)) "same size" to "same size and alignment" in the option module's null pointer optimization docs in <https://doc.rust-lang.org/std/option/#representation>. As far as I know, this has been true for a long time in the actual rustc implementation, but it's not in the text of those docs, so I figured I'd bring this up to FCP it. I also see no particular reason that we'd ever *want* to have higher alignment on these. In many of the cases it's impossible, as the minimum alignment is already the size of the type, but even if we *could* do things like on 32-bit we could say that `NonZeroU64` is 4-align but `Option<NonZeroU64>` is 8-align, I just don't see any value in doing that, so feel completely fine closing this door for the few things on which the NPO is already guaranteed. These are basically all primitives, and should end up with the same size & alignment as those primitives. (There's no layout guarantee for something like `Option<[u8; 3]>`, where it'd be at least plausible to consider raising the alignment from 1 to 4 on, say, some hypothetical target that doesn't have efficient unaligned 4-byte load/stores. And even if we ever did start to offer some kind of guarantee around such a type, I doubt we'd put it under the "null pointer" optimization header.) Screenshots for the new examples:  
fmease
added a commit
to fmease/rust
that referenced
this pull request
Sep 2, 2023
Add alignment to the NPO guarantee This PR [changes](rust-lang#114845 (comment)) "same size" to "same size and alignment" in the option module's null pointer optimization docs in <https://doc.rust-lang.org/std/option/#representation>. As far as I know, this has been true for a long time in the actual rustc implementation, but it's not in the text of those docs, so I figured I'd bring this up to FCP it. I also see no particular reason that we'd ever *want* to have higher alignment on these. In many of the cases it's impossible, as the minimum alignment is already the size of the type, but even if we *could* do things like on 32-bit we could say that `NonZeroU64` is 4-align but `Option<NonZeroU64>` is 8-align, I just don't see any value in doing that, so feel completely fine closing this door for the few things on which the NPO is already guaranteed. These are basically all primitives, and should end up with the same size & alignment as those primitives. (There's no layout guarantee for something like `Option<[u8; 3]>`, where it'd be at least plausible to consider raising the alignment from 1 to 4 on, say, some hypothetical target that doesn't have efficient unaligned 4-byte load/stores. And even if we ever did start to offer some kind of guarantee around such a type, I doubt we'd put it under the "null pointer" optimization header.) Screenshots for the new examples:  
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Sep 2, 2023
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#114349 (rustc_llvm: Link to `zlib` on dragonfly and solaris) - rust-lang#114845 (Add alignment to the NPO guarantee) - rust-lang#115427 (kmc-solid: Fix `is_interrupted`) - rust-lang#115443 (feat(std): Stabilize 'os_str_bytes' feature) - rust-lang#115444 (Create a SMIR visitor) - rust-lang#115449 (Const-stabilize `is_ascii`) - rust-lang#115456 (Add spastorino on vacation) r? `@ghost` `@rustbot` modify labels: rollup
RalfJung
pushed a commit
to RalfJung/miri
that referenced
this pull request
Sep 3, 2023
Add alignment to the NPO guarantee This PR [changes](rust-lang/rust#114845 (comment)) "same size" to "same size and alignment" in the option module's null pointer optimization docs in <https://doc.rust-lang.org/std/option/#representation>. As far as I know, this has been true for a long time in the actual rustc implementation, but it's not in the text of those docs, so I figured I'd bring this up to FCP it. I also see no particular reason that we'd ever *want* to have higher alignment on these. In many of the cases it's impossible, as the minimum alignment is already the size of the type, but even if we *could* do things like on 32-bit we could say that `NonZeroU64` is 4-align but `Option<NonZeroU64>` is 8-align, I just don't see any value in doing that, so feel completely fine closing this door for the few things on which the NPO is already guaranteed. These are basically all primitives, and should end up with the same size & alignment as those primitives. (There's no layout guarantee for something like `Option<[u8; 3]>`, where it'd be at least plausible to consider raising the alignment from 1 to 4 on, say, some hypothetical target that doesn't have efficient unaligned 4-byte load/stores. And even if we ever did start to offer some kind of guarantee around such a type, I doubt we'd put it under the "null pointer" optimization header.) Screenshots for the new examples:  
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR changes "same size" to "same size and alignment" in the option module's null pointer optimization docs in https://doc.rust-lang.org/std/option/#representation.
As far as I know, this has been true for a long time in the actual rustc implementation, but it's not in the text of those docs, so I figured I'd bring this up to FCP it.
I also see no particular reason that we'd ever want to have higher alignment on these. In many of the cases it's impossible, as the minimum alignment is already the size of the type, but even if we could do things like on 32-bit we could say that
NonZeroU64is 4-align butOption<NonZeroU64>is 8-align, I just don't see any value in doing that, so feel completely fine closing this door for the few things on which the NPO is already guaranteed. These are basically all primitives, and should end up with the same size & alignment as those primitives.(There's no layout guarantee for something like
Option<[u8; 3]>, where it'd be at least plausible to consider raising the alignment from 1 to 4 on, say, some hypothetical target that doesn't have efficient unaligned 4-byte load/stores. And even if we ever did start to offer some kind of guarantee around such a type, I doubt we'd put it under the "null pointer" optimization header.)Screenshots for the new examples:
