Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 42 commits into
base: master
Choose a base branch
from
Open

repr(ordered_fields) #3845

Changes from 1 commit
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
e0cdce8
Create 0000-repr-ordered-fields.md
RustyYato Aug 1, 2025
0637bbf
Add a link to the zullip thread
RustyYato Aug 1, 2025
7e4b0c8
Update RFC with PR number
RustyYato Aug 5, 2025
61ce0f2
Grammer fixes
RustyYato Aug 5, 2025
4fa572e
Update Motivation to include the exact issue from MSVC
RustyYato Aug 5, 2025
755644c
Rework reference level section
RustyYato Aug 5, 2025
0d0a79b
Add description for union layout
RustyYato Aug 5, 2025
4f4bf18
Tabs -> Spaces
RustyYato Aug 5, 2025
063af08
Apply suggestions from code review
RustyYato Aug 5, 2025
0c0e429
discriminant -> tag
RustyYato Aug 5, 2025
9e316ff
Qualify alignment assumptions
RustyYato Aug 5, 2025
a1700b6
oops, fix typo
RustyYato Aug 6, 2025
d75a497
add `repr(declaration_order)` as a potential spelling
RustyYato Aug 6, 2025
6526f5a
Switch enum tag type to defer to `repr(C)` tag type
RustyYato Aug 6, 2025
ed96c1b
Rework edition_2024_repr_c warning from future compat to edition warning
RustyYato Aug 7, 2025
01cfb40
Add lints to summary section
RustyYato Aug 7, 2025
f11567c
edition migraiton lint -> edition compatibility lint
RustyYato Aug 7, 2025
b17f192
fix code example of layout algorithm for enums
RustyYato Aug 7, 2025
488068e
Add reference to MSVC bug in motivation
RustyYato Aug 11, 2025
93f53a5
Add the suspicious_repr_c lint
RustyYato Aug 11, 2025
a2737d5
specify precedence of `suspicious_repr_c` lint
RustyYato Aug 11, 2025
bb8a392
minor grammer/punctuation fixes
RustyYato Aug 11, 2025
612b99e
Fix MSVC bug description
RustyYato Aug 11, 2025
283df46
Update text/3845-repr-ordered-fields.md
RustyYato Aug 18, 2025
8fe1576
Update from reviews
RustyYato Aug 18, 2025
489b31a
fix typo
RustyYato Aug 18, 2025
a5fb9bc
Fix typo
RustyYato Aug 18, 2025
88f631e
Add AIX to motivation
RustyYato Aug 20, 2025
dee831d
Add some description for why it's a problem to conflate the two roles…
RustyYato Aug 21, 2025
f007f6f
wording improvements for the motivation
RustyYato Aug 21, 2025
703dcb9
fix typo
RustyYato Aug 21, 2025
2e36454
Apply suggestions from code review
RustyYato Sep 3, 2025
472cae7
Minor rewordings
RustyYato Sep 3, 2025
6c48b17
Add sections to motivation and expand AIX to it's own section
RustyYato Sep 3, 2025
2043a02
try fix intradoc link?
RustyYato Sep 3, 2025
4c81c7c
Minor rewording of motivation section
RustyYato Sep 3, 2025
92eb763
Add unresolved question for `repr(C)` where corrosponding C type does…
RustyYato Sep 3, 2025
9fb58ad
fix link
RustyYato Sep 3, 2025
6a16195
`align` -> `__alignof__` on AIX
RustyYato Sep 3, 2025
e7bd72a
Apply suggestions from code review
RustyYato Sep 3, 2025
7d7b01a
update motivation section for AIX
RustyYato Sep 3, 2025
81a1a42
Update AIX section and minor rewording throughout
RustyYato Sep 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 4 additions & 9 deletions text/3845-repr-ordered-fields.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,6 @@ struct SomeFFI([i64; 0]);

Of course, making `SomeFFI` size 8 doesn't work for anyone using `repr(C)` for case 1. They want it to be size 0 (as it currently is).
Copy link
Member

@RalfJung RalfJung Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Of course, making `SomeFFI` size 8 doesn't work for anyone using `repr(C)` for case 1. They want it to be size 0 (as it currently is).
Of course, making `SomeFFI` size 8 doesn't work for anyone using `repr(C)` for case 1. They want it to be size 0 (as it currently is).
Fixing the layout of this struct will also most likely let us remove a long-standing hack from our implementation of the MSVC ABI:
unlike all other ABIs, we do *not* entirely skip ZST with that ABI but instead mark them to be passed by-ptr.
This is needed to correctly pass types like `SomeFFI`.
If we fix our layout computation to match that of MSVC, we no longer need this special case in the ABI logic.

Copy link
Member

@RalfJung RalfJung Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out this was not entirely right, see rust-lang/unsafe-code-guidelines#552 (comment). So probably best to remove this paragraph again.

Copy link
Contributor

@Jules-Bertholet Jules-Bertholet Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That issue should presumably be addressed in this RFC, no? repr(C) 1-ZSTs need to be passed by pointer, but does that apply to repr(ordered_fields) as well?

Copy link
Member

@RalfJung RalfJung Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main problem is that we have already specified that all 1-ZST have the same ABI. We need to figure out how to clean that up but I don't think this RFC needs to do that.

Copy link
Contributor

@Jules-Bertholet Jules-Bertholet Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This RFC currently specifies that new repr(C) matches C. That requires breaking the “all 1-ZST have the same ABI” rule. So the RFC either needs to specify that it’s actually “repr(C) matches C except for 1-ZSTs”, or it needs to specify that we are breaking the rule. Can’t have it both ways

Copy link
Author

@RustyYato RustyYato Sep 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did see that you claimed this before in rust-lang/unsafe-code-guidelines#552. I'm not sure what this is referencing.

If this is referencing the current implementation where 1-ZSTs are explicitly skipped in the ABI. I'm not seeing any documentation guaranteeing this behavior, so it should be fine to change this.

If this is referencing this: https://internals.rust-lang.org/t/creating-1-zsts-guaranteed-to-have-same-extern-c-abi-as/19399/18
Where you said:

To make repr(transparent) work, we need all 1-ZST to have the same ABI, no matter their repr.

This seems rather subtle, and it should be explicitly documented somewhere (perhaps the Rust reference on type layouts/abi).

And if this is indeed your reasoning, then we could (in future editions) ensure that #[repr(C)] 1-ZSTs are only allowed in a #[repr(transparent)] type Foo if all other types in Foo are normal non-#[repr(C)] 1-ZSTs. (i.e. treat #[repr(C)] types as if they are not 1-ZSTs for #[repr(transparent)]).

Copy link
Member

@RalfJung RalfJung Sep 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is a subtle indirect consequence, as explained at the top of rust-lang/unsafe-code-guidelines#552:

Note that the reason for this rule of any two 1-ZST being ABI compatible is that for types like #[repr(transparent)] ZST([u8; 0]; ()), we promise that the type is ABI-compatible with the field that is being transparently wrapped, and that could be either field. So the only way to always deliver on our repr(transparent) ABI-compatibility promise is to make all 1-ZST ABI-compatible.

If ABI(ZST) == ABI( () ) and ABI(ZST) == ABI([u8; 0]) it follows that ABI( () ) == ABI([u8; 0]).

then we could (in future editions) ensure that #[repr(C)] 1-ZSTs are only allowed in a #[repr(transparent)] type Foo if all other types in Foo are normal non-#[repr(C)] 1-ZSTs. (i.e. treat #[repr(C)] types as if they are not 1-ZSTs for #[repr(transparent)]).

Yes we could. Though ABI is a cross-edition concern so it'd only help marginally to do this on new editions only.

But we could try to crater whether just doing a transition for this is realistic...

Copy link
Contributor

@Jules-Bertholet Jules-Bertholet Sep 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though ABI is a cross-edition concern so it'd only help marginally to do this on new editions only.

I assume it would be based on the edition where the repr(C) 1-ZST is defined? I.e., same as everything else in this RFC. Note that, if you have:

// on new edition with new `repr(C)`
#[repr(C)]
struct NoFields {}

#[repr(transparent)]
struct Foo(u32, NoFields);

NoFields won’t be a ZST at all on windows-msvc, so it’s not really much more problematic to say Foo doesn’t compile on windows-gnu either.

Copy link
Member

@RalfJung RalfJung Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume it would be based on the edition where the repr(C) 1-ZST is defined?

Usually it'd be based on the edition of the crate that the repr(transparent) type is defined in.
Otherwise, bumping the edition of the crate that has the empty C struct could lead to downstream build failures.

NoFields won’t be a ZST at all on windows-msvc

Yeah, also see rust-lang/unsafe-code-guidelines#552 (comment).

Copy link
Contributor

@Jules-Bertholet Jules-Bertholet Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bumping the edition of the crate that has the empty C struct could lead to downstream build failures.

Well, yes, the whole point of this RFC is that changing the meaning of repr(C) is a breaking change that must be done over an edition.


Fixing the layout of this struct will also most likely let us remove a long-standing hack from our implementation of the MSVC ABI:
unlike all other ABIs, we do *not* entirely skip ZST with that ABI but instead mark them to be passed by-ptr.
This is needed to correctly pass types like `SomeFFI`.
If we fix our layout computation to match that of MSVC, we no longer need this special case in the ABI logic.

## RFC #3718

This also plays a role in [#3718](https://github.com/rust-lang/rfcs/pull/3718), where `repr(C, packed(N))` wants allow fields which are `align(M)` (while making the `repr(C, ...)` struct less packed). This is a footgun for normal uses of `repr(packed)`, so it would be better to relegate this strictly to the FFI use-case. However, since `repr(C)` plays two roles, this is difficult.
Expand Down Expand Up @@ -89,7 +84,7 @@ Field `b` would be laid out at offset 4, which is under-aligned (since `f64` has

For more details, see this discussion on [irlo](https://internals.rust-lang.org/t/repr-c-aix-struct-alignment/21594/3).

In AIX, the following struct `Floats` has the following field offsets: `[0, 8, 12]` (in bytes)
In AIX, the following struct `Floats` has the following field offsets: `[0, 8, 12]` (in bytes) and a size of 24 bytes (since the first field has a preferred alignment of 8 bytes).

```C
struct Floats {
Expand All @@ -103,10 +98,10 @@ This is because
> In aggregates, the first member of this data type is aligned according to its natural alignment value; subsequent members of the aggregate are aligned on 4-byte boundaries.
> - [IBM Documentation](https://www.ibm.com/docs/en/xl-c-and-cpp-aix/16.1?topic=data-using-alignment-modes) (Table 1, Note 1)

Even though on AIX `__alignof__(double)` is 8, it is still laid out an a 4-byte boundary.
(That's no contradiction since `__alignof__` designates the *preferred* alignment, not the *required* alignment.)
On AIX `__alignof__(double)` is 8, but field `c` is laid out at a 4-byte boundary. This is fine because `__alignof__` designates the *preferred* alignment, not the *required* alignment. Note that in Rust, we only ever use the *required* alignment and don't have a concept of a *preferred* alignment. So in Rust, we have designated the alignment of f64 to be 8 bytes.

Any fix for this would require splitting up `repr(C)`, since reducing the alignment of `f64` would reduce the size of `Floats` from `24` to `20`, which also doesn't match `C`, and we cannot special case the alignment of `Floats` to be larger since that doesn't match the algorithm currently specified for `repr(C)` (making it a breaking change).

This is in stark contrast with `repr(C)` in Rust, which always lays out fields at their "natural alignment". Any fix for this would require splitting up `repr(C)` since anyone in case 2 cannot tolerate under-aligned fields (since it would disallow taking references to those fields).
# Guide-level explanation
[guide-level-explanation]: #guide-level-explanation

Expand Down