[RFC] Allow packed types to transitively contain aligned types #3718

Neo-Zhixing · 2024-10-24T21:24:46Z

This issue was introduced in the original implementation of #[repr(packed(N))] and have since underwent extensive community discussions:

The solution proposed in this RFC mostly reflects what was proposed here

text/0000-layout-packed-aligned.md

CAD97

It should be mentioned in the drawbacks that this still doesn't make repr(C) fully match the target (MSVC) toolchain in all cases; the known other divergences are enums with overflowing discriminant and how a field of type [T; 0] is handled. So while this does improve parity, the reality is that there are still edge cases to keep track of for now, and the benefit of changing repr(C) for MSVC isn't properly realized until all known cases are addressed.

If I understand properly, the new layout algorithm is effectively:

// gnu
fn push_field(prefix: Layout, mut field: Layout) -> Result<(Layout, usize)> {
    let mut align = field.align();
    if let Some(pack) = prefix.packed() {
        align = align.min(pack);
    }
    field = Layout::from_size_align(field.size(), align)?;
    prefix.extend(field)
}

// msvc
fn push_field(prefix: Layout, mut field: Layout) -> Result<(Layout, usize)> {
    let mut align = field.align();
    if let Some(pack) = prefix.packed() {
        align = align.min(pack);
    }
    if let Some(force) = field.msvc_align() {
        assert!(force <= field.align());
        align = align.max(force);
    }
    field = Layout::from_size_align(field.size(), align)?;
    prefix.extend(field)
}

text/0000-layout-packed-aligned.md

CAD97 · 2024-10-25T20:39:52Z

text/0000-layout-packed-aligned.md

+increased to be N.
+
+When a `#[repr(packed(M))]` struct transitively contains a field with `#[repr(align(N))]` type,
+- The field is first `pad_to_align`. Then, the field is added to the struct with alignment decreased to M. The packing requirement overrides the alignment requirement. (GCC, `#[repr(Rust)]`, `#[repr(C)]` on gnu targets, `#[repr(system)]` on non-windows targets)


This pad_to_align seems incorrect. If it's Layout::new::<FieldTy>().pad_to_align(), that's unnecessary; type size is already a multiple of alignment by construction. The padding in the struct is handled by adding the field as with an alignment decreased to M. packed does not ever strip trailing padding, but that's just due to said padding being part of the type, not coming from some different layout stride property.

However, Clang and GCC has the following layout:

struct __attribute__((aligned(4))) MyStructInner { // Alignment = 4, Size = 4 uint8_t a; // Offset = 0 }; struct __attribute__((packed)) MyStruct { // Alignment = 1, Size = 8 MyStructInner a; // Offset = 0 uint32_t b; // Offset = 4 };

It is indeed curious that b would have offset = 4 in MyStruct, even though it's not technically necessary. This behavior was covered by the language I used in the RFC.

No, there's no curiosity here? MyStructInner has a size of 4, so the next field to follow it must be offset by at least 4 bytes from the field of type MyStructInner to avoid overlapping it. It's not allowed to put fields within the trailing padding of other fields in C, C++, nor Rust. (Unless using [[no_unique_address]] in C++.)

I think I might understand where the disconnect is coming from. MyStruct does not have the field of type uint8_t pushed into its layout; it has a field of type MyStructInner pushed into its layout, and that MyStructInner has the full size of 4 bytes.

No additional alignment needs to be applied to push the field, because the MyStructInner type is already sufficiently sized and aligned for the MyStructInner value.

I don't even know what "the field is first pad_to_align" even means. First of all I can't quite parse it from a grammatical PoV, and secondly the output of a layout algorithm is the offset of each field of the type (that is, the immediate fields listed in the type declaration, not the recursive ones). So it's completely unclear what this business with recursive fields here is supposed to mean. It sounds like it is talking about the offset of a field of an inner type in the outer type (like, the offset of a field of MyStructInner in MyStruct), but that makes no sense.

Ok I see where the disconnect came from as well. I was under the impression that MyStructInner would have a size of 1 byte and an alignment of 4 bytes. If MyStructInner already have a size of 4 bytes, pad_to_align is indeed not necessary, although it wouldn't be incorrect.

text/0000-layout-packed-aligned.md

CAD97 · 2024-10-25T20:50:47Z

text/0000-layout-packed-aligned.md

+
+## `#[repr(C)]`
+When `align` and `packed` attributes exist on the same type, or when `packed` structs transitively contains `align` types,
+the resulting layout matches the target toolchain ABI.


To verify, Clang/LLVM does replicate the MSVC behavior correctly for its MSVC target, correct? It's a harder sell to change repr(C) if extended types already weren't portable between the C++ compiler toolchains for the target.

I do believe so.

https://clang.llvm.org/docs/MSVCCompatibility.html

First, Clang attempts to be ABI-compatible, meaning that Clang-compiled code should be able to link against MSVC-compiled code successfully.

Record layout: Complete. We’ve tested this with a fuzzer and have fixed all known bugs.

CAD97 · 2024-10-25T21:59:06Z

text/0000-layout-packed-aligned.md

+
+When a `#[repr(packed(M))]` struct transitively contains a field with `#[repr(align(N))]` type,
+- The field is first `pad_to_align`. Then, the field is added to the struct with alignment decreased to M. The packing requirement overrides the alignment requirement. (GCC, `#[repr(Rust)]`, `#[repr(C)]` on gnu targets, `#[repr(system)]` on non-windows targets)
+- The field is added to the struct with alignment increased to N. The alignment requirement overrides the packing requirement. (MSVC, `#[repr(C)]` on msvc targets, `#[repr(system)]` on windows targets)


The RFC should be explicit about the fact that this introduces a new kind of alignment to Rust type layout. There's the existing align_of/Layout::align, and then there's explicitly added alignment, which can be different.

Is this new alignment flavor exposed to code in any way? If so, is it included in alloc::Layout? Given Layout is part of the very performance critical allocation API, it probably shouldn't be, but then Layout isn't sufficient to compute the layout of a compound repr(C) type anymore.

We're not introducing a new type of alignment. Conceptually:

packed(N) changes the alignment of the inner fields to be N.

aligned(M) changes the alignment of the entire structure to be M.

When both rules apply to the same type, (that's when a packed type transitively contains an aligned field), we have ambiguity, and we need some rules to determine who wins.

So really we're just changing the value returned by offset_of. align_of will continue to return the base alignment of a type like before.

But it is a different kind of alignment. i64 has an alignment of 8, which can be lowered by packed. #[repr(align(8))] i64 has an alignment of 8, which cannot be lowered by packed. #[repr(align(4))] i64 has an alignment of 8, which can be lowered to 4 by packed. What is this if not a new kind of alignment? Attributes apply to the type they decorate, not to all types which transitively contain the decorated type.

To reiterate this point: you end up with "primitive alignment" which can be suppressed by packed, originating from primitives; as well as "user alignment" which cannot be overridden by packed, originating from source attributes.

Types are not constructed as a list of primitive fields flattened from the relevant types, even as much as the MSVC layout algorithm sometimes pretends that this is the case. Types are instead defined compositionally, and a field of a struct or enum type is no different from that of some primitive type.

And here "user alignment" would be what is ultimately returned by align_of, i.e. the alignment required for references to this type?

This reminds me a lot of the discussions around u64 on 32bit MSVC, which requires separating "alignment of references to this type" and "alignment of struct fields to this type"... is the new kind of alignment proposed in this RFC the same as as that, or does MSVC actually have 3 kinds of alignment?

I think any new kind of alignment should be exposed to user code specifically because user code may want to have its own layout algorithm (e.g. for JIT). Maybe add it as a new field to Layout and add methods for msvc struct layout?

Adding fields to core::alloc::Layout will be a difficult sell, because Layout is part of the global dynamic allocation ABI, and adding more fields to allocation queries which are not needed for the actual allocation wouldn't be very zero cost abstraction of us.

Layout should be able to stay the same size since it can be:

pub struct OldLayout { size: usize, align: usize, // really a wrapper of a repr(usize) enum } pub struct NewLayout { size: usize, log2_align: u8, #[cfg(windows)] // or any other platforms that have msvc's weirdness log2_manual_align: u8, }

Adding fields to core::alloc::Layout will be a difficult sell,

ok, maybe make a #[cfg(windows)] MsLayout struct then?

actually, now that I'm thinking about it, you'd probably want repr(MS) to be available on non-windows too since you'd need it in writing software like Wine where you have to interface with both the host system and with Windows programs running inside your Win32 API implementation

petrochenkov · 2024-10-26T10:23:49Z

Do C/C++ toolchains for mingw address the #[repr(system)] issue in any way?
What do they do when they need to interoperate with msvc-style layouts?

Neo-Zhixing · 2024-10-29T16:12:23Z

@petrochenkov

My understanding is that MinGW doesn't really do anything, and attempting to call MSVC APIs from MinGW will result in ABI issues.

scottmcm · 2024-10-31T00:32:42Z

text/0000-layout-packed-aligned.md

+```
+`align_of::<Bar>()` would be 4 for `*-pc-windows-msvc` and `*-pc-windows-gnu`. It would be 1 for everything else.
+
+## `#[repr(Rust)]`


When would someone want this on repr(Rust)? Can we just make it disallowed for repr(Rust)?

(repr(packed) is generally bad, because it affects safe code too, so I'd rather ban it as much as is practical.)

We do allow standalone packed and aligned attributes for repr(Rust), so I do believe that it is a good idea to enable nested packed and aligned attributes as well so that the language design seems more coherent. So I'm inclined to allow it unless we have a good reason not to.

scottmcm · 2024-10-31T00:37:19Z

One thing that seems like an alternative would be @Jules-Bertholet's https://internals.rust-lang.org/t/pre-rfc-align-attribute/21004?u=scottmcm proposal for #[align] on fields.

Would that be enough to do this?

scottmcm · 2024-10-31T00:40:39Z

text/0000-layout-packed-aligned.md

+- Both packed and aligned.
+- Packed, and transitively contains`#[repr(align)]` types.
+
+It also introduces `#[repr(system)]` which is designed for interoperability with operating system APIs.


I wonder if it's be worth splitting this out.

I'm a big fan of splitting repr(linear) vs repr(C) (which I think this is spelling as repr(C) vs repr(system)) to have the distinction between "the layout you get with https://doc.rust-lang.org/beta/std/alloc/struct.Layout.html#method.extend" and "whatever weird layout your C compiler uses". That distinction would be really nice for making intent clearer, since today you get "you can't do that in C" warnings sometimes just because you used repr(C) to have a predictable layout for your Rust-only code.

So I'd kinda like to consider that separately from any new packed-related stuff.

No, the only difference between #[repr(C)] and #[repr(system)] by this RFC is that on pc-windows-gnu #[repr(system)] is the MSVC layout while #[repr(C)] is the GCC layout. On all other targets #[repr(system)] and #[repr(C)] are identical (per this RFC).

I think repr(linear) is independently useful and we should have all 3. that said, I am concerned that current repr(C) code often means "I want stable linear layout" rather than "I want whatever weirdness the C compiler decides to use", so I think deprecating repr(C) and replacing it with repr(linear), repr(bikeshed_other_C) and repr(system) is worth considering.

Indeed, this RFC proposes a different distinction between repr(C) and repr(system) than what has been previously discussed in other threads.

Since the distinction between the two layouts we have here is Windows-only, I wonder if it should be some Windows-only name, like repr(msvc) or so? Is there a good reason to even make both of them available on all targets -- effectively exporting a Windows-only complication to other, saner platforms?

Is there a good reason to even make both of them available on all targets

The reason is that this is also how the extern "system" function ABI string works. Code uses extern "system" to link to libraries which use __stdcall on Windows platforms, and in the same way code would use #[repr(system)] for linking to a dylib which provides builds with only the MSVC toolchain for Windows targets.

I don't necessarily endorse this option, but it is logically consistent with how Rust already uses "system" as an ABI string.

It was previously forbidden but the check has some gaps, so this can impact existing code.

It also breaks existing code that assumes that all repr(C) types are laid out according to the rules described here.

The reason is that this is also how the extern "system" function ABI string works. Code uses extern "system" to link to libraries which use __stdcall on Windows platforms, and in the same way code would use #[repr(system)] for linking to a dylib which provides builds with only the MSVC toolchain for Windows targets.

From what I understand, extern "system" is the same on MSVC and GNU Windows targets? So this is IMO a false analogy then, making it more confusing than if we instead use a name that more explicitly represents that Windows has two ABIs, which we support with two target triples, and you might want to write code that talks with the "other" ABI.

Speaking of which, how would a program for the MSVC target lay out its type in the right way to call a GNU ABI library? That does not seem possible with this proposal.

Yes, extern "system" is extern "stdcall" on all Windows targets. Just like how this RFC makes repr(system) on both windows-msvc and windows-gnu behave as repr(MS). The difference is repr(C), which switches between msvc/gnu respectively.

Oh okay, I mixed up which is C and which is system then.

Just like how this RFC makes repr(C) on both windows-msvc and windows-gnu behave as repr(MS).

The first repr in that sentence should be system, not C, right?

Oops, yes, that's correct.

RalfJung · 2024-10-31T17:57:55Z

text/0000-layout-packed-aligned.md

+currently no easy way to create a matching Rust type:
+
+```cpp
+struct  __attribute__((packed, aligned(4))) MyStruct {


It is somewhat confusing that the Rust example uses packed(2) but the C example just uses packed. Would be better to make them equivalent.

RalfJung · 2024-10-31T18:01:13Z

text/0000-layout-packed-aligned.md

+increased to be N.
+
+When a `#[repr(packed(M))]` struct transitively contains a field with `#[repr(align(N))]` type, depending on the
+target triplet, either:


This should IMO be rephrased as an algorithm that works one "layer" at a time. Computing the layout of a type T should only consider the fields of T and their properties. It should never recurse into the fields of T.

I don't currently actually understand the proposed spec here, and this should make it a lot easier to understand.

I suspect what will happen is that as part of this, we will have to introduce a new property of T that is "bubbled up" in the recursion -- a new degree of freedom that was not required so far. (@CAD97 has already alluded to this elsewhere.) Identifying and clearly describing this property will make it a lot easier to understand the resulting layout algorithm.

My current understanding of the algorithm is as follows:

We equip types with a new property, the "explicitly requested alignment". For all base types, this alignment is 1. For structs, it is by default the maximum of the explicitly requested alignments of all fields. For a struct with the #[repr(align(N))] attribute, the explicitly requested alignment the maximum of N and the explicitly requested alignments of its fields.

Note that the explicitly requested alignment of a type can never be bigger than the required alignment of the type.

When computing the layout of a packed(P) struct, then currently we ensure each field is aligned to min(A, P), where A is the (regular) alignment of the field type. Under the new rules, only on MSVC targets, we instead ensure the field is aligned to max(E, min(A, P)), where E is the explicitly requested alignment. (Due to the aforementioned inequality, this is equivalent to min(max(E, P), A). Also, in particular the packed struct has at least this alignment itself.) This is the only time the explicitly requested alignment of a type has any effect.

I am not fully confident that this is correct. Here's a corner case:

#[repr(C, align(4))] struct Align4(i32); #[repr(C, align(2))] struct Align2(Align4); #[repr(C, packed)] struct Packed(Align2);

What is the resulting alignment of Packed on MSVC? My proposed algorithm says 4. Is that correct?

Another corner case:

#[repr(C, align(4))] struct Align4(i32); struct Group(u8, Align4); #[repr(C, packed)] struct Packed(u16, Group);

What does the resulting layout of Packed look like? My algorithm says:

field 0 (type u16): offset 0

field 1 (type Group): offset 4

nested field 0 (type u8): offset 4 (relative to the beginning of Packed)

nested field 1 (type Align4): offset 8

Is that correct?

(The post this referred to has since been deleted.)

Oh, so having an align attribute on a struct where a field already has a higher explicitly requested alignment is an error? Should this also be an error in Rust? The RFC doesn't say so, and it would be a breaking change... but that should at least be mentioned in the RFC. It might be worth a warning if someone writes a repr(C) type in Rust that couldn't be written in C.

https://c.godbolt.org/z/es1cGPhz9

On MSVC 19.40 (VS 17.10) in C mode,

#include <stdalign.h> #include <stddef.h> #include <stdint.h> #include <stdio.h> __declspec(align(4)) struct Align4 { int32_t _0; }; __declspec(align(2)) struct Align2 { struct Align4 _0; }; #pragma pack(push, 1) struct Packed { struct Align2 _0; }; #pragma pack(pop) struct Group { uint8_t _0; struct Align4 _1; }; #pragma pack(push, 1) struct P { uint16_t _0; struct Group _1; }; #pragma pack(pop) int main() { printf("alignof(Packed) = %zu\n", alignof(struct Packed)); printf("\n"); printf("offsetof(P, _0) = %zu\n", offsetof(struct P, _0)); printf("offsetof(P, _1) = %zu\n", offsetof(struct P, _1)); printf("offsetof(P, _1._0) = %zu\n", offsetof(struct P, _1) + offsetof(struct Group, _0)); printf("offsetof(P, _1._1) = %zu\n", offsetof(struct P, _1) + offsetof(struct Group, _1)); }

gives

alignof(Packed) = 4 offsetof(P, _0) = 0 offsetof(P, _1) = 4 offsetof(P, _1._0) = 4 offsetof(P, _1._1) = 8

Using alignas/_Alignas on the type (alignas(N) struct Tag) instead of __declspec(align) gives

warning C5274: behavior change: _Alignas no longer applies to the type 'Align4' (only applies to declared data objects)

and results of 1 / 0, 2, 2, 6; fully just ignoring the alignas modifier. Note that standard C does not permit the use of alignas for struct definitions. C++ does. In C++ mode (/std:c++latest, using struct alignas(N) Tag), MSVC gives:

alignof(Packed) = 4 offsetof(P, _0) = 0 offsetof(P, _1) = 4 offsetof(P, _1._0) = 4 offsetof(P, _1._1) = 8

along with a warning:

warning C4359: 'Align2': Alignment specifier is less than actual alignment (4), and will be ignored.

EDIT TO ADD: interesting: in C mode, __declspec(align(2)) struct Align2 gives no warning, but struct __declspec(align(2)) Align2 gives the same warning as in C++ mode. Odd. The standard C compliant way of writing the alignment (alignas on the first data member) also does not warn, in C nor C++ mode.

For completeness, clang does not seem to implement this fully, unless I made a mistake: [godbolt]

#include <stdalign.h> #include <stddef.h> #include <stdint.h> #include <stdio.h> struct [[gnu::ms_struct]] Align4 { alignas(4) int32_t _0; }; struct [[gnu::ms_struct]] Align2 { // alignas(2) // error: requested alignment is less than minimum alignment of 4 for type 'struct Align4' struct Align4 _0; }; struct [[gnu::packed]] Packed { struct Align2 _0; }; struct Group { uint8_t _0; struct Align4 _1; }; struct [[gnu::packed]] P { uint16_t _0; struct Group _1; }; int main() { printf("alignof(Packed) = %zu\n", alignof(struct Packed)); printf("\n"); printf("offsetof(P, _0) = %zu\n", offsetof(struct P, _0)); printf("offsetof(P, _1) = %zu\n", offsetof(struct P, _1)); printf("offsetof(P, _1._0) = %zu\n", offsetof(struct P, _1) + offsetof(struct Group, _0)); printf("offsetof(P, _1._1) = %zu\n", offsetof(struct P, _1) + offsetof(struct Group, _1)); }

alignof(Packed) = 1 offsetof(P, _0) = 0 offsetof(P, _1) = 2 offsetof(P, _1._0) = 2 offsetof(P, _1._1) = 6

RalfJung · 2024-10-31T18:02:11Z

text/0000-layout-packed-aligned.md

+
+When a `#[repr(packed(M))]` struct transitively contains a field with `#[repr(align(N))]` type, depending on the
+target triplet, either:
+- The field is first `pad_to_align`. Then, the field is added to the struct with alignment decreased to M. The packing requirement overrides the alignment requirement. (GCC, `#[repr(Rust)]`, `#[repr(C)]` on gnu targets, `#[repr(system)]` on non-windows targets), or


It seems like the parenthetical here is meant to say when which case applies? That's quite confusing, please state the condition before the consequence -- just like in code, if condition { ... } else { ... }.

Fixed. Thanks!

RalfJung · 2024-10-31T18:06:47Z

text/0000-layout-packed-aligned.md

+
+Clang matches the Windows ABI for `x86_64-pc-windows-msvc` and matches the GCC ABI for `x86_64-pc-windows-gnu`.
+
+MinGW always uses the GCC ABI.


Is there prior art for a compiler that can lay out types both using the Windows ABI and the GCC ABI for code within a single target? If yes, how are they distinguishing the two? If no, why does Rust need this ability?

gcc apparently supports that by using the ms_struct attribute

So that would correspond tom in Rust

have repr(C) on win-gnu targets match non-win targets

have a separate window-only repr(MS) to ask for the msvc layout

For a more full parallel to extern "ABI", we should also support repr(GCC). Then repr(C) is a sort of alias to repr(GCC) or repr(MS) chosen by the target, like extern "C" is an alias (strongly newtyped) to "sysv64"/"win64" (etc).

clang appears to also implement [[gnu::ms_struct]] has an open PR implementing [[gnu::gcc_struct]]

joshlf · 2024-10-31T19:38:01Z

text/0000-layout-packed-aligned.md

+#[repr(C, packed(1))]
+struct Bar(Foo);
+```
+`align_of::<Bar>()` would be 4 for `*-pc-windows-msvc` and 1 for everything else.


IMO this is extremely unintuitive behavior, especially for those of us writing custom derives (like zerocopy). We can't just rely on querying align_of - we need to be able to parse a type's definition and understand the semantics of any repr attributes on it.

Also, while this is technically not a breaking change, I wouldn't be surprised if some custom derive code implicitly assumes that this behavior isn't possible, and would become unsound in the face of this change. For example, as it stands today, it is valid to assume that a #[repr(align(N))] type has alignment at least N, but that would stop being valid with this change.

I'd propose that if system-specific behavior like this is required, it'd be better to do it behind a new repr so that the behavior of repr(C) remains straightforward.

as it stands today, it is valid to assume that a #[repr(align(N))] type has alignment at least N, but that would stop being valid with this change

Did you say this wrong somehow? By this RFC, align(N) types are still always aligned to at least N when behind a reference. In fact, they're aligned to N in more cases, as packed types can't underalign them with MSVC layout rules.

What changes is that packed(M) types no longer have an alignment of exactly M, but instead have an alignment of at least M.

custom derive code might be unsound

Such custom derive code is already likely quite iffy, since #[derive(Trait)] #[proc_macro] struct will have the derive see the token stream before the proc macro processes it and can modify all the decorated code arbitrarily. zerocopy is aware of this and doesn't permit any attribute it doesn't know the reserved semantics of to be in the type definition, but that's a high, difficult, and restrictive bar to clear most of the time.

as it stands today, it is valid to assume that a #[repr(align(N))] type has alignment at least N, but that would stop being valid with this change

Did you say this wrong somehow? By this RFC, align(N) types are still always aligned to at least N when behind a reference. In fact, they're aligned to N in more cases, as packed types can't underalign them with MSVC layout rules.

What changes is that packed(M) types no longer have an alignment of exactly M, but instead have an alignment of at least M.

You're right, I misread the text. Still, there's a concern: Our derives currently take #[repr(C, packed(N))] as a guarantee that the decorated type cannot have alignment greater than N. This is consistent with the Reference:

For packed, ... the alignments of each field, for the purpose of positioning fields, is the smaller of the specified alignment and the alignment of the field’s type.

IIUC, to make packed no longer provide this guarantee is a breaking change with respect to the Reference.

There's another concern as well: This makes "whether or not the type has an alignment repr (packed/align)" part of the type's layout. The Reference currently specifies how types are laid out purely in terms of the sizes and alignments of their field types, and not in terms of any other facts about the field types. With this change, the following two Bar types would have different layouts despite containing Foo types with identical sizes and alignments:

#[repr(C, align(4))] struct Foo(u8); #[repr(C, packed(1))] struct Bar(Foo); #[repr(C)] struct Foo(u8, [u32; 0]); #[repr(C, packed(1))] struct Bar(Foo);

#[repr(C, align(4))] struct Foo(u8); #[repr(C, packed(1))] struct Bar(Foo);

This does not compile today (reference said "a packed type cannot transitively contain another aligned type"), so we are free to choose whether the two Bars should have same or different layouts after this RFC.

error[E0588]: packed type cannot transitively contain a `#[repr(align)]` type --> src/lib.rs:4:1 | 4 | struct Bar(Foo); | ^^^^^^^^^^ | note: `Foo` has a `#[repr(align)]` attribute --> src/lib.rs:2:1 | 2 | struct Foo(u8); | ^^^^^^^^^^

My point isn't that this is a breaking change, but rather than it breaks the current mental model, which is that a type's layout is determined solely by the size and alignment of its fields. This RFC changes that to say that a type's layout is determined by the size, alignment, and representation of its fields. Since representation is a fairly complicated concept (it can include repr(C), repr(Int), repr(packed), repr(align), repr(transparent), and some-but-not-all combinations of these), IMO it significantly complicates the mental model of type layout to say that a type's layout also depends upon its fields' representations.

Unfortunately, the existing. mental model turned out to be in contradiction to reality, at least on MSVC targets. When you build a model and then reality demonstrates the incorrectness of the model, sometimes the best option is to fix your model. Yes we documented this model in a bunch of places, but... what else could we do? Stick our head into the sand and continue pretending that the simpler model is "good enough"?

So now the layout of a type is determined by the size, alignment, and explicitly requested alignment of its fields.

Unfortunately, the existing mental model turned out to be in contradiction to reality, at least on MSVC targets. When you build a model and then reality demonstrates the incorrectness of the model, sometimes the best option is to fix your model.

There is another alternative: argue that because __declspec(align) and #pragma pack are nonstandard C extensions, we don't necessarily need to be layout-compatible with the system C in these cases. That's basically the argument for a struct with one zero-length array member on MSVC being zero-sized and not align-sized.

Counter to my own point, though, is that struct alignas(N) Name is standard C*++*, and the MSVC behavior is the same as with declspec in C mode. #pragma pack is still nonstandard (and I find MSVC's definition of it to be bonkers) but standard type alignas significantly weakens the argument that it isn't necessary to support.

But if there exists a type on MSVC with basic alignment higher than the default member packing level without also having an explicitly requested alignment (thus not being sufficiently aligned by default), then I would swing towards writing of MSVC layout edge cases as broken.

This does not compile today

Unfortunately, this error is straightforward to sidestep with generics.

#[repr(align(4))] struct Aligned; #[repr(packed(1))] struct Packed<T = Aligned>(T); fn main() { dbg!(align_of::<Packed>()); // [src/main.rs:8:5] align_of::<Packed>() = 1 }

tmccombs · 2024-11-01T03:53:36Z

text/0000-layout-packed-aligned.md

+[guide-level-explanation]: #guide-level-explanation
+
+## `#[repr(C)]`
+When `align` and `packed` attributes exist on the same type, or when `packed` structs transitively contains `align` types,


Is there any difference in behavior between the c, system, and rust for types with both packed and align?

My expectation would be that regardless of whether it is c, system, or rust, if I have repr(packed(2),align(4)) the alignment of the overall type is 4, and the alignment of it's fields is at most 2 unless maybe one of those fields itself has an alignment specified. My reading of the reference section agrees with that, but the guide section is a little ambiguous.

And FWIW, it would be more intuitive to me if packed ignored the alignment of field types, but had a way to specify higher alignment for individual fields.

As currently written, #[repr(align)] used in a field's type definition (or transitively in its fields' types') could still raise the alignment of the type beyond the minimum alignment provided by the attribute, if the "infectious" alignment is in use (MSVC layout).

In MSVC C, such would theoretically be a compilation error (alignas cannot be used to lower alignment). I don't know what the behavior of templated C++ is, actually.

This is consistent with the behavior of repr(align) for an alignment less than the alignment required for primitive field alignment.

RalfJung · 2024-11-01T06:45:13Z

text/0000-layout-packed-aligned.md

+# Drawbacks
+[drawbacks]: #drawbacks
+
+Historically the meaning of `#[repr(C)]` has been somewhat ambiguous. When someone puts `#[repr(C)]` on their struct, their intention could be one of three things:


https://doc.rust-lang.org/reference/type-layout.html#the-c-representation actually documents the meaning of repr(C) quite clearly: it means types are laid out linearly, according to a fixed algorithm. So this RFC is proposing a breaking change, and unsafe code that relies on what is documented in the Reference might become subtly unsound if this RFC gets implemented.

That should at least be mentioned and discussed as a drawback.

Isn't such a breaking change an explicit violation of Rust's stability policy? Though I can't actually find where it's documented ATM, my understanding is that breaking changes are only permitted in the following cases:

The breakage is due to type inference

The breakage is required in order to fix a security issue

As described in #3718 (comment), it should be simple to avoid this problem by introducing a new repr (in addition to the proposed #[repr(system)]) instead of changing the behavior of #[repr(C)].

It's definitely in a gray area. One cold also argue that the existing repr(C) is simply wrong/unsound, and we have to fix it to satisfy its promise of providing type layout compatible with the current target's C ABI.

iirc someone suggested deprecating repr(C) entirely and replacing it with repr(linear), repr(really_C_bikeshed), and other reprs as necessary.

RalfJung · 2024-11-01T06:46:27Z

text/0000-layout-packed-aligned.md

+- The field is added to the struct with alignment increased to N. The alignment requirement overrides the packing requirement. (MSVC, `#[repr(C)]` on msvc targets, `#[repr(system)]` on windows targets)
+
+# Drawbacks
+[drawbacks]: #drawbacks


Due to this, this RFC will actually change the layout of some types that are currently accepted on stable, on MSVC targets. That should be discussed as a drawback.

retep998 · 2024-11-01T08:55:55Z

Yay, finally some progress on this front! I haven't dug into the details of this RFC, but based on a quick skim I am generally in favor.

CAD97 · 2024-11-01T18:42:18Z

An alternative, more minimal spot fix could be as such:

Allow #[repr(align(N), packed(M))] on structs with the semantics of packed(M) wrapped in align(N).
Allow #[repr(align(N))] on fields to request a higher alignment for the field without introducing a type wrapper or additional fields.

This would allow tools like bindgen to implement the odd MSVC packing rules without putting them into the language.

text/0000-layout-packed-aligned.md

Co-authored-by: Christopher Durham <[email protected]>

CAD97 · 2024-11-12T19:17:13Z

What changes is that packed(M) types no longer have an alignment of exactly M, but instead have an alignment of at least M.

…Actually, if we want to exactly match the semantics of #pragma pack on MSVC, the "correct" behavior would actually be to allow the type to have alignment less than M still. MSVC's #pragma pack does not raise the alignment of a struct to the packing width, and in fact, x64 and ARM64EC operates under #pragma pack(16) by default. (x86, ARM, and ARM64 default to #pragma pack(8).)

There is actual utility in #[packed] working this way in Rust for generic containers. I don't think that the utility is anywhere near enough for the change, but it bears keeping in mind.

RalfJung · 2024-11-12T19:22:17Z

MSVC's #pragma pack does not raise the alignment of a struct to the packing width

Neither does our attribute?

CAD97 · 2024-11-13T07:27:55Z

Oh, I, uh, misremembered overconfidently. Oops.

Neo-Zhixing added 3 commits October 24, 2024 14:18

Initial writeup

8518173

Update PR numbers

3d44198

update

e77c6be

ehuss added the T-lang Relevant to the language team, which will review and decide on the RFC. label Oct 24, 2024

kennytm reviewed Oct 25, 2024

View reviewed changes

text/0000-layout-packed-aligned.md Outdated Show resolved Hide resolved

Update

af3cc30

CAD97 requested changes Oct 25, 2024

View reviewed changes

Neo-Zhixing added 2 commits October 29, 2024 09:05

Updates

175d29f

Updates

a53aab1

scottmcm reviewed Oct 31, 2024

View reviewed changes

RalfJung reviewed Oct 31, 2024

View reviewed changes

joshlf reviewed Oct 31, 2024

View reviewed changes

tmccombs reviewed Nov 1, 2024

View reviewed changes

RalfJung reviewed Nov 1, 2024

View reviewed changes

RalfJung mentioned this pull request Nov 1, 2024

#[repr(align(N))] fields not allowed in #[repr(packed(M>=N))] structs rust-lang/rust#100743

Open

This was referenced Nov 1, 2024

bindgen wanted features & bugfixes Rust-for-Linux/linux#353

Open

Rust wanted features Rust-for-Linux/linux#354

Open

CAD97 reviewed Nov 1, 2024

View reviewed changes

text/0000-layout-packed-aligned.md Outdated Show resolved Hide resolved

Neo-Zhixing and others added 2 commits November 4, 2024 14:26

Update text/0000-layout-packed-aligned.md

5190399

Co-authored-by: Christopher Durham <[email protected]>

Update drawbacks

a12370d


		Clang matches the Windows ABI for `x86_64-pc-windows-msvc` and matches the GCC ABI for `x86_64-pc-windows-gnu`.

		MinGW always uses the GCC ABI.

[RFC] Allow packed types to transitively contain aligned types #3718

Are you sure you want to change the base?

[RFC] Allow packed types to transitively contain aligned types #3718

Conversation

Neo-Zhixing commented Oct 24, 2024

CAD97 left a comment • edited Loading

Choose a reason for hiding this comment

CAD97 Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrochenkov commented Oct 26, 2024

Neo-Zhixing commented Oct 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmcm commented Oct 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CAD97 Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

RalfJung Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

RalfJung Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

RalfJung Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

RalfJung Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

CAD97 Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retep998 commented Nov 1, 2024

CAD97 commented Nov 1, 2024 • edited Loading

CAD97 commented Nov 12, 2024

CAD97 left a comment •

edited

Loading

CAD97 Oct 25, 2024 •

edited

Loading

CAD97 Nov 1, 2024 •

edited

Loading

RalfJung Nov 1, 2024 •

edited

Loading

RalfJung Oct 31, 2024 •

edited

Loading

RalfJung Nov 1, 2024 •

edited

Loading

RalfJung Nov 1, 2024 •

edited

Loading

RalfJung Nov 1, 2024 •

edited

Loading

CAD97 Nov 1, 2024 •

edited

Loading

CAD97 commented Nov 1, 2024 •

edited

Loading