Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deduplicate the code that turns transparent structs into typedefs #991

Closed
wants to merge 6 commits into from

Conversation

scovich
Copy link
Contributor

@scovich scovich commented Aug 12, 2024

This is a "prefactor" for #966, which centralizes the way cbindgen handles struct definitions that should be emitted as typedefs instead. For example, all backends emit NonZero<T> as a simple typedef, and the C backend emits Option<T> as a typedef as well. With this change, the code is generalized so that struct typedefs can be handled cleanly and centrally, instead of replicating code in each backend that needs it. It also opens a future path to handling transparent enums in the same way.

NOTE: With this change, cbindgen also handles zero-sized structs gracefully instead of crashing.

Copy link
Collaborator

@emilio emilio left a comment

Choose a reason for hiding this comment

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

Looks great, thanks! Just some minor questions

src/bindgen/ir/structure.rs Outdated Show resolved Hide resolved
if self.is_transparent {
// NOTE: A `#[repr(transparent)]` struct with 2+ NZT fields fails to compile, but 0
// fields is allowed for some strange reason. Don't emit the typedef in that case.
if let Some(field) = self.fields.first() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a test for #[repr(transparent)] struct Foo; or so, so that we have test-coverage for the None branch?

Copy link
Contributor Author

@scovich scovich Aug 12, 2024

Choose a reason for hiding this comment

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

I tried, but it exposed a latent cbindgen bug for test cases that enable struct tagging:

Output failed to compile: Output {
  status: ExitStatus(unix_wait_status(256)),
  stdout: "",
  stderr: "
    /path/to/cbindgen/tests/expectations/transparent_tag.compat.c:43:11: 
    error: must use 'struct' tag to refer to type 'TransparentEmptyStructure'
    TransparentEmptyStructure h,
    ^
    struct
    1 error generated.
  "
}

If I run the same test against master, cbindgen itself fails:

cbindgen failed: Some("/path/to/cbindgen/tests/expectations/transparent.compat.c") with error: 
thread 'main panicked at src/bindgen/language_backend/clike.rs:548:34:
index out of bounds: the len is 0 but the index is 0

This only happens if the empty struct is marked #[repr(transparent)].

My newly added check avoided the index out of bounds panic, but I don't know where the tagging issue would lurk?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also filed rust-lang/rust#129029, because it seems like the rust compiler shouldn't allow an empty transparent struct in the first place.

Copy link
Contributor Author

@scovich scovich Aug 12, 2024

Choose a reason for hiding this comment

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

Found it: structure.rs:274:

     fn collect_declaration_types(&self, resolver: &mut DeclarationTypeResolver) {
-        if self.is_transparent {
+        if self.is_transparent && self.fields.len() == 1 {
             resolver.add_none(&self.path);
         } else {
             resolver.add_struct(&self.path);
         }
     }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The above fix is surgical, but I'm not sure it's ideal? Should we forbid is_transparent for an empty struct, rather than tolerate it like this code currently does? I worry the checks are proliferating...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and moved the check into the constructor, so that transparent implies single field everywhere else.

Copy link
Contributor Author

@scovich scovich Aug 13, 2024

Choose a reason for hiding this comment

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

Hmm. It's apparently intentional behavior to allow an empty transparent struct: rust-lang/rust#77841 (comment). But I don't know what the underlying type should be in that case, if we were to emit a typedef. Should we continue suppressing the typedef for empty transparent structs? Or is there some canonical type we should use as underlying for the typedef?

Copy link
Contributor Author

@scovich scovich Aug 13, 2024

Choose a reason for hiding this comment

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

Update: All zero-sized structs now trigger a warning about undefined behavior, and an empty struct definition is emitted regardless of whether the user asked for repr(C) or repr(transparent).

@scovich scovich requested a review from emilio August 12, 2024 23:45
@emilio
Copy link
Collaborator

emilio commented Aug 14, 2024

Squashed the patches, closed by 3ed9434

@emilio emilio closed this Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants