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

RalfJung · 2024-05-21T12:11:52Z

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 · 2024-05-21T12:12:00Z

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 · 2024-05-21T12:13:24Z

compiler/rustc_abi/src/lib.rs

 self.flags.intersects(ReprFlags::IS_UNOPTIMISABLE) || self.int.is_some()
 }

 /// Returns `true` if this type is valid for reordering and `-Z randomize-layout`
 /// was enabled for its declaration crate.
 pub fn can_randomize_type_layout(&self) -> bool {
- !self.inhibit_struct_field_reordering_opt()
- && self.flags.contains(ReprFlags::RANDOMIZE_LAYOUT)
+ !self.inhibit_struct_field_reordering() && self.flags.contains(ReprFlags::RANDOMIZE_LAYOUT)


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 · 2024-05-21T12:36:04Z

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

rustbot · 2024-05-21T12:52:11Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

RalfJung · 2024-05-21T12:52:38Z

src/tools/clippy/clippy_lints/src/transmute/transmute_undefined_repr.rs

@@ -278,7 +278,7 @@ fn reduce_ty<'tcx>(cx: &LateContext<'tcx>, mut ty: Ty<'tcx>) -> ReducedTy<'tcx>
 ty = sized_ty;
 continue;
 }
- if def.repr().inhibit_struct_field_reordering_opt() {
+ if def.repr().inhibit_struct_field_reordering() {


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 · 2024-05-21T17:22:12Z

The Miri subtree was changed

cc @rust-lang/miri

fmease · 2024-05-29T11:26:41Z

@bors r+

bors · 2024-05-29T11:26:43Z

📌 Commit 37aeb75 has been approved by fmease

It is now in the queue for this repository.

bors · 2024-05-29T11:57:17Z

⌛ Testing commit 37aeb75 with merge f2e1a3a...

bors · 2024-05-29T14:04:23Z

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

rust-timer · 2024-05-29T15:20:08Z

Finished benchmarking commit (f2e1a3a): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.9%	[0.6%, 1.3%]	13
Regressions ❌ (secondary)	0.4%	[0.4%, 0.4%]	2
Improvements ✅ (primary)	-0.7%	[-0.8%, -0.7%]	4
Improvements ✅ (secondary)	-0.4%	[-0.7%, -0.3%]	13
All ❌✅ (primary)	0.5%	[-0.8%, 1.3%]	17

Max RSS (memory usage)

Results (primary -4.5%, secondary 2.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	2.5%	[2.5%, 2.5%]	1
Improvements ✅ (primary)	-4.5%	[-4.5%, -4.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-4.5%	[-4.5%, -4.5%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 669.28s -> 667.137s (-0.32%)
Artifact size: 318.23 MiB -> 318.87 MiB (0.20%)

lqd · 2024-05-29T15:25:42Z

Is this noise 👨 🫴 🦋

(a bunch of them look like the previous PR's results bouncing back to normal. hopefully it will stabilize soon, before weekly triage)

RalfJung · 2024-05-29T15:32:59Z

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 · 2024-05-29T16:08:54Z

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.

don't inhibit random field reordering on repr(packed(1)) `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 rust-lang#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](https://doc.rust-lang.org/reference/type-layout.html): > 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.

pnkfelix · 2024-06-04T13:44:59Z

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 · 2024-06-13T22:35:15Z

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 · 2024-06-13T22:49:06Z

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 · 2024-06-13T22:56:38Z

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.

Fixes #5039 Caused by rust-lang/rust#125360. We were assuming that `packed` meant `C, packed` already. This is an assumption I've seen throughout the Rust ecosystem so there may be reasons to revert.

ChrisDenton · 2024-06-18T20:30:37Z

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

Fixes unicode-org#5039 Caused by rust-lang/rust#125360. We were assuming that `packed` meant `C, packed` already. This is an assumption I've seen throughout the Rust ecosystem so there may be reasons to revert.

cuviper · 2024-06-19T01:03:56Z

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.

Fixes unicode-org#5039 Caused by rust-lang/rust#125360. We were assuming that `packed` meant `C, packed` already. This is an assumption I've seen throughout the Rust ecosystem so there may be reasons to revert.

This update brings us to zerovec 0.10.4 which addresses a flaw that Dependabot is warning about: > The affected versions make unsafe memory accesses under the > assumption that `#[repr(packed)]` has a guaranteed field order. > > The Rust specification does not guarantee this, and > rust-lang/rust#125360 (1.80.0-beta) starts reordering > fields of `#[repr(packed)]` structs, leading to illegal memory > accesses. > > The patched versions 0.9.7 and 0.10.3 use `#[repr(C, packed)]`, > which guarantees field order.

Using #[repr(packed)] alone does not guarantee that the struct fields will stay in the specified order, and as of a change in Rust 1.80, the compiler will actually reorder such structs in practice in some cases: <rust-lang/rust#125360> Add "C" to all structs that were previously #[repr(packed)] alone, since these are all trying to match an externally-defined layout where order matters. None of these would get reordered in practice today, even with the Rust 1.80 change, but this ensures they will always stay consistent. Change-Id: I397fd0bd531a34e0f1726afb830bcd7fcc6a2f05 Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/5758933 Commit-Queue: Daniel Verkamp <[email protected]> Reviewed-by: Frederick Mayle <[email protected]>

rustbot assigned fmease May 21, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 21, 2024

RalfJung force-pushed the packed-field-reorder branch from ea7545b to 1fcce90 Compare May 21, 2024 12:12

RalfJung commented May 21, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung force-pushed the packed-field-reorder branch from 1fcce90 to a53f3ed Compare May 21, 2024 12:52

RalfJung commented May 21, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung force-pushed the packed-field-reorder branch from a53f3ed to 823f5bb Compare May 21, 2024 14:32

This comment has been minimized.

Sign in to view

RalfJung force-pushed the packed-field-reorder branch from 823f5bb to f707e61 Compare May 21, 2024 16:38

This comment has been minimized.

Sign in to view

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

37aeb75

RalfJung force-pushed the packed-field-reorder branch from f707e61 to 37aeb75 Compare May 21, 2024 17:22

fmease approved these changes May 29, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 29, 2024

the8472 added the relnotes Marks issues that should be documented in the release notes of the next release. label May 29, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label May 29, 2024

bors merged commit f2e1a3a into rust-lang:master May 29, 2024
7 checks passed

rustbot added this to the 1.80.0 milestone May 29, 2024

rustbot added the perf-regression Performance regression. label May 29, 2024

RalfJung deleted the packed-field-reorder branch May 29, 2024 15:33

rustbot added the perf-regression-triaged The performance regression has been triaged. label Jun 4, 2024

Manishearth mentioned this pull request Jun 13, 2024

Enforce C,packed, not just packed, on ULE types unicode-org/icu4x#5049

Merged

wavefnx mentioned this pull request Jul 9, 2024

Update to 0.1.1 wavefnx/roller#3

Merged

waywardmonkeys mentioned this pull request Jul 25, 2024

Update icu_locid, icu_properties linebender/parley#91

Merged

kp-mariappan-ramasamy mentioned this pull request Jul 26, 2024

[auto] Update Rust toolchain to 1.80.0 expressvpn/wolfssl-rs#165

Merged

nbdd0121 mentioned this pull request Aug 8, 2024

[rust] use explicit repr(C, packed) instead of just repr(packed) lowRISC/opentitan#24256

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

RalfJung commented May 21, 2024 •

edited

Loading

rustbot commented May 21, 2024

RalfJung May 21, 2024 •

edited

Loading

the8472 commented May 21, 2024

This comment has been minimized.

rustbot commented May 21, 2024

RalfJung May 21, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rustbot commented May 21, 2024

fmease commented May 29, 2024

bors commented May 29, 2024

bors commented May 29, 2024

bors commented May 29, 2024

rust-timer commented May 29, 2024

lqd commented May 29, 2024 •

edited

Loading

RalfJung commented May 29, 2024 •

edited

Loading

the8472 commented May 29, 2024 •

edited

Loading

pnkfelix commented Jun 4, 2024

Manishearth commented Jun 13, 2024 •

edited

Loading

the8472 commented Jun 13, 2024

Manishearth commented Jun 13, 2024

ChrisDenton commented Jun 18, 2024

cuviper commented Jun 19, 2024

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

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

Conversation

RalfJung commented May 21, 2024 • edited Loading

rustbot commented May 21, 2024

RalfJung May 21, 2024 • edited Loading

Choose a reason for hiding this comment

the8472 commented May 21, 2024

This comment has been minimized.

rustbot commented May 21, 2024

RalfJung May 21, 2024

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

rustbot commented May 21, 2024

fmease commented May 29, 2024

bors commented May 29, 2024

bors commented May 29, 2024

bors commented May 29, 2024

rust-timer commented May 29, 2024

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Binary size

lqd commented May 29, 2024 • edited Loading

RalfJung commented May 29, 2024 • edited Loading

the8472 commented May 29, 2024 • edited Loading

pnkfelix commented Jun 4, 2024

Manishearth commented Jun 13, 2024 • edited Loading

the8472 commented Jun 13, 2024

Manishearth commented Jun 13, 2024

ChrisDenton commented Jun 18, 2024

cuviper commented Jun 19, 2024

RalfJung commented May 21, 2024 •

edited

Loading

RalfJung May 21, 2024 •

edited

Loading

lqd commented May 29, 2024 •

edited

Loading

RalfJung commented May 29, 2024 •

edited

Loading

the8472 commented May 29, 2024 •

edited

Loading

Manishearth commented Jun 13, 2024 •

edited

Loading