Rust: `#[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]

https://github.com/rust-lang/rust/issues/27060 is closely related to this.

[bitshifter]

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

[camlorn]

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.

[camlorn]

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.

[camlorn]

@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]

https://github.com/rust-lang/rust/pull/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? :)

[eddyb]

@bitshifter I've been consider making Align hold one value, and use a pair of them for abi & pref, or another type to group them. For now, feel free to rename the fields to abi_pow2 etc.

[bitshifter]

OK, nearly there I think.

The RFC states

Specifying #[repr(packed)] and #[repr(packed = "N")] where N is not 1 should result in an error.

I haven't implemented that yet, but I do have a follow up question - what should happen when multiple packed conflicting packed values are specified? e.g. #[repr(packed(4))] and #[repr(packed(2))]. For repr align, this would use the max value, since repr align raises alignment. I'm not sure what the appropriate behaviour would be for packed. One option would be to error like the above example in the RFC. The other option might be to use the smallest pack value. I am not sure what the user expectation would be here.

edit: another option would be issuing a warning for conflicting pack values rather than an error.

[retep998]

If we don't make specifying packed multiple times a hard error, then the behavior should be to use the smallest value.

[bitshifter]

I went for mixing different packed values being an error for now (so #[repr(packed] and #[repr(packed(1))] is acceptable, but not packed(n>1)) . I figure loosening that up wouldn't be a breaking change.

[bitshifter]

I don't know if people get notifications for issues being referenced, so in case anyone who is interested missed it, the PR is here https://github.com/rust-lang/rust/pull/48528

[bitshifter]

#[repr(packed(n))] support has been merged to master now.

[gnzlbg]

The original RFC mentioned following drawbacks, alternatives, and unresolved questions:

• [Drawbacks]: Duplication in the language where #[repr(packed)] and #[repr(pack = "1")] have identical behavior.

IIUC #[repr(packed)] and #[repr(packed(1))] are equivalent in @bitshifter 's implementation, is this correct ?

• [alternative]: The alternative is not doing this and forcing people to continue using #[repr(packed)] with manual padding, although such structs would always have an alignment of 1 which is often wrong.
  Alternatively a new attribute could be used such as #[pack].

I think this alternative has turned out not to be very practical. In particular, it hinders automatic verification with tools like ctest, but IIUC this is exactly what rust-bindgen does. Maybe @fitzgen can comment on how nice this works in practice ?

• [alternative]: #[repr(packed)] could be extended as either #[repr(packed(N))] or #[repr(packed = "N")].

IIUC this is exactly what was implemented: #[repr(packed)] was extended to #[repr(packed(N))]

• [unresolved]: The behavior specified here should match the behavior of MSVC at least. Does it match the behavior of other C/C++ compilers as well?

I've updated mach and libc to use this on MacOSX and it at least matches the behavior of clang. I would find it very surprising if clang did not match here the behavior of GCC as well.

• [unresolved]: Should it still be safe to borrow fields whose alignment is less than or equal to the specified packing or should all field borrows be unsafe?

Can someone post an mwe showing what can go wrong here?

[retep998]

I think this alternative has turned out not to be very practical.

Of course it's not practical. That's why I wrote the RFC!

IIUC this is exactly what was implemented: #[repr(packed)] was extended to #[repr(packed(N))]

Effectively just syntax bikeshedding, but the functionality is identical otherwise.

Can someone post an mwe showing what can go wrong here?

https://github.com/rust-lang/rfcs/pull/1240

The question is basically whether all packed fields should be unsafe to borrow, or only fields where the specified packing is less than the field's natural alignment.

[bitshifter]

@gnzlbg one unanswered question you might be interested in which is talked about in the align RFC but not in the packed RFC is the interaction between packed and align.

From https://github.com/rust-lang/rfcs/blob/master/text/1358-repr-align.md:

For now, it will be illegal for any #[repr(packed)] struct to transitively contain a struct with #[repr(align)]. Specifically, both attributes cannot be applied on the same struct, and a #[repr(packed)] struct cannot transitively contain another struct with #[repr(align)]. The flip side, including a #[repr(packed)] structure inside of a #[repr(align)] one will be allowed. The behavior of MSVC and gcc differ in how these properties interact, and for now we'll just yield an error while we get experience with the two attributes.

Perhaps you will run into this situation with your macos work.

[gnzlbg]

@retep998

The question is basically whether all packed fields should be unsafe to borrow, or only fields where the specified packing is less than the field's natural alignment.

It looks like currently this produces a warning and not a hard error, why is that?

---

@bitshifter Thanks for pointing the interaction with repr(align) out. I am just trying to figure out how many unresolved issues remain, to see if it would be plausible to stabilize some minimally useful subset of this "soon".

[retep998]

It looks like currently this produces a warning and not a hard error, why is that?

Presumably to avoid breaking existing code that still relies on being able to borrow packed fields in safe Rust. It's probably discussed in the tracking issue: https://github.com/rust-lang/rust/issues/27060

[fitzgen]

[alternative]: The alternative is not doing this and forcing people to continue using #[repr(packed)] with manual padding, although such structs would always have an alignment of 1 which is often wrong.

I think this alternative has turned out not to be very practical. In particular, it hinders automatic verification with tools like ctest, but IIUC this is exactly what rust-bindgen does. Maybe @fitzgen can comment on how nice this works in practice ?

I don't see this alternative as solving the problem, since you can't align-to-two a struct that would otherwise naturally be aligned to four.

The only real alternative is to generate an opaque blob of bytes (eg [u16; 4]) and provide accessors for fields that are implemented with unaligned reads/writes. This results in a pretty terrible API.

What bindgen should do is recognize when the C/C++ is using some alignment that it cannot implement, and generate an opaque struct in that case (although it doesn't attempt to generate field accessors because it unexpectedly changes the syntax of how bindgen'd structs' fields are used).

[retep998]

Um, in certain situations, the stability check isn't working correctly.

#[repr(C)] #[repr(packed(2))] 
pub struct Testme {
    field: u32,
    field2: u8,
    field3: u64,
}

fn main() {
    use std::mem::{align_of, size_of};
    println!("A: {}, S: {}", align_of::<Testme>(), size_of::<Testme>());
}

Compiles and runs successfully, and even returns the correct alignment and padding, on STABLE RUST!

Nominating because accidental stabilization.

Thanks to @MSxDOS for notifying me about this.

[hanna-kruppe]

Oh dear, that's a general error in how repr feature gates are checked, repr(simd) has the same issue. The problem is that the feature gating code only looks at the first attribute with name repr, not all of them. #47094 was a similar error in other code.

[withoutboats]

We discussed this at lang team (last week actually!) and decided that we should make this unstable again, based on our longstanding position that accidentally stable things are not really stable.

[bitshifter]

Finally got around to fixing this.

[gnzlbg]

Which issues does repr(packed(N)) has that repr(packed) does not have?

[retep998]

None as far as I can tell. The two unresolved questions both apply to repr(packed) as well.

[gnzlbg]

So could we please please please stabilize this ?

It is as broken as repr(packed), and repr(packed) is stable. Without this feature, it is extremely painful to interface with C without invoking undefined behavior.

I mean, some crates (e.g. libc) just use repr(packed(N)) when it's available. On stable Rust, the attribute is not used, and the structs used in C FFI just end up having different size and alignment.

[cramertj]

Stabilization Report

This feature, implemented in https://github.com/rust-lang/rust/pull/48528, allows structs to specify a custom alignment with which to pack struct fields. Today's #[repr(packed)] is equivalent to #[repr(packed = 1)]. Emulating this feature today requires using #[repr(packed)] paired with manual padding fields, which is quite burdensome and makes FFI life worse. Tests demonstrating the feature can be found here.

This issue currently lists two unresolved questions:

• 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?

Both questions apply to #[repr(packed)] (no (N)) and are both backwards-compatible to fix, as they would both allow more code to compile, not less (w/ the exception of users of #![deny(warnings)], who may see an "unused unsafe" error if the first item is fixed). Stabilizing this feature now will not affect our ability to implement either of these additional behaviors.

With that in mind,

@rfcbot fcp merge

[rfcbot]

Team member @cramertj has proposed to merge this. The next step is review by the rest of the tagged teams:

• [x] @Centril
• [x] @aturon
• [x] @cramertj
• [x] @eddyb
• [x] @joshtriplett
• [ ] @nikomatsakis
• [x] @nrc
• [x] @pnkfelix
• [x] @scottmcm
• [x] @withoutboats

Concerns:

• raw-ptr-conversion resolved by https://github.com/rust-lang/rust/issues/33158#issuecomment-442274049
• tests-implementation-and-soundness resolved by https://github.com/rust-lang/rust/issues/33158#issuecomment-442274049

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[eddyb]

I assume by #[repr(packed = 1)] you meant #[repr(packed(1))]?
(the RFC might've used the older notation)

[cramertj]

@eddyb Yes, fixed. Thanks :)

[Centril]

First, I note that the implementation has diverged syntactically from what the RFC specified in two ways:

• packed instead of pack
• repr(packed(N)) instead of repr(packed = "N")

These divergences are good in my opinion because they a) seem more consistent with align(N), b) are less error prone and more IDE friendly due to not using a string literal. Nevertheless they are divergences that should be noted.

---

@rfcbot concern tests-implementation-and-soundness

According to the stabilization report, I got the impression that it would be an error in safe code to borrow a field whose specified alignment is smaller than the natural alignment. However, this is what we get on nightly:

#![feature(repr_packed)]

#[repr(packed(2))]
struct _Foo(u8, u64);

fn _bar() {
    let x = _Foo(1, 2);
    let _ = &x.1;
}

results in:

warning: borrow of packed field is unsafe and requires unsafe function or block (error E0133)
 --> src/lib.rs:8:13
  |
8 |     let _ = &x.1;
  |             ^^^^
  |
  = note: #[warn(safe_packed_borrows)] on by default
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #46043 <https://github.com/rust-lang/rust/issues/46043>
  = note: fields of packed structs might be misaligned: dereferencing a misaligned pointer or even just creating a misaligned reference is undefined behavior

This should be resolved before stabilizing and the proper tests verifying that this is now a hard error should be added.

---

@rfcbot concern raw-ptr-conversion

In RFC https://github.com/rust-lang/rfcs/pull/2582 we are currently discussing whether &packed.field as *const _ should be defined behavior or not. While packed(N) does not create a new category of problems, it does quantitatively add to the problem of people assuming that this is defined behavior while we haven't specified that it is. Pending such a decision, and given that @RalfJung noted:

This situation is our fault. We provided packed structs and no way to take a pointer to their fields that isn't insta-UB. We have to clean up the mess, and we shouldn't let our users pay for it.

it seems to me that we should first decide RFC 2582 and then stabilize packed(N) so as to not make an even bigger mess.

[cramertj]

@Centril

This should be resolved before stabilizing and the proper tests verifying that this is now a hard error should be added.

This warning matches the warning for existing repr(packed) structs, and they work via the same mechanism. repr(packed(N)) doesn't change anything about the situation here.

[cramertj]

@Centril

it seems to me that we should first decide RFC 2582 and then stabilize packed(N) so as to not make an even bigger mess.

I don't see how this makes it a bigger mess-- this is just enabling a case that is poorly supported by the existing repr(packed). It does not extend, counteract, or in any other way interact with the current set of questions around repr(packed) and unsafety.

[Centril]

After some discussion with @cramertj on Discord...

@rfcbot resolve tests-implementation-and-soundness
@rfcbot resolve raw-ptr-conversion

The compatibility lint has been in effect for roughly 1 year; there seems to still be some problems re. winapi that isn't trivial to fix but I think we can renew our efforts to try to fix the soundness hole. However, that doesn't have to tie in with the decision on stabilizing repr(packed(N)). However, we should try to speed up the resolution of the issues.

Taylor noted that it is unlikely that packed(N) will lead to a notable increase in the use of packed structs overall and that there's more likely to be a transition of packed => packed(N) instead. As this doesn't change the fundamental nature of the soundness hole or impact the resolution of &packed.field as *const _ notably, I revise my thinking: we don't have to wait -- this provides a ~strict improvement to the status quo.

[bitshifter]

This warning mentioned above is incorrect right? Reading https://github.com/rust-lang/rust/issues/33158#issuecomment-442258617 borrowing a 1 byte aligned field with packing of 2 should not generate a warning but borrowing the second field which has a natural alignment of 8 should generate a warning? Am I understanding this correctly?

[cramertj]

@bitshifter The warning above is on an access of the second field of the struct:

#![feature(repr_packed)]

#[repr(packed(2))]
struct _Foo(u8, u64);

fn _bar() {
    let x = _Foo(1, 2);
    let _ = &x.1; // x.0 is the u8 with align=1, x.1 is the u64 with align=8
}

It's correct that borrowing the u64 will generate a warning, but borrowing the u8 will not because the N in packed(N) is greater than the required alignment of u8 but less than the required alignment of u64.

[gnzlbg]

First, I note that the implementation has diverged syntactically from what the RFC specified in two ways:

packed instead of pack

[...] Nevertheless they are divergences that should be noted.

The repr(packed(N)) feature extends the stable repr(packed) feature. This divergence was already noted during the stabilization of repr(packed), and there is nothing that the repr(packed(N)) feature can do about it. Why should anything about this be noted here?

repr(packed(N)) instead of repr(packed = "N")

The syntax for this has evolved over time, and other merged RFCs using the old syntax, for example, repr(align = "N"), have been stabilized without issues using the new syntax repr(align(N)) without having to "note" anything about this.

The repr(packed(N)) feature extends the language, and it neither adds nor modifies the current syntax for this. Why should anything about this be noted here?

According to the stabilization report, I got the impression that it would be an error in safe code to borrow a field whose specified alignment is smaller than the natural alignment.

This should be resolved before stabilizing and the proper tests verifying that this is now a hard error should be added.

That might have given the wrong impression: this is an issue with the already stable repr(packed) feature . Turning the warning into an error is technically a breaking change, and performing this breaking change is an orthogonal problem to extending repr(packed) with repr(packed(N)).

it seems to me that we should first decide RFC 2582 and then stabilize packed(N) so as to not make an even bigger mess.

As the same RFC notes, a lot of unsafe code already relies on this, no good alternatives to this have been proposed yet, there is no other way of doing this in nightly today, without a way to do this a lot of code would have undefined behavior which is why current code picks up the only reasonable way to do this, and we haven't actually established that this is undefined behavior either.

OTOH, without stable repr(packed(N)), the lowest-level C FFI bindings used by the whole Rust ecosystem on all Tier-1 platforms (winapi, libc, and mach) have undefined behavior today on stable Rust.

The "mess" already exist, this does not make the "mess" worse, this fixes one of the big undefined behavior holes in Rust C FFI, there seems to be consensus that this is the proper way to fix this problem, and all new open issues can be solved in a backwards compatible way.

[RalfJung]

we haven't actually established that this is undefined behavior either.

We pretty much have, by emitting aligned attributes to LLVM.

But packed(N) does not make the problem any worse, so the only possible risk I can see here related to RFC 2582 is more people using packed structs, and it seems y'all do not expect that to happen.

[gnzlbg]

We pretty much have, by emitting aligned attributes to LLVM.

What I meant is that we don't have a merged RFC stating that &[const, mut] x requires x to always be properly aligned, even when one uses it as &[const, mut] x as *[const,mut] T to directly create a raw pointer. We only have one RFC open about this which proposes guaranteeing that &[const, mut] x as *[const,mut] T can be used to create an unaligned pointer to x.

[Centril]

@gnzlbg

Why should anything about this be noted here?

Because the RFC said one thing and we are doing another. It is good, for the sake of fairness, transparency, and accuracy, to document divergences when stabilizing a feature. This is what I think we should do with all stabilization proposals. I completely agree with the divergences in this instance.

That might have given the wrong impression: this is an issue with the already stable repr(packed) feature.

The point that @cramertj made to convince me that it is OK to move ahead with this stabilization proposal is that we don't expect a notable increase in the use of packed structs as a consequence of this stabilization. As I have resolved my concerns I am not sure why you wish to debate this. ^.^

As the same RFC notes, [...], and we haven't actually established that this is undefined behavior either.

Behavior is undefined when it is not defined (or until such time that it is defined); this is what it means by definition for behavior to be undefined.

The "mess" already exist, this does not make the "mess" worse, this fixes one of the big undefined behavior holes in Rust C FFI, there seems to be consensus that this is the proper way to fix this problem, and all new open issues can be solved in a backwards compatible way.

Which is why I'm OK with this stabilization per the reasoning @cramertj gave to convince me.

What I meant is that we don't have a merged RFC stating that &[const, mut] x requires x to always be properly aligned, even when one uses it as &[const, mut] x as *[const,mut] T to directly create a raw pointer.

We don't need to merge an RFC stating this for it to be undefined behavior. As aforementioned, it is in the very nature of defined behavior that it must be defined. That doesn't mean that we should intentionally seek to make people's life's harder, there are practical considerations of course.

[hanna-kruppe]

@gnzlbg

What I meant is that we don't have a merged RFC stating that &[const, mut] x requires x to always be properly aligned, even when one uses it as &[const, mut] x as *[const,mut] T to directly create a raw pointer. We only have one RFC open about this which proposes guaranteeing that &[const, mut] x as *[const,mut] T can be used to create an unaligned pointer to x.

Language semantics are specified inductively. The expression &x as *const _ (like any other complex expression) is thus the composition of

1. creating a reference and
2. converting that reference to a raw pointer

Assuming we agree the first step is immediate UB on its own, what happens afterwards is irrelevant, unless there is a special exception to when the first step is UB triggered by the subsequent code, but as you note such an exception is only now being discussed for inclusion in the language. I could see an argument being made that 1. is not currently conclusively settled to be immediate UB, but it's a rather different argument (because then reference creation doesn't care about alignment at all, only specific uses of references assert that it must be aligned).

Edit: Note that I'm intentionally not taking the route "if it's not explicitly defined then it's UB" @Centril took because virtually none of Rust is really defined in this strict sense, yet we still can talk about and 99.9% agree on the pre-formal intended semantics, and that sense of "Rust semantics" is the one I invoke here.

[Centril]

@rkruppe

Edit: Note that I'm intentionally not taking the route "if it's not explicitly defined then it's UB" @Centril took because virtually none of Rust is really defined in this strict sense, yet we still can talk about and 99.9% agree on the pre-formal intended semantics, and that sense of "Rust semantics" is the one I invoke here.

My problem with @gnzlbg's phrasing is that it turns things on its head; The base assumption becomes "everything that isn't explicitly undefined is defined". The inductive way of defining the language semantics I ascribe to as well. We don't need to have a formal language semantics for things to be defined behavior so I actually mostly agree with you on the notion of a pre-formal intended semantics (because that is the only recourse all of us have in the absence of a formally specified, and agreed to, semantics). There are other, more subtle, readily available, and inexact ways to define a behavior.

[gnzlbg]

The base assumption becomes "everything that isn't explicitly undefined is defined".

Ehm, it's a "little" more complicated than that: repr(packed) has been stable since Rust 1.0, is required to avoid undefined behavior in C FFI, and requires (at least syntactically) constructing references to unaligned fields to work properly.

Since Rust 1.24.0 we warn that constructing unaligned references requires unsafe (but there it is ok to do so), and since Rust 1.29 we also warn that this is undefined behavior, but have no migration path in place. Today, constructing an unaligned reference to a repr(packed) struct field is still allowed in the latest stable Rust release.

Requiring &T to be properly aligned would be a breaking change that would need to go through the RFC process, which hasn't happened yet. Without a migration path, the affected code would need to be rejected. Since the purpose of this code is ensuring correct size and alignment in repr(C) types on C FFI, such an outcome would introduce even subtler sources of undefined behavior in Rust programs.

So no, my point isn't that "everything that isn't explicitly undefined is defined" as you put it, but rather, that backwards compatibility imposes extra constraints on how undefined behavior can actually be defined, and in this case, our hands are pretty much tied.

---

It is good, for the sake of fairness, transparency, and accuracy, to document divergences when stabilizing a feature. This is what I think we should do with all stabilization proposals.

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.

[rfcbot]

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

[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:

• https://github.com/rust-lang/rust/issues/48075#issuecomment-433177954 and subsequently https://github.com/rust-lang/rust/issues/48075#issuecomment-433243048
• https://github.com/rust-lang/rust/issues/48215#issuecomment-435397697

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: https://github.com/rust-lang-nursery/reference/issues/483.