Implement the special repr(C)-non-clike-enum layout #46123

Gankra · 2017-11-20T17:31:54Z

This is the second half of rust-lang/rfcs#2195

which specifies that

#[repr(C, u8)]
#[derive(Copy, Clone, Eq, PartialEq, Debug)]
enum MyEnum {
    A(u32),                 // Single primitive value
    B { x: u8, y: i16 },    // Composite, and the offset of `y` depends on tag being internal
    C,                      // Empty
    D(Option<u32>),         // Contains an enum
    E(Duration),            // Contains a struct
}

Has the same layout as

#[repr(C)]
struct MyEnumRepr {
    tag: MyEnumTag,
    payload: MyEnumPayload,
}

#[repr(C)]
#[allow(non_snake_case)]
union MyEnumPayload {
    A: MyEnumVariantA,
    B: MyEnumVariantB,
    D: MyEnumVariantD,
    E: MyEnumVariantE,
}

#[repr(u8)] #[derive(Copy, Clone)] enum MyEnumTag { A, B, C, D, E }
#[repr(C)] #[derive(Copy, Clone)] struct MyEnumVariantA(u32);
#[repr(C)] #[derive(Copy, Clone)] struct MyEnumVariantB {x: u8, y: i16 }
#[repr(C)] #[derive(Copy, Clone)] struct MyEnumVariantD(Option<u32>);
#[repr(C)] #[derive(Copy, Clone)] struct MyEnumVariantE(Duration);

rust-highfive · 2017-11-20T17:31:58Z

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

Gankra · 2017-11-20T17:32:25Z

r? @eddyb

eddyb · 2017-11-20T17:52:51Z

src/librustc/ty/layout.rs

@@ -945,6 +945,10 @@ impl<'a, 'tcx> LayoutDetails {
            /// A univariant, but part of an enum.
            EnumVariant(Integer),
        }
+
+        // To be updated further down, if needed.
+        let enum_union_align = ::std::cell::Cell::new(Align::from_bytes(1, 1).unwrap());


You're not using this, are you?

Gankra · 2017-11-20T19:24:26Z

Changed impl to reflect discussion in IRC. Also fixed a test that was asserting the old behaviour.

eddyb · 2017-11-20T19:27:07Z

src/librustc/ty/layout.rs

@@ -943,8 +943,9 @@ impl<'a, 'tcx> LayoutDetails {
            /// A univariant, the last field of which may be coerced to unsized.
            MaybeUnsized,
            /// A univariant, but part of an enum.
-            EnumVariant(Integer),
+            EnumVariant(Size, Align),


Can you rename this to Prefixed?

oh right, forgot!

eddyb · 2017-11-20T19:28:21Z

src/librustc/ty/layout.rs

@@ -962,13 +963,11 @@ impl<'a, 'tcx> LayoutDetails {
            let mut inverse_memory_index: Vec<u32> = (0..fields.len() as u32).collect();

            // Anything with repr(C) or repr(packed) doesn't optimize.
+            let can_optimize = (repr.flags & ReprFlags::IS_UNOPTIMISABLE).is_empty();


I think I prefer naming this optimize, making it mutable, and &=-ing it below instead.

eddyb · 2017-11-20T19:29:48Z

src/librustc/ty/layout.rs

                }
+                // Round the offset up the union's aggregate alignment.
+                offset = discr_size.abi_align(union_align);


What I meant was that you wouldn't need special discriminant vs union semantics, just applying a size&align prefix.

eddyb · 2017-11-20T19:30:30Z

src/librustc/ty/layout.rs

+                // repr(C) on an enum tells us to make a (tag, union) layout,
+                // so we need to compute the alignment of the union here.
+                // (enum_union_align is checked by univariant_uninterned)
+                let mut union_align = min_ity.align(dl);


Rename this to prefix_align, I guess.

But it's not how aligned the prefix should be, it's how aligned the suffix should be.

It's the alignment imposed by the prefix, the logic looking at it need not know why it's chosen the way it is.

eddyb · 2017-11-20T19:30:39Z

src/librustc/ty/layout.rs

+                // (enum_union_align is checked by univariant_uninterned)
+                let mut union_align = min_ity.align(dl);
+                if def.repr.c() {
+                    for fields in variants.iter() {


&variants

Gankra · 2017-11-20T20:08:26Z

Comments addressed

eddyb · 2017-11-20T20:09:38Z

src/librustc/ty/layout.rs

@@ -943,7 +943,7 @@ impl<'a, 'tcx> LayoutDetails {
            /// A univariant, the last field of which may be coerced to unsized.
            MaybeUnsized,
            /// A univariant, but part of an enum.


Can you change the comment to say "with a prefix of an arbitrary size & alignment (e.g. enum tag)" instead of "but part of an enum"?

eddyb · 2017-11-20T20:10:32Z

src/librustc/ty/layout.rs

@@ -1001,12 +998,11 @@ impl<'a, 'tcx> LayoutDetails {

            let mut offset = Size::from_bytes(0);

-            if let StructKind::EnumVariant(discr) = kind {
-                offset = discr.size();
+            if let StructKind::Prefixed(discr_size, prefix_align) = kind {


prefix_size instead of discr_size should do nicely.

eddyb · 2017-11-20T20:12:18Z

src/librustc/ty/layout.rs

@@ -1558,10 +1554,21 @@ impl<'a, 'tcx> LayoutDetails {
                let mut start_align = Align::from_bytes(256, 256).unwrap();
                assert_eq!(Integer::for_abi_align(dl, start_align), None);

+                // repr(C) on an enum tells us to make a (tag, union) layout,
+                // so we need to compute the alignment of the union here.


This comment could be expanded to describe how increasing the prefix alignment produces the same variant layouts as if they were separately computed as structs and then had some space inserted in the front.

eddyb · 2017-11-20T20:13:30Z

cc @rust-lang/compiler @rust-lang/lang LGTM, should this wait on the RFC / have a FCP?

Gankra · 2017-11-20T20:19:09Z

Comments addressed.

carols10cents · 2017-11-27T15:13:19Z

eddyb (Eduard-Mihai Burtescu) approved these changes 7 days ago

soooo do I hear an r+ @eddyb? or an fcp merge? :)

eddyb · 2017-11-27T23:45:53Z

@carols10cents But nobody answered...

nikomatsakis · 2017-11-28T00:04:34Z

hould this wait on the RFC / have a FCP?

I do expect the RFC to be accepted, but I don't believe we have to block on it. I believe the current behavior is undefined, so we are within our rights to adjust it.

FCP might be reasonable, though I think not really necessary, I don't feel like major controversy is expected here.

eddyb · 2017-11-28T00:05:08Z

@bors r+

bors · 2017-11-28T00:05:09Z

📌 Commit 904ccbc has been approved by eddyb

bors · 2017-11-28T04:42:58Z

⌛ Testing commit 904ccbc9c2b1f8f24664a7ef89071738954f0028 with merge 2e0fd271c40ec90c0d6fa5b99929b81f12413d63...

bors · 2017-11-28T05:38:16Z

💔 Test failed - status-travis

Gankra · 2017-11-28T05:49:36Z

uhhh so apparently

#[repr(u64)]
enum E6 {
    A(u8, u16, u8),
    B(u8, u16, u8)
}

is 16 bytes on i586-gnu-i686-musl, but u64 is only aligned to 4 bytes?

wat

[00:50:11] thread 'main' panicked at 'assertion failed: `(left == right)`
[00:50:11]   left: `16`,
[00:50:11]  right: `12`', /checkout/src/test/run-pass/multiple-reprs.rs:78:4

    assert_eq!(size_of::<E6>(), align_size(12, align_of::<u64>()));

Gankra · 2017-11-28T05:55:08Z

nevermind, I'm dumb. 6 + 8 != 12. On 64-bit that error didn't matter.

eddyb · 2017-11-28T06:09:20Z

@bors r+

bors · 2017-11-28T06:09:21Z

📌 Commit 0e63d27 has been approved by eddyb

bors · 2017-11-28T08:05:04Z

⌛ Testing commit 0e63d27 with merge 436ac89...

Implement the special repr(C)-non-clike-enum layout This is the second half of rust-lang/rfcs#2195 which specifies that ```rust #[repr(C, u8)] #[derive(Copy, Clone, Eq, PartialEq, Debug)] enum MyEnum { A(u32), // Single primitive value B { x: u8, y: i16 }, // Composite, and the offset of `y` depends on tag being internal C, // Empty D(Option<u32>), // Contains an enum E(Duration), // Contains a struct } ``` Has the same layout as ```rust #[repr(C)] struct MyEnumRepr { tag: MyEnumTag, payload: MyEnumPayload, } #[repr(C)] #[allow(non_snake_case)] union MyEnumPayload { A: MyEnumVariantA, B: MyEnumVariantB, D: MyEnumVariantD, E: MyEnumVariantE, } #[repr(u8)] #[derive(Copy, Clone)] enum MyEnumTag { A, B, C, D, E } #[repr(C)] #[derive(Copy, Clone)] struct MyEnumVariantA(u32); #[repr(C)] #[derive(Copy, Clone)] struct MyEnumVariantB {x: u8, y: i16 } #[repr(C)] #[derive(Copy, Clone)] struct MyEnumVariantD(Option<u32>); #[repr(C)] #[derive(Copy, Clone)] struct MyEnumVariantE(Duration); ```

bors · 2017-11-28T10:41:41Z

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 436ac89 to master...

rust-highfive assigned petrochenkov Nov 20, 2017

rust-highfive assigned eddyb and unassigned petrochenkov Nov 20, 2017

Gankra mentioned this pull request Nov 20, 2017

Implement Support For repr(int) non-C-like enums mozilla/cbindgen#78

Closed

eddyb reviewed Nov 20, 2017

View reviewed changes

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 20, 2017

Gankra force-pushed the c-repr branch from b43db9c to de8094f Compare November 20, 2017 19:23

eddyb reviewed Nov 20, 2017

View reviewed changes

Gankra force-pushed the c-repr branch from de8094f to 29c49fa Compare November 20, 2017 20:08

eddyb reviewed Nov 20, 2017

View reviewed changes

Gankra force-pushed the c-repr branch from 29c49fa to ceeb812 Compare November 20, 2017 20:13

Gankra added 3 commits November 20, 2017 15:18

Implement the special repr(C)-non-clike-enum layout

8293fe9

Prevent repr(C, u8) from triggering a warning on non-clike enums

3d7a6fe

Add tests for repr(C)-non-clike-enum layout

7e79b9b

Gankra force-pushed the c-repr branch from ceeb812 to 904ccbc Compare November 20, 2017 20:18

eddyb approved these changes Nov 20, 2017

View reviewed changes

Gankra mentioned this pull request Nov 21, 2017

Formally define repr(u32, i8, etc...) and repr(C) on enums with payloads rust-lang/rfcs#2195

Merged

kennytm added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 23, 2017

kennytm added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Nov 23, 2017

Fix and improve test for enum repr sizes

0e63d27

Gankra force-pushed the c-repr branch from 904ccbc to 0e63d27 Compare November 28, 2017 05:54

bors merged commit 0e63d27 into rust-lang:master Nov 28, 2017

eddyb mentioned this pull request May 1, 2018

Correct initial field alignment for repr(C)/repr(int) #50354

Merged

sw17ch mentioned this pull request Jan 13, 2020

SystemV ABI Mismatch on x86 with a repr(C) enum for extern "C"/FFI functions. #68190

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the special repr(C)-non-clike-enum layout #46123

Implement the special repr(C)-non-clike-enum layout #46123

Gankra commented Nov 20, 2017 •

edited

Loading

rust-highfive commented Nov 20, 2017

Gankra commented Nov 20, 2017

eddyb Nov 20, 2017

eddyb Nov 20, 2017

Gankra commented Nov 20, 2017

eddyb Nov 20, 2017

Gankra Nov 20, 2017

eddyb Nov 20, 2017

eddyb Nov 20, 2017

eddyb Nov 20, 2017

Gankra Nov 20, 2017

eddyb Nov 20, 2017

eddyb Nov 20, 2017

Gankra commented Nov 20, 2017

eddyb Nov 20, 2017

eddyb Nov 20, 2017

eddyb Nov 20, 2017

eddyb commented Nov 20, 2017

Gankra commented Nov 20, 2017

carols10cents commented Nov 27, 2017 •

edited

Loading

eddyb commented Nov 27, 2017

nikomatsakis commented Nov 28, 2017

eddyb commented Nov 28, 2017

bors commented Nov 28, 2017

bors commented Nov 28, 2017

bors commented Nov 28, 2017

Gankra commented Nov 28, 2017

Gankra commented Nov 28, 2017

eddyb commented Nov 28, 2017

bors commented Nov 28, 2017

bors commented Nov 28, 2017

bors commented Nov 28, 2017

Implement the special repr(C)-non-clike-enum layout #46123

Implement the special repr(C)-non-clike-enum layout #46123

Conversation

Gankra commented Nov 20, 2017 • edited Loading

rust-highfive commented Nov 20, 2017

Gankra commented Nov 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gankra commented Nov 20, 2017

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

Gankra commented Nov 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Nov 20, 2017

Gankra commented Nov 20, 2017

carols10cents commented Nov 27, 2017 • edited Loading

eddyb commented Nov 27, 2017

nikomatsakis commented Nov 28, 2017

eddyb commented Nov 28, 2017

bors commented Nov 28, 2017

bors commented Nov 28, 2017

bors commented Nov 28, 2017

Gankra commented Nov 28, 2017

Gankra commented Nov 28, 2017

eddyb commented Nov 28, 2017

bors commented Nov 28, 2017

bors commented Nov 28, 2017

bors commented Nov 28, 2017

Gankra commented Nov 20, 2017 •

edited

Loading

carols10cents commented Nov 27, 2017 •

edited

Loading