Support repr(C) for rustified enums

[thedataking]

TODOs:

• check enum width as suggested by @emilio in this comment
• should there be a --rustified-non-exhaustive-repr-c-enum flag?
• Update CHANGELOG.md

Closes #3263.

[emilio]

Thanks! One nit

[emilio]

Actually, if we implement the suggested check for enum class Foo : uint8_t and so, we might not even need to add this switch? We could just repr(C) by default...

I think with this patch if you have something like:

enum class Foo : uint8_t {
};

Using this option will miscompile it.

At the very least we should add a big warning on this option since it'd cause wrong struct layout.

[thedataking]

At the very least we should add a big warning on this option since it'd cause wrong struct layout.

I agree this would be good. However, it appears that LLVM does not expose the necessary APIs. Even if we exposed the necessary functionality, it would presumably only work for recent clang versions. Thoughts on how to proceed?

[emilio]

What do you think of extending our layout tests to test the layout of enums? I think at least that would signal that there's something wrong with the generated code if you misuse this option.

Other than that it looks good.

[workingjubilee]

Our repr(C) layout is actually very likely to be incorrect for many enums and we are considering how to deprecate it because it is very hard to address this.

[workingjubilee]

@thedataking Your added test doesn't touch the case of most concern for repr(C): enums with values bigger than the platform's c_int.

[thedataking]

@thedataking Your added test doesn't touch the case of most concern for repr(C): enums with values bigger than the platform's c_int.

Good point, I added a test that requires the enum to have a 64-bit value. Any other cases of concern I should add?

[thedataking]

What do you think of extending our layout tests to test the layout of enums? I think at least that would signal that there's something wrong with the generated code if you misuse this option.

@emilio I've added test cases for repr(C) specifically. I can also add testcases for rustified enums more broadly but that is a larger change... do you want it to be part of this PR or could we add it in a separate PR?

[workingjubilee]

Good point, I added a test that requires the enum to have a 64-bit value. Any other cases of concern I should add?

In addition to u32::MAX + 1, I believe the other "touchy" cases are

• an enum with a value in i32::MAX + 1..=u32::MAX
• the previous enum but also with a negative value (so cannot be represented as u32)
• in general, enums with both positive and negative values vs. ones with just positive values
• anything in the i128 or u128 range

And maybe a "control" test for more usual-size enums without short-enums.

See rust-lang/rust#147017 for more on the issue here and the FCW that wound up landing.