-
Notifications
You must be signed in to change notification settings - Fork 501
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
Specify guarantees for repr(rust) structs #1152
Specify guarantees for repr(rust) structs #1152
Conversation
The exception to this is the unit tuple (`()`), which is guaranteed as a | ||
zero-sized type to have a size of 0 and an alignment of 1. |
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.
Does the former statement imply this statement?
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.
Not as I wrote it here, but you could certainly argue that it should be changed so it does.
src/type-layout.md
Outdated
3. The alignment of the struct is not less than the alignment of any of its | ||
fields. |
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.
Should we guarantee that the alignment is equal to the largest alignment of the fields?
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.
If it ever becomes possible for PGO to change the layout of rust structs, it may be beneficial to increase alignment for certain types in some cases.
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.
(Can be ignored by author of PR)
Is it within the scope of the reference to include non-normative statements like:
"In practice, there's no reason a compiler would ever require a type to be more aligned than this, but we would like to reserve the right to do so, in case such a situation is found." (Possibly also noting that there isn't any clear reason why a user would need to care about this unless they were doing something that probably motivates using other repr annotations anyway, so it's basically just erring on the side of compiler freedom in a situation that doesn't much matter either way.).
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 would avoid saying anything about an upperbound on alignment for repr(Rust), especially if that statement isn't normative, because it might confuse what is guaranteed.
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.
Should we guarantee that the alignment is equal to the largest alignment of the fields?
I would say no. I'd like the freedom to be able to have struct Foo([u8; 16]);
automatically get align(4)
, for example, if not everywhere at least on anything that doesn't like unaligned large reads.
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.
Or, for that case, align(16) on x86_64, so you can use aligned SSE reads (movaps
) to copy it.
src/type-layout.md
Outdated
fields can be ordered such that the offset plus the size of any field is less | ||
than or equal to the offset of the next field in the ordering. The ordering does | ||
not have to be the same as the order in which the field are specified in the | ||
declaration of the struct. |
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.
(also can be ignored)
If non-normative statements that provide motivation/context are permitted, it would also be worth noting here that only the compiler has enough information to optimally lay out a generic type for all possible type substitutions. i.e. repr(C)
gets you consistency and control at the cost of sometimes not being able to always put generic fields in the best place.
I'm not sure who is to review this PR, but as the author of #1151, I wanted to make explicit that IMO, merging this PR would completely resolve this issue. My thanks to the author of the PR. |
Nominating for t-lang. These seem like reasonable guarantees to add to me, and want to get your input if you agree or have comments. The only open question I had was would it help to define the layout of |
This would be very helpful. E.g., in zerocopy, we want to implement |
@rfcbot merge |
@Darksonn have you seen this write-up here? https://rust-lang.github.io/unsafe-code-guidelines/layout/structs-and-tuples.html I didn't do a detailed comparison, but I would like to ensure they are compatible as that text had a fair bit of thought put into it :) |
Co-authored-by: Josh Triplett <[email protected]>
No, I haven't looked at that one in detail either. |
Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns:
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! See this document for info about what commands tagged team members can give me. |
src/type-layout.md
Outdated
@@ -86,9 +86,10 @@ String slices are a UTF-8 representation of characters that have the same layout | |||
|
|||
## Tuple Layout | |||
|
|||
Tuples do not have any guarantees about their layout. | |||
Tuples have the same layout guarantees as a struct with the same fields when |
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 opposed to documenting this, as it precludes us from tuple variadic approaches which require "tail borrowing."
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.
@rfcbot concern tuple-tail-borrowing
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.
Can you elaborate on what that is?
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 have been a number of discussions in relation to variadic generics about wanting the ability to write code which could recurse over the structure of a tuple, HList-style. In order for this to work, the "tail" of a tuple must be represented contiguously. That is, for tuple (A, B, C, D)
, there must be a sub-tuple (B, C, D)
in memory. Allowing arbitrary ordering as structs do would cause layouts like (A, D, C, B)
, from which it's impossible to create a reference to a tuple of a sub-structure like (B, C, D)
.
Note that hlist-compatible representations of tuples need not be strictly ordered-- it's fine for each new element to be placed at either the front, back, or even inside of the tuple so long as the tail tuple remains a part of the representation.
I'd like our representation documentation to stay forward-compatible with tuple tail-borrowing until we've explicitly decided not to do it so that users aren't expecting to be able to transmute between struct S(A, B, C, D);
and tuple (A, B, C, D)
.
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.
As I intended the guarantees I have written, what you described here would still be backwards-compatible to introduce, because that particular ordering is just one among the many different orderings that the guarantees allow the compiler to choose. As for the struct, you also can't transmute between different #[repr(Rust)]
structs with the same fields either.
I would be happy to elaborate on this in the text though.
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 see, I misunderstood "have the same layout guarantees as a struct with the same fields". I over-read this to mean "have the same layout as a struct with the same fields". Thanks for clarifying! Still, I can imagine this documentation going stale: if we provide more guarantees for structs in the future, we wouldn't necessarily match those guarantees for tuples.
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 tend to think that tuples and tuple structs ought to have analogous layout. The UCG conversation at least came to that conclusion (we do discuss "tail references" as well). There was talk, as I recall, of making (T, T, T)
sort of special case, to permit casting that to [T; 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.
What do you think of the new phrasing?
@cramertj It looks like your concern didn't get registered; can you try again in a top-level comment? |
src/type-layout.md
Outdated
@@ -86,9 +86,9 @@ String slices are a UTF-8 representation of characters that have the same layout | |||
|
|||
## Tuple Layout | |||
|
|||
Tuples do not have any guarantees about their layout. | |||
Tuples are laid out according to the default representation. |
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.
What does "the default representation" refer to here?
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 section with most of my changes has the heading "The Default Representation", and that's what I'm referring to. I don't know if there's a better way to phrase it, or perhaps add a link to the heading?
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.
+1 a link to the heading would be great, and with an explicit note that they may not be the same representation as a struct with equivalent fields.
@rfcbot concern tuple-layout See #1152 (comment) |
What do I need to do to get this PR finished? I believe that the tuple-layout concern is resolved per the conversation above. |
@rfcbot resolve tuple-layout I think the intention is to clarify here that tuple layout and struct layout are not guaranteed to be the same. With that in mind, the rest of this looks great to me! Sorry for forgetting this thread for ages :( |
🔔 This is now entering its final comment period, as per the review above. 🔔 psst @joshtriplett, I wasn't able to add the |
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. psst @joshtriplett, I wasn't able to add the |
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! I pushed a commit to link to the default representation.
Update books ## nomicon 1 commits in d880e6ac2acf133dce640da24b9fb692844f02d4..f53bfa056929217870a5d2df1366d2e7ba35096d 2022-08-24 12:42:34 -0700 to 2022-09-05 07:19:02 -0700 - Small typo (rust-lang/nomicon#379) ## reference 9 commits in f62e93c28323ed9637d0a205a0c256498674a509..a7cdac33ca7356ad49d5c2b5e2c5010889b33eee 2022-08-28 10:01:28 -0700 to 2022-09-19 17:39:58 -0700 - Clarify wording for references. (rust-lang/reference#1223) - Update Unicode reference to match rustc implementation (rust-lang/reference#1271) - Add documentation for raw-dylib and link_ordinal (rust-lang/reference#1244) - Specify guarantees for repr(rust) structs (rust-lang/reference#1152) - Classify AsyncBlockExpression as ExpressionWithoutBlock (rust-lang/reference#1268) - Update closure-expr.md (rust-lang/reference#1269) - Clarify that 0 is a valid multiple of a type's alignment (rust-lang/reference#1260) - Remove `ne` from derive example (rust-lang/reference#1264) - Clarify reference on async blocks (rust-lang/reference#1262) ## book 6 commits in 0a5421ceb238357b3634fb75234eba4d1dad643c..f1e5ad844d0c61738006cdef26227beeb136948e 2022-08-28 19:51:04 -0400 to 2022-09-19 09:48:21 -0400 - Fix punctuation in ch05-02 - Ownership move chapter link fix - Wrong listing number - Reword text around box - `Box<T>` instead of "box" - Update Clippy output in Appendix D ## rust-by-example 2 commits in 03301f8ae55fa6f20f7ea152a517598e6db2cdb7..767a6bd9727a596d7cfdbaeee475e65b2670ea3a 2022-08-14 08:51:44 -0300 to 2022-09-14 09:17:18 -0300 - struct_visibility.md: Remove unneeded '#[allow(dead_code)]' (rust-lang/rust-by-example#1609) - Fix assorted typos (rust-lang/rust-by-example#1601) ## rustc-dev-guide 15 commits in 04892c1..f587d6e 2022-08-29 20:07:51 +0200 to 2022-09-20 07:43:59 +0900 - Update stability guide to use CURRENT_RUSTC_VERSION (rust-lang/rustc-dev-guide#1468) - Add a note about building `rust-analyzer-proc-macro-srv` (rust-lang/rustc-dev-guide#1467) - Link from "implementing to new features" to mcp.md (rust-lang/rustc-dev-guide#1465) - remove stray ** - Explain the new valtree system for type level constants. (rust-lang/rustc-dev-guide#1097) - fix typos and formatting - Say "bootstrap" instead of "rustbuild"; the latter is not explained anywhere and is not much more clear. - Rewrite the section on passing flags to subcommands - Remove the diagram of all outputs generated by x.py - "symbol names" => ABI - Add symbol-addition to the how-to for new features (rust-lang/rustc-dev-guide#1457) - Fix typo (rust-lang/rustc-dev-guide#1459) - Document multipart_suggestion derive on SessionSubdiagnostic - Add reference for updating Windows PATH and fix typo - Update for removal of RLS (rust-lang/rustc-dev-guide#1450) ## embedded-book 1 commits in befe6840874311635c417cf731377f07234ee373..4ce51cb7441a6f02b5bf9b07b2eb755c21ab7954 2022-07-25 07:51:14 +0000 to 2022-09-15 08:53:09 +0000 - Create CITATION.bib (as per rust-embedded/book#327) (rust-embedded/book#329)
Update books ## nomicon 1 commits in d880e6ac2acf133dce640da24b9fb692844f02d4..f53bfa056929217870a5d2df1366d2e7ba35096d 2022-08-24 12:42:34 -0700 to 2022-09-05 07:19:02 -0700 - Small typo (rust-lang/nomicon#379) ## reference 9 commits in f62e93c28323ed9637d0a205a0c256498674a509..a7cdac33ca7356ad49d5c2b5e2c5010889b33eee 2022-08-28 10:01:28 -0700 to 2022-09-19 17:39:58 -0700 - Clarify wording for references. (rust-lang/reference#1223) - Update Unicode reference to match rustc implementation (rust-lang/reference#1271) - Add documentation for raw-dylib and link_ordinal (rust-lang/reference#1244) - Specify guarantees for repr(rust) structs (rust-lang/reference#1152) - Classify AsyncBlockExpression as ExpressionWithoutBlock (rust-lang/reference#1268) - Update closure-expr.md (rust-lang/reference#1269) - Clarify that 0 is a valid multiple of a type's alignment (rust-lang/reference#1260) - Remove `ne` from derive example (rust-lang/reference#1264) - Clarify reference on async blocks (rust-lang/reference#1262) ## book 6 commits in 0a5421ceb238357b3634fb75234eba4d1dad643c..f1e5ad844d0c61738006cdef26227beeb136948e 2022-08-28 19:51:04 -0400 to 2022-09-19 09:48:21 -0400 - Fix punctuation in ch05-02 - Ownership move chapter link fix - Wrong listing number - Reword text around box - `Box<T>` instead of "box" - Update Clippy output in Appendix D ## rust-by-example 2 commits in 03301f8ae55fa6f20f7ea152a517598e6db2cdb7..767a6bd9727a596d7cfdbaeee475e65b2670ea3a 2022-08-14 08:51:44 -0300 to 2022-09-14 09:17:18 -0300 - struct_visibility.md: Remove unneeded '#[allow(dead_code)]' (rust-lang/rust-by-example#1609) - Fix assorted typos (rust-lang/rust-by-example#1601) ## rustc-dev-guide 15 commits in 04892c1..f587d6e 2022-08-29 20:07:51 +0200 to 2022-09-20 07:43:59 +0900 - Update stability guide to use CURRENT_RUSTC_VERSION (rust-lang/rustc-dev-guide#1468) - Add a note about building `rust-analyzer-proc-macro-srv` (rust-lang/rustc-dev-guide#1467) - Link from "implementing to new features" to mcp.md (rust-lang/rustc-dev-guide#1465) - remove stray ** - Explain the new valtree system for type level constants. (rust-lang/rustc-dev-guide#1097) - fix typos and formatting - Say "bootstrap" instead of "rustbuild"; the latter is not explained anywhere and is not much more clear. - Rewrite the section on passing flags to subcommands - Remove the diagram of all outputs generated by x.py - "symbol names" => ABI - Add symbol-addition to the how-to for new features (rust-lang/rustc-dev-guide#1457) - Fix typo (rust-lang/rustc-dev-guide#1459) - Document multipart_suggestion derive on SessionSubdiagnostic - Add reference for updating Windows PATH and fix typo - Update for removal of RLS (rust-lang/rustc-dev-guide#1450) ## embedded-book 1 commits in befe6840874311635c417cf731377f07234ee373..4ce51cb7441a6f02b5bf9b07b2eb755c21ab7954 2022-07-25 07:51:14 +0000 to 2022-09-15 08:53:09 +0000 - Create CITATION.bib (as per rust-embedded/book#327) (rust-embedded/book#329)
…ieyouxu Add language tests for aggregate types This adds some tests for struct and union types, ensuring that they satisfy the rules for all structs and unions - namely that fields of structs do not overlap, fields are well-aligned, and the size of the entire. The reference annotations used are from rust-lang/reference#1654, though the rules tested here were FCPed in <rust-lang/reference#1152>.
…ieyouxu Add language tests for aggregate types This adds some tests for struct and union types, ensuring that they satisfy the rules for all structs and unions - namely that fields of structs do not overlap, fields are well-aligned, and the size of the entire. The reference annotations used are from rust-lang/reference#1654, though the rules tested here were FCPed in <rust-lang/reference#1152>.
Rollup merge of rust-lang#133355 - chorman0773:spec-layout-tests, r=jieyouxu Add language tests for aggregate types This adds some tests for struct and union types, ensuring that they satisfy the rules for all structs and unions - namely that fields of structs do not overlap, fields are well-aligned, and the size of the entire. The reference annotations used are from rust-lang/reference#1654, though the rules tested here were FCPed in <rust-lang/reference#1152>.
Closes: #1151