Fix a non-C-like-enumeration size mismatch between Rust and C when using Style::Tag.

[sw17ch]

When using Style::Tag with non-C-like-enumerations and discriminant types that do not match the size of enumerations in C (such as those with #[repr(C, u8)]), cbindgen emits types that are incompatible between Rust and C.

I have found two problems:

Inconsistent Padding

Here's the input type:

#[repr(C, u8)]
enum Tu64 {
    None,
    Some(u64),
}

Here's the generated C code:

enum Tu64_Tag {
  None,
  Some,
};
typedef uint8_t Tu64_Tag;

struct Some_Body {
  uint64_t _0;
};

struct Tu64 {
  enum Tu64_Tag tag;
  union {
    struct Some_Body some;
  };
};

In this case, both C and Rust agree that Tu64 is sized as 16 bytes. The u64 forces its alignment to 8 bytes, so Rust designates the first byte as the discriminant, and the following 7 bytes as padding.

However, C has the tag defined as an enum Tu64_Tag. This means that the tag is sized the same way enumerations are on the host system. In our case, that's likely going to be 4 bytes. In C, the first 4 bytes are discriminant, and the following 4 are padding.

This means the interface between Rust and C is unsafe since the discriminant C reads may be influenced by noise in padding bytes that Rust may not initialize. For example, allocating a Tu64 in C, setting it all to 0xFF, then passing it by reference into Rust for initialization will result in a corrupted discriminant when C looks at the initialization result.

Inconsistent Sizes

We'll use an example similar to the one above, but replace the u64 with a u8.

#[repr(C, u8)]
enum Tu8 {
    None,
    Some(u8),
}

Here's the generated code:

enum Tu8_Tag {
  None,
  Some,
};
typedef uint8_t Tu8_Tag;

struct Some_Body {
  uint8_t _0;
};

struct Tu8 {
  enum Tu8_Tag tag;
  union {
    struct Some_Body some;
  };
};

In this case, Rust sees that the value we can hold in the enum and the discriminant are both of size 1, so we can safely pack them in 2 bytes.

However, in C, we can see that we're still using enum Tu8_Tag for the discriminant tag type. This means that the size in C is going to be at least the size of an enumeration + 1 byte, and will have an alignment other than the one used by Rust.

Passing a Tu8 from C to Rust results in Rust reading a corrupted body value. Passing a Tu8 from Rust to C will almost certainly result in corruption of the tag. Furthermore, any mutation C does to the Tu8 allocated by Rust can corrupt unrelated structures in Rust's memory by overflowing the bounds of Rust's Tu8.

Solution

In both cases, the solution is to use the typedef that's generated as the tag type in C. That is, using Tu64_Tag tag instead of enum Tu64_Tag tag and Tu8_Tag tag instead of enum Tu8_Tag tag.

Testing

This PR includes some extra infrastructure so that tests to verify this sort of thing can be written. The details of this test system are documented in the commit message of ea03db1. The test I added is at tests/checkers/tag/enum.c. The body of this test #includes the cbindgen generated file, and then verifies the expected size. If bac063e is checked out, this test is seen to fail in cargo test.

Fix

I have created another commit that appears to fix this issue (by not emitting the enum before the tag type name when the representation expresses a size), and does not cause any other tests to fail. I'm not convinced this is the best way to fix this, nor is the importance of the change in df0167b obvious. I would appreciate feedback about alternative ways to fix this that might be more clear to future readers.

[emilio]

Thanks! So, the fix looks good, but I have one suggestion / simplification that doesn't require adding all the "checkers" infrastructure. Given so far we only have use cases for static_asserts, is there any reason your patch can't be replaced by something like creating tests/rust/enum.toml, with something like:

trailer = """
  static_assert(sizeof(struct P) == 4, "unexpected size for P");
"""

In general it'd be good to have better end-to-end tests, but that requires at least linking the rust code too.

Probably if we do that, we should spend some effort to automatically verify the struct offsets and sizes and what not. But that requires either generating some rust code with assertions and such, which requires a bit more thought and probably a bit more work, or parsing -Z print-type-sizes, which doesn't sound terribly appealing.

So lacking that, it doesn't seem to me that the checker infrastructure brings much benefit over just (ab)-using the trailer option. Thoughts?

[emilio]

Is static_assert a C thing nowadays? since when?

[emilio]

(Oh, I found this btw, TIL that C11 won static_assert, that's great)

[emilio]

I opened #465 with a proposal with a slightly less invasive change which allows to test this as well, and has the side benefit of testing it on all the styles / configurations. WDYT?

[sw17ch]

This sounds great. I'm happy to add the trailers (I didn't realize trailers were a supported feature). If you merge #465 with the helper, I'll replace my test infrastructure in this test with trailers in the toml file.

Thank you!

[emilio]

Alright, merged that to master, as it was green. Thanks for bearing with me!

[sw17ch]

Force-pushed from 9d2b8fa to 228f7df. This drops the checkers tests in favor of static assertions in the trailers. I also added some uses of offsetof to demonstrate where the tags and bodies are expected. I've only done this for the single struct.

Using trailers was a much better idea. Thank you for the assistance!

[emilio]

Thank you, this looks great!