#[repr(packed(N))] (tracking issue for RFC 1399)

[nikomatsakis]

Tracking issue for rust-lang/rfcs#1399: #[repr(packed(N))]

Unresolved issues

• Should borrows to packed fields be safe if the field's natural alignment is greater than or equal to the packing value, or should all fields of a packed struct always be unsafe to borrow?
• Currently #[repr(packed(_))] structs cannot transitively contain #[repr(align(_))] structs due to differing behavior between msvc and gcc. Do we want to keep this a hard error, pick one behavior over the other, or provide some way to choose which behavior is desired?

[retep998]

#27060 is closely related to this.

[bitshifter]

This should probably use attr_literals now like repr(align) does, e.g. repr(pack(2)).

[ahicks92]

Is anyone working on this? If not I think I mostly know how to do it, modulo someone telling me how the attribute parsing and verification needs to be extended higher up.

[ahicks92]

Unless someone comes in and says they're doing this before then, I'm going to take a crack at it Friday or thereabouts. My intent is to use the attribute literals approach (or support both if that's what people want), plus potentially submitting a PR against the RFC repo to update the RFC to attribute literals.

@eddyb or whoever else might know: is inserting a bunch of [0 x u8] in packed structs going to degrade performance? The easiest approach here seems to be to multiply everything in the type index to memory index vector by 2, then insert such dummy fields in packed structs. I think that everything else after that may just work except maybe constructing constants for them, but iirc constant construction is in one place and relatively easy for me to fix in this way.

[eddyb]

@camlorn everything currently has those usually-empty arrays, for #[repr(align(N))].
I also have a work-in-progress branch where I've simplified a lot of trans (including making constant ADTs boring), but there's more to be done - maybe I should try to polish it and open a PR soon.

[ahicks92]

@eddyb
Should I wait on you?

[eddyb]

I'd say so, yes, at least for the trans part. In fact, I'm not sure there is anything left to do once I'm done, other than use the value from the attribute in the align field.

[LegNeato]

@eddyb did you land the work-in-progress changes that were being discussed here?

[eddyb]

@LegNeato They ended up in #45225, which might take a bit longer to get merged.

[fitzgen]

Is there anything I can do to help implement and stabilize this feature? We need it badly for bindgen.

[arielb1]

Is this waiting for anything more than #45225? That PR should land not too far away from now.

[LegNeato]

#45225 appears to have landed.

[bitshifter]

If no one is working on this I might have a go. I started looking into it on the weekend and got something going but don't know yet if it's correct. There are already a lot of tests for repr packed, seems like a sharp edge!

[bitshifter]

I'm nearly finished with this, I haven't run into and big problems so far. If you're interested my WIP branch is here https://github.com/bitshifter/rust/tree/repr_packed. I just have a couple more tests I want to add and then I'll make a PR.

[bitshifter]

@retep998 pointed out a mistake in my implementation which was a misunderstanding on my part of how packing works (I've never actually used pack in C/C++). My misunderstanding was I was raising the alignment of fields that were smaller than the packed size. Packing does make a lot more sense when you don't do that! It does raise a follow up question though. At the moment, packing by default disables struct field reordering optimisations, this makes sense for packed(1) but for higher values there may be benefits to reordering struct fields. Should this be enabled for packed(n > 1)?

[retep998]

Personally I would enable struct field reordering for packed(n>1) unless the user explicitly specifies repr(C) as well.

[bitshifter]

Spent a far too much time trying to work out why let mut optimize = packed && align.abi > 1 wasn't doing what I expected.

This is the relevant parts of Align:

pub struct Align { abi: u8, pref: u8 };

impl Align {
    pub fn abi(self) -> u64 { 1 << self.abi }
}

What I wanted was packed && align.abi() > 1. Related question, is it possible to make struct fields private within the module the struct is declared in? :)

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[cramertj]

@gnzlbg

Requiring &T to be properly aligned would be a breaking change

No, we require that today and always have.

[gnzlbg]

we require

Safe Rust code has knowingly accepted taking unaligned references to packed struct fields to create raw pointers since 1.0 and users rely on them for correctness. At this point, if the implementation requires them, then IMO the implementation is wrong. This is not as bad as it sounds, there is an RFC in FCP that changes this requirement of the implementation for the relevant cases (by not creating references in cases in which they were created before) and offers an easy migration path.

Any other alternative I can think of sends a really poor message.

Or in other words, you are not wrong: we "rustc" require references to be aligned and always have, but if we don't tell stable safe-Rust users what we require by rejecting unsound code, then what we require does not really matter. From a user POV, their code has worked for two years and now it does not compile.

[Centril]

@gnzlbg

So what is the process for "noting" these things? Where is it documented? This is the first time I've heard of having to do this.

See:

• tracking issue for ? macro repetition #48075 (comment) and subsequently tracking issue for ? macro repetition #48075 (comment)
• Tracking issue for RFC #2175: or-patterns in if / while let expressions  #48215 (comment)

There is no official requirement about this; but it's something I and others want when stabilization reports are made. In other words: it is internal to the language team's operation.

This is not as bad as it sounds, there is an RFC in FCP that changes this requirement of the implementation for the relevant cases (by not creating references in cases in which they were created before) and offers an easy migration path.

It's not in FCP.

Safe Rust code has knowingly accepted taking unaligned references to packed struct fields to create raw pointers since 1.0 and users rely on them for correctness. At this point, if the implementation requires them, then IMO the implementation is wrong.

[...]

Or in other words, you are not wrong: we "rustc" require references to be aligned and always have, but if we don't tell stable safe-Rust users what we require by rejecting unsound code, then what we require does not really matter. From a user POV, their code has worked for two years and now it does not compile.

Please refer to RFC 1122 for rules and recommendations around handling unsoundness and breakage.

As for "we don't tell stable safe-Rust users", we do emit a future compat warning, do we not?
In other words, the following will emit a warning:

#[repr(packed)]
struct Foo {
    a: u8,
    b: u64
}

fn bar() {
    let x = Foo { a: 1, b: 2 };
    &x.b as *const _;
}

[gnzlbg]

As for "we don't tell stable safe-Rust users", we do emit a future compat warning, do we not?

After 24 releases accepting the code, an incorrect future compat warning was emitted. 5 releases later, the incorrect future compat warning was corrected. The corrected future compatibility warning currently states "don't do this", but it crucially does not suggest what to do instead.

Please refer to RFC 1122 for rules and recommendations around handling unsoundness and breakage.

I am unsure whether RFC 1122 applies here. RFC 1122 assumes that there is a path to upgrade and/or correct affected code and all its recommendations are based on this assumption holding. AFAICT one cannot construct a pointer to an unaligned field of a repr(Rust, packed) struct without creating a reference first yet (without something like RFC 2582).

The policy about emitting warnings or errors that are breaking changes might need to be revisited, because it does not make much sense to do this without having a migration path for users in place unless the breaking changes are "impactless" like RFC 1122 mentions.

[Centril]

@gnzlbg

I am unsure whether RFC 1122 applies here. RFC 1122 assumes that there is a path to upgrade and/or correct affected code and all its recommendations are based on this assumption holding.

RFC 1122 definitely applies and it talks about and suggests recommendations around mitigation strategies (plural), transitions, and easing things. In general, there is no hard requirement that there must be a path to upgrade; it may be in some cases that a path to upgrade is simply untenable and something simply cannot be done. However, that does not apply to repr(packed).

[gnzlbg]

In general, there is no hard requirement that there must be a path to upgrade;

My point is that there should probably be a hard requirement or a very very strong encouragement to provide one. If a migration path is not provided, and enough code is affected, then repr(packed) is a perfect example of what actually ends up happening: nothing, the bug remains open for years until a migration path is in place anyways (over 3 years old bug report and merged RFC with the fix, ~1 year old fix in place waiting to be enabled).

In particular, the solution proposed in RFC1240 to fix the issue, that is, to require unsafe to take references to packed struct fields, does not actually fix the issue (it added an unsafe super power though) because materializing an unaligned reference is UB independently of whether the reference is materialized in safe or unsafe Rust code. I can't know whether this issue would have come to light during the RFC process of RFC1240 if it had included a migration path, but doing so would have definitely have shown how hard it is to avoid undefined behavior with the proposed fix.

FWIW the C++ committee does have a hard requirement on a migration path being available before making a breaking change. This is not always respected and that's ok, but it is always on the checklist for breaking changes. They do consider the feasibility of performing a breaking change on whether the migration path can be applied automatically, manually, or there not being a migration path at all.

[cramertj]

Clarifying since I think we're talking across each other: there has never been an operation in Rust that allows you to get the address of an unaligned field. This has never been a supported thing. There's an RFC in the works currently that would make this possible. It used to be that there was a way to spell something that looked like taking the address of an unaligned field, but was in fact UB and anything in the language reference, the compiler internals, or anyone on the lang team that you asked should have told you that &x as *const _ was not a valid spelling of "take the address of an unaligned field", which unfortunately is not something you can spell in Rust today. If someone told you differently, then I'm sorry that you received misinformation, and I really hope we can have greater clarity on these issues in the future. It is definitely unfortunate that we didn't have a way to do this in Rust, and that some folks who needed this feature hit UB in an attempt to spell it.

The thing that is unsafe today is taking a reference to a packed field. It is unsafe because it is expected that in order for the usage of unsafe to be correct, that the field must be properly aligned, and that you have verified correct alignment of that field manually. This still does not allow taking the address of an unaligned field. There is still no operation today that allows that.

There's an RFC out now that would permit taking the address of an unaligned field in a way that looks like how some people have attempted to spell that operation in the past, but up till now has always been undefined behavior. There's no breaking change being made here-- we're specifying that we'll allow &x as *const _ to be a valid spelling of "take the address of an unaligned field", which it has never been before.

[retep998]

Fortunately all that discussion of whether it is currently possible to take the address of an unaligned field without causing UB does not actually matter for stabilizing #[repr(packed(N))] because it applies equally to #[repr(packed)] which is already stable, and #[repr(packed(N))] does not force our hand either way with how to resolve that discussion.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

[Centril]

Any takers for a stabilization PR?

[cramertj]

I'll do that.

[Centril]

Reference documentation issue: rust-lang/reference#483.

[ic3man5]

Noting action item

Currently #[repr(packed())] structs cannot transitively contain #[repr(align())] structs due to differing behavior between msvc and gcc. Do we want to keep this a hard error, pick one behavior over the other, or provide some way to choose which behavior is desired?

seems to have been missed in review (unless I missed it reading through all the comments).

[RalfJung]

That case still errors currently. So the resolution "keep this a hard error" was picked, maybe implicitly. This should be forward-compatible with the other options.

If you have further questions, I suggest asking on Zulip. Issue necromancy doesn't usually end well. ;)

[ic3man5]

@RalfJung Would opening a new issue be the correct solution then? If we want to have complete interoperability with C, this is a needed feature since C can align and pack at the same time. The linked issue does show a need for this feature.

[RalfJung]

There are already issues for

• having packed & align on the same type: E0587 error on packed and aligned structures from C #59154
• having packed structs with aligned fields: #[repr(align(N))] fields not allowed in #[repr(packed(M>=N))] structs #100743