don't inhibit random field reordering on repr(packed(1))

[RalfJung]

inhibit_struct_field_reordering_opt being false means we exclude this type from random field shuffling. However, packed(1) types can still be shuffled! The logic was added in #48528 since it's pointless to reorder fields in packed(1) types (there's no padding that could be saved) -- but that shouldn't inhibit -Zrandomize-layout (which did not exist at the time).

We could add an optimization elsewhere to not bother sorting the fields for repr(packed) types, but I don't think that's worth the effort.

This does change the behavior in that we may now reorder fields of packed(1) structs (e.g. if there are niches, we'll try to move them to the start/end, according to NicheBias). We were always allowed to do that but so far we didn't. Quoting the reference:

On their own, align and packed do not provide guarantees about the order of fields in the layout of a struct or the layout of an enum variant, although they may be combined with representations (such as C) which do provide such guarantees.

[rustbot]

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[RalfJung]

This is still a bit silly, the only place where can_randomize_type_layout is called is effectively inside an if !self.inhibit_struct_field_reordering()... but I didn't want to do a larger refactoring here, not sure what even would be the better structure.

[the8472]

Note that this does not only affect Zrandomize-layout builds. Some structs will also be reordered in regular builds.

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[RalfJung]

FWIW clippy already used to assume inhibit_struct_field_reordering_opt means that the field order is guaranteed, which was not the case before this PR.

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[fmease]

@bors r+

[bors]

📌 Commit 37aeb75 has been approved by fmease

It is now in the queue for this repository.

[bors]

⌛ Testing commit 37aeb75 with merge f2e1a3a...

[bors]

☀️ Test successful - checks-actions
Approved by: fmease
Pushing f2e1a3a to master...

[RalfJung]

We seem to have a lot of noise recently (see e.g. these results). But still, the most serious "primary" regressions seem a bit too big to be just noise. Do these crates have to many packed(1) structs that sorting their fields make such a difference...?

[the8472]

bitmaps is regressing on check, this could be rustc itself being slower due to layout changes.

but cycles are unchanged, so it might be extra leas needed for offset computations or more niche computation happening in ways that don't affect cycles.

[pnkfelix]

• regressed bitmaps and typenum; improved helloworld
• instruction counts were affected but not cycle counts; one theory is that object code has extra offset computations or niche computations...
• since cycle count was not affected, does not seem worth further investigation; marking as triaged

@rustbot label: +perf-regression-triaged

[Manishearth]

This broke ICU4X. It's our fault: we assumed repr(packed) meant repr(C, packed), but this is an assumption I have seen quite commonly in the Rust ecosystem around networking and FFI code, since there has never really been a concept of "Rust packed" distinct from "C packed" till now, and 99% of code using packed will either be using it for networking (where they need C, packed, or for zero-copy stuff (where they need stability, which you get from C, packed)

So while I wouldn't go as far as to say it's a breaking change, I wonder if this should be done more carefully, perhaps requiring packed to be paired with Rust or C.

Or even having repr(packed) default to C, packed unless explicitly specified? Rust, packed does seem to be somewhat niche to me until repr(Rust) stabilizes.

(The fix for ICU4X is unicode-org/icu4x#5049)

[the8472]

Some crates getting broken is expected, it also happened the last time we changed field ordering (#102750) which is why i added the relnotes label.

Also note that packed(2) and up already did reorder fields.

If the beta crater run shows a bigger impact then a more careful approach would make sense.

[Manishearth]

Some crates getting broken is expected, it also happened the last time we changed field ordering (#102750) which is why i added the relnotes label.

Right, but I think in this case, while the nomicon and reference seem to be clear that repr(packed) is repr(Rust, packed) by default, my experience with the ecosystem is that most people assume repr(packed) to mean repr(C, packed), because repr(Rust, packed) doesn't really make sense for 99% of the use cases of packed.

So while the docs are clear, I wonder if the ecosystem's impression of how things work is going to be a problem for this change. That's not as much the case for packed(2).

I'm also not sure if this will show up much on crater since it may require specific kinds of test failures.

[ChrisDenton]

If this is a common source of confusion, maybe there should be a lint that encourages people to use an explicit representation with packed?

[cuviper]

maybe there should be a lint that encourages people to use an explicit representation with packed?

The missing-abi lint is similar in spirit, "No declared ABI for extern declaration." That one is allowed by default, but in this case the implicit "Rust" ABI may be more surprising, justifying a warning by default.