Show packed field alignment in mir_transform_unaligned_packed_ref #147743

21aslade · 2025-10-15T20:53:06Z

Fixes #147528

rustbot · 2025-10-15T20:53:09Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

rustbot · 2025-10-15T20:53:12Z

r? @madsmtm

rustbot has assigned @madsmtm.
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

compiler/rustc_mir_transform/src/errors.rs

madsmtm

Thanks, and welcome to working on the compiler! Have a few comments, see below. Remember to squash once you're done.

Also, I see that you originally implemented the full message as discussed by the referenced issue, but then decided to do something simpler - could you edit your top-level comment to discuss why this wasn't done in the end? (see the review policy for why to put stuff in the PR message).

View changes since this review

madsmtm · 2025-10-16T03:50:41Z

compiler/rustc_mir_transform/messages.ftl


 mir_transform_unaligned_packed_ref = reference to packed field is unaligned
-    .note = packed structs are only aligned by one byte, and many modern architectures penalize unaligned field accesses
+    .note = this field is only {$actual}-byte aligned, but its type requires more alignment


Suggested change

.note = this field is only {$actual}-byte aligned, but its type requires more alignment

.note = this field is only {$actual}-byte aligned, but its type requires higher alignment

madsmtm · 2025-10-16T04:00:06Z

compiler/rustc_mir_transform/messages.ftl

    }

 mir_transform_unaligned_packed_ref = reference to packed field is unaligned
-    .note = packed structs are only aligned by one byte, and many modern architectures penalize unaligned field accesses


What's the rationale for dropping the "many modern architectures penalize unaligned field accesses" part?

Maybe keep it as another note?

I'm actually happy to see that disappear -- no idea when or why it was added, but I don't see why it is relevant at all here. The most likely effect it has is to reinforce the common misconception that the alignment requirements in Rust arise because of hardware alignment requirements.

Seems this was added in #110973. This was supposedly meant to resolve #110199 but #110199 was about

point out that #[packed] makes the struct itself have lower alignment

so still not sure why the part about penalizing unaligned accesses would be relevant.

However, the new message arguably regresses talking about the struct type. So maybe this should say

reference to field of packed struct is unaligned

(since arguably it's not the field that's packed, it's the struct)

the struct is at most {$actual}-byte aligned, but the type of this field may require higher alignment

I initially removed "this struct" because it's ambiguous - in the expression &a.b.c, it could be either a or b that causes the unalignment. (It's also technically incorrect, because unions can also be packed.)

I'm happy with this final message in spite of that though, so I'll go ahead and implement it.

Deep inside is_within_packed we know the actual ADT this is talking about; this could be exposed to call adt.descr() instead of just saying "struct".

madsmtm · 2025-10-16T04:20:04Z

compiler/rustc_const_eval/src/util/alignment.rs

        _ => {
            // We cannot figure out the layout. Conservatively assume that this is disaligned.
            debug!("is_disaligned({:?}) - true", place);
-            true


Nit: Fix the debug! message above

madsmtm · 2025-10-16T04:20:54Z

compiler/rustc_const_eval/src/util/alignment.rs

-/// struct).
+/// Returns the packed alignment if this place is allowed to be less aligned
+/// than its type normally requires (because it is within a packed struct).
 pub fn is_disaligned<'tcx, L>(


Nit: The is_ part is kinda wrong now, maybe the function should be renamed?

Maybe something like misalignment? (would also fix my problem with the use of "disaligned" here which sounds weird, the proper term is "misaligned").

"misaligned" is the term for "alignment violated". But this function can't know if alignment is truly violated, it only checks if alignment may be violated.

So maybe "unaligned" is a better term.

RalfJung · 2025-10-16T06:30:44Z

tests/ui/binding/issue-53114-safety-checks.stderr

   |             ^^^^
   |
-   = note: packed structs are only aligned by one byte, and many modern architectures penalize unaligned field accesses
+   = note: this field is only 1-byte aligned, but its type requires more alignment


So if I have a packed(8) struct with a field whose type cannot be determined, the error will say

this field is only 8-byte aligned, but its type requires more alignment

That's not correct though -- the type may require higher alignment, but we don't actually know that for sure.

Show packed field alignment in mir_transform_unaligned_packed_ref

ca65bc9

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 Oct 15, 2025

rustbot assigned madsmtm Oct 15, 2025

21aslade commented Oct 15, 2025

View reviewed changes

compiler/rustc_mir_transform/src/errors.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Don't print alignment in error message

e314e04

madsmtm added A-diagnostics Area: Messages for errors, warnings, and lints A-repr-packed Area: the naughtiest repr labels Oct 16, 2025

madsmtm requested changes Oct 16, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 16, 2025

RalfJung reviewed Oct 16, 2025

View reviewed changes

	.note = this field is only {$actual}-byte aligned, but its type requires more alignment
	.note = this field is only {$actual}-byte aligned, but its type requires higher alignment

Show packed field alignment in mir_transform_unaligned_packed_ref #147743

Are you sure you want to change the base?

Show packed field alignment in mir_transform_unaligned_packed_ref #147743

Uh oh!

Conversation

21aslade commented Oct 15, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Oct 15, 2025

Uh oh!

rustbot commented Oct 15, 2025

Uh oh!

Uh oh!

This comment has been minimized.

madsmtm left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

21aslade commented Oct 15, 2025 •

edited by rustbot

Loading

madsmtm left a comment •

edited by rustbot

Loading

RalfJung Oct 16, 2025 •

edited

Loading

RalfJung Oct 16, 2025 •

edited

Loading