Implement the special repr(C)-non-clike-enum layout by Gankra · Pull Request #46123 · rust-lang/rust

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

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

Can you rename this to Prefixed?

oh right, forgot!

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

src/librustc/ty/layout.rs

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

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

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

&variants

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

Comments addressed

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

src/librustc/ty/layout.rs

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

prefix_size instead of discr_size should do nicely.

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

src/librustc/ty/layout.rs

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