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.

Sign up for GitHub

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
Closed

Deduplicate the code that turns transparent structs into typedefs #991

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ba96d90
Deduplicate the code that turns transparent structs into typedefs
scovich May 17, 2024
f6d956d
add missing test case for empty transparent struct
scovich Aug 12, 2024
0f4f3c3
handle empty transparent struct in a better way
scovich Aug 12, 2024
4935dd0
improve code comments about transparent empty structs
scovich Aug 12, 2024
1659532
revise ZST handling again
scovich Aug 13, 2024
bb92f00
fix typo
scovich Aug 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion src/bindgen/ir/structure.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use crate::bindgen::declarationtyperesolver::DeclarationTypeResolver;
use crate::bindgen::dependencies::Dependencies;
use crate::bindgen::ir::{
AnnotationSet, Cfg, Constant, Documentation, Field, GenericArgument, GenericParams, Item,
ItemContainer, Path, Repr, ReprAlign, ReprStyle, Type,
ItemContainer, Path, Repr, ReprAlign, ReprStyle, Type, Typedef,
};
use crate::bindgen::library::Library;
use crate::bindgen::mangle;
Expand Down Expand Up @@ -150,6 +150,23 @@ impl Struct {
}
}

/// Attempts to convert this struct to a typedef (only works for transparent structs).
pub fn as_typedef(&self) -> Option<Typedef> {
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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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

return Some(Typedef::new_from_struct_field(self, field));
} else {
scovich marked this conversation as resolved.
Show resolved Hide resolved
error!(
"Cannot convert empty transparent struct {} to typedef",
self.name()
);
}
}
None
}

pub fn add_monomorphs(&self, library: &Library, out: &mut Monomorphs) {
// Generic structs can instantiate monomorphs only once they've been
// instantiated. See `instantiate_monomorph` for more details.
Expand Down
17 changes: 15 additions & 2 deletions src/bindgen/ir/typedef.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ use crate::bindgen::config::Config;
use crate::bindgen::declarationtyperesolver::DeclarationTypeResolver;
use crate::bindgen::dependencies::Dependencies;
use crate::bindgen::ir::{
AnnotationSet, Cfg, Documentation, GenericArgument, GenericParams, Item, ItemContainer, Path,
Type,
AnnotationSet, Cfg, Documentation, Field, GenericArgument, GenericParams, Item, ItemContainer,
Path, Struct, Type,
};
use crate::bindgen::library::Library;
use crate::bindgen::mangle;
Expand Down Expand Up @@ -70,6 +70,19 @@ impl Typedef {
self.aliased.simplify_standard_types(config);
}

// Used to convert a transparent Struct to a Typedef.
pub fn new_from_struct_field(item: &Struct, field: &Field) -> Self {
Self {
path: item.path().clone(),
export_name: item.export_name().to_string(),
generic_params: item.generic_params.clone(),
aliased: field.ty.clone(),
cfg: item.cfg().cloned(),
annotations: item.annotations().clone(),
documentation: item.documentation().clone(),
}
}

pub fn transfer_annotations(&mut self, out: &mut HashMap<Path, AnnotationSet>) {
if self.annotations.is_empty() {
return;
Expand Down
18 changes: 0 additions & 18 deletions src/bindgen/language_backend/clike.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,24 +540,6 @@ impl LanguageBackend for CLikeLanguageBackend<'_> {
}

fn write_struct<W: Write>(&mut self, out: &mut SourceWriter<W>, s: &Struct) {
if s.is_transparent {
let typedef = Typedef {
path: s.path.clone(),
export_name: s.export_name.to_owned(),
generic_params: s.generic_params.clone(),
aliased: s.fields[0].ty.clone(),
cfg: s.cfg.clone(),
annotations: s.annotations.clone(),
documentation: s.documentation.clone(),
};
self.write_type_def(out, &typedef);
for constant in &s.associated_constants {
out.new_line();
constant.write(self.config, self, out, Some(s));
}
return;
}

let condition = s.cfg.to_condition(self.config);
condition.write_before(self.config, out);

Expand Down
18 changes: 0 additions & 18 deletions src/bindgen/language_backend/cython.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,24 +159,6 @@ impl LanguageBackend for CythonLanguageBackend<'_> {
}

fn write_struct<W: Write>(&mut self, out: &mut SourceWriter<W>, s: &Struct) {
if s.is_transparent {
let typedef = Typedef {
path: s.path.clone(),
export_name: s.export_name.to_owned(),
generic_params: s.generic_params.clone(),
aliased: s.fields[0].ty.clone(),
cfg: s.cfg.clone(),
annotations: s.annotations.clone(),
documentation: s.documentation.clone(),
};
self.write_type_def(out, &typedef);
for constant in &s.associated_constants {
out.new_line();
constant.write(self.config, self, out, Some(s));
}
return;
}

let condition = s.cfg.to_condition(self.config);
condition.write_before(self.config, out);

Expand Down
20 changes: 19 additions & 1 deletion src/bindgen/language_backend/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,24 @@ pub trait LanguageBackend: Sized {
}
}

/// If the struct is transparent, emit a typedef of its NZST field type instead.
fn write_struct_or_typedef<W: Write>(
&mut self,
out: &mut SourceWriter<W>,
s: &Struct,
b: &Bindings,
) {
if let Some(typedef) = s.as_typedef() {
self.write_type_def(out, &typedef);
for constant in &s.associated_constants {
out.new_line();
constant.write(&b.config, self, out, Some(s));
}
} else {
self.write_struct(out, s);
}
}

fn write_items<W: Write>(&mut self, out: &mut SourceWriter<W>, b: &Bindings) {
for item in &b.items {
if item
Expand All @@ -155,7 +173,7 @@ pub trait LanguageBackend: Sized {
ItemContainer::Constant(..) => unreachable!(),
ItemContainer::Static(..) => unreachable!(),
ItemContainer::Enum(ref x) => self.write_enum(out, x),
ItemContainer::Struct(ref x) => self.write_struct(out, x),
ItemContainer::Struct(ref x) => self.write_struct_or_typedef(out, x, b),
ItemContainer::Union(ref x) => self.write_union(out, x),
ItemContainer::OpaqueItem(ref x) => self.write_opaque_item(out, x),
ItemContainer::Typedef(ref x) => self.write_type_def(out, x),
Expand Down
Loading