Support repr(C) for rustified enums - #3265
Conversation
33d5339 to
1885dc0
Compare
Note addition of rust-lang#3265
emilio
left a comment
emilio
left a comment
There was a problem hiding this comment.
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.
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? |
Note addition of rust-lang#3265
ec7b724 to
d020ef1
Compare
|
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. |
|
Our |
d020ef1 to
61f39db
Compare
Note addition of rust-lang#3265
|
@thedataking Your added test doesn't touch the case of most concern for |
Note addition of rust-lang#3265
867ed28 to
a11cced
Compare
Good point, I added a test that requires the enum to have a 64-bit value. Any other cases of concern I should add? |
@emilio I've added test cases for |
In addition to
And maybe a "control" test for more usual-size enums without See rust-lang/rust#147017 for more on the issue here and the FCW that wound up landing. |
Note addition of rust-lang#3265
a11cced to
a76013f
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
873a66f to
af02d12
Compare
It is not possible to control the repr via custom attributes so add a new rustified enum variant which does not use repr(u*) or repr(i*). Using repr(C) is sometimes necessary to bindgen enums used in functions subject to cross-language CFI checks. Closes 3263. Link: https://rcvalle.com/docs/rust-cfi-design-doc/ Signed-off-by: Per Larsen <perlarsen@google.com>
Note addition of rust-lang#3265
Add layout tests for --rustified-repr-c-enum covering small and large enum values. Emit size/align asserts during codegen for enums. Signed-off-by: Per Larsen <perlarsen@google.com>
Rust sizes a fieldless #[repr(C)] enum like the target's default C enum type, so C/C++ enums with a different layout - e.g. due to -fshort-enums, an explicit underlying type, or values outside the C int range - produce ABI-incompatible bindings. Emit a warning during code generation when such an enum's layout differs from the usual 4 bytes, and document the caveat on the --rustified-repr-c-enum and --default-enum-style options. Also list the new rust_repr_c styles in the invalid enum style error message. Signed-off-by: Per Larsen <perlarsen@google.com>
Cover an enum value in the i32::MAX+1..=u32::MAX range, an enum mixing negative and positive values, and one whose mixed-sign values only fit in 64 bits. The case triggers rustc's repr_c_enums_larger_than_int future-compatibility lint. Signed-off-by: Per Larsen <perlarsen@google.com>
af02d12 to
1fc93f6
Compare
5 participants
Adds a rustified enum style that uses repr(C) instead of a fixed integer
repr. Cross-language CFI requires this for enums passed across the FFI
boundary, since the Rust and C/C++ function types must agree
(https://rcvalle.com/docs/rust-cfi-design-doc/).
Rust's repr(C) does not match every C/C++ enum layout, e.g. with
-fshort-enums, an explicit underlying type, or values outside the c_int
range. bindgen now warns at generation time when the enum's layout is not
int-sized, the option docs call out the caveat, and layout tests are
emitted for repr(C) rustified enums so a mismatch fails to compile.
Closes #3263.