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

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

Can you rename this to Prefixed?

oh right, forgot!

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

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

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

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

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

&variants

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

Comments addressed

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

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

prefix_size instead of discr_size should do nicely.

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

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

Comment thread src/librustc/ty/layout.rs Outdated

eddyb Nov 20, 2017

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&variants

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

Comment thread src/librustc/ty/layout.rs Outdated

eddyb Nov 20, 2017

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefix_size instead of discr_size should do nicely.

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

Uh oh!

Uh oh!

Conversation

Gankra commented Nov 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Nov 20, 2017

Uh oh!

Gankra commented Nov 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gankra commented Nov 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gankra commented Nov 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb commented Nov 20, 2017

Uh oh!

Gankra commented Nov 20, 2017

Uh oh!

carols10cents commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eddyb commented Nov 27, 2017

Uh oh!

nikomatsakis commented Nov 28, 2017

Uh oh!

eddyb commented Nov 28, 2017

Uh oh!

bors commented Nov 28, 2017

Uh oh!

bors commented Nov 28, 2017

Uh oh!

bors commented Nov 28, 2017

Uh oh!

Gankra commented Nov 28, 2017

Uh oh!

Gankra commented Nov 28, 2017

Uh oh!

eddyb commented Nov 28, 2017

Uh oh!

bors commented Nov 28, 2017

Uh oh!

bors commented Nov 28, 2017

Uh oh!

bors commented Nov 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Gankra commented Nov 20, 2017 •

edited

Loading

carols10cents commented Nov 27, 2017 •

edited

Loading