-
Notifications
You must be signed in to change notification settings - Fork 1.6k
repr(ordered_fields) #3845
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?
repr(ordered_fields) #3845
Conversation
Not to add too many extra colours to the list, but (Note: those three things should cover every case I've seen that uses Whereas Also, while it may be more technical than most users need to understand, it would be helpful if the RFC reiterated the current issues with |
Just as a small point of style the Guide Level Explanation is usually "what would be written in the rust tutorial book", and the Reference Level Explanation is "what could be written into the Rust Reference". This isn't a strict requirement, but personally I'd like to see the Reference Level part written out. Using the present tense, as if the RFC was accepted and implemented. |
add more unresolved questions
Rework struct layout description.
text/3845-repr-ordered-fields.md
Outdated
# Guide-level explanation | ||
[guide-level-explanation]: #guide-level-explanation | ||
|
||
`repr(ordered_fields)` is a new representation that can be applied to `struct`, `enum`, and `union` to give them a consistent, cross-platform, and predictable in memory layout. |
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.
`repr(ordered_fields)` is a new representation that can be applied to `struct`, `enum`, and `union` to give them a consistent, cross-platform, and predictable in memory layout. | |
`repr(ordered_fields)` is a new representation that can be applied to `struct`, `enum`, and `union` to give them a consistent, cross-platform, and predictable in-memory layout. |
"cross-platform" -- the layout will differ when there are different layouts for struct members' types, in particular primitive types can have different alignments which changes the amount of padding.
e.g., #[repr(ordered_fields)] struct S(u8, f64);
doesn't have the same layout on x86_64 and i686
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.
Good point, this will need to be documented as a hazard in the ordered_fields
docs. However, the repr
itself will be cross-platform. For example, #[repr(ordered_fields)] struct Cross([u8; 3], SomeEnum);
will be truly cross-platform (given that SomeEnum
is!).
Co-authored-by: Jacob Lifshay <[email protected]>
Just voicing support for |
Nominating this so that we can do a preliminary vibe-check on it in a lang triage meeting. |
Currently `repr(C)` serves two roles | ||
1. Provide a consistent, cross-platform, predictable layout for a given type | ||
2. Match the target C compiler's struct/union layout algorithm and ABI |
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.
Big fan of doing this split, especially for struct
s. (It's less obvious what choices to make for other things, IMHO, but at least for structs this is something I've wanted for ages, so that for example Layout::extend
can talk about it instead of C
.)
Pondering the bikeshed: declaration_order
or something could also be used to directly say what you're getting.
(This could be contrasted with other potential reprs that I wouldn't expect this RFC to add, but could consider as future work, like a deterministic_by_size_and_alignment
where some restricted set of optimizations are allowed but you can be sure that usize
and NonNull<String>
can be mixed between different types while still getting the "same" field offsets, for example.)
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 think this is also useful for unions, so we don't need to rely on repr(C)
to ensure that all fields of a union
are at offset 0.
This could be contrasted with other potential reprs that I wouldn't expect this RFC to add...
This also works as an argument against names like repr(consistent)
, since there are multiple consistent and useful repr
, making it not descriptive enough.
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 do think that declaration_order
or ordered_fields
is a bit weird on a union
, because of course they're not really in any "order".
It makes me ponder whether we should just have repr(offset_zero)
for union
s to be explicit about it, or something.
(Which makes me think of other things like addressing rust-lang/unsafe-code-guidelines#494 by having a different constructs for "bag of maybeuninit stuff that overlap" vs "distinct options with an active-variant rule for enum-but-without-stored-discriminant". But those are definitely 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.
I don't mind spelling it as repr(offset_zero)
for unions if that helps get this RFC accepted 😄. However, I have a sneaking suspicion that this isn't the contentious part of this RFC.
I know the name isn't optimal (intentionally). This can be hashed out after the RFC is accepted (or even give a different name for all of struct
, union
, and enum
).
The most important bit for me is just that we do the split (for all of struct
, union
, and enum
, to be consistent).
I've updated how enums's tags are specified, now they just defer to whatever |
text/3845-repr-ordered-fields.md
Outdated
|
||
`repr(C)` in edition <= 2024 is an alias for `repr(ordered_fields)` and in all other editions, it matches the default C compiler for the given target for structs, unions, and field-less enums. Enums with fields will be laid out as if they are a union of structs with the corresponding fields. | ||
|
||
Using `repr(C)` in editions <= 2024 triggers a lint to use `repr(ordered_fields)` as a future compatibility lint with a machine-applicable fix. If you are using `repr(C)` for FFI, then you may silence this lint. If you are using `repr(C)` for anything else, please switch over to `repr(ordered_fields)` so updating to future editions doesn't change the meaning of your code. |
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 think this is too noisy. Most code out there using repr(C) is probably fine - IIUC, if you're not targeting Windows or AIX, maybe definitely fine? - and having a bunch of allow(...) across a bunch of projects seems unfortunate.
Maybe we can either (a) only enable the lint for migration, i.e., the next edition's cargo fix would add allows for you or (b) we find some new name... C2 for the existing repr(C) usage to avoid allows. But (b) also seems too noisy to me.
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.
Maybe it could just be an optional edition compatibility lint, so if someone enables e.g. rust_20xx_compatibility
it shows up but otherwise not.
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.
(a) only enable the lint for migration
That was the intention, hence the name edition_2024_repr_c
. I'll make this more clear, that this is intended to be a migration lint.
Rustfix would update to #[repr(ordered_fields)]
to preserve the current behavior. For the FFI crates, #![allow(edition_2024_repr_c)]
at the top of lib.rs
would suffice. If you have a mix of FFI and non-FFI uses of repr(C)
, then you'll have to do the work to figure out which is which, no matter what option is chosen to update repr(C)
- even adding repr(C2)
, since then the FFI use case would need to update all their reprs to repr(C2)
.
Overall, I think this scheme only significantly burdens those who have a mix of FFI and non-FFI uses of repr(C)
. But they were going to be burdened no matter what option was chosen.
|
||
`repr(ordered_fields)` is a new representation that can be applied to `struct`, `enum`, and `union` to give them a consistent, cross-platform, and predictable in-memory layout. | ||
|
||
`repr(C)` in edition <= 2024 is an alias for `repr(ordered_fields)` and in all other editions, it matches the default C compiler for the given target for structs, unions, and field-less enums. Enums with fields will be laid out as if they are a union of structs with the corresponding 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.
This proposal is not sufficient to fix the x86-32 MSVC alignment issue (rust-lang/rust#112480).
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 crux of the issue there is that on that platform, u64
and i64
(and possibly f64
also?) are only guaranteed to be aligned to 4 bytes, but in C structs they are given padding as if aligned to 8. Currently, Rust incorrectly gives these types an align_of
of 8 (matching an analogous bug in MSVC’s alignof
). However, in correcting the align_of
, we can’t change the layout of repr(C)
structs accordingly on editions ≤2024, because that would break interop with C. So repr(C)
on editions ≤2024 can’t be an alias for ordered_fields
as proposed. But that would break people doing manual layout calculations on x86-32 MSVC…
Add
repr(ordered_fields)
and provide a migration path to switch users fromrepr(C)
torepr(ordered_fields)
, then change the meaning ofrepr(C)
in the next edition.This RFC is meant to be an MVP, and any extensions (for example, adding more
repr
s) are not in scope. This is done to make it as easy as possible to accept this RFC and make progress on the issue ofrepr(C)
serving two opposing roles.Rendered
To avoid endless bikeshedding, I'll make a poll if this RFC is accepted with all the potential names for the new
repr
. If you have a new name, I'll add it to the list of names in the unresolved questions section, and will include it in the poll.