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

Document the correct FFI equivalent to a C opaque struct #27303

Closed
crumblingstatue opened this issue Jul 26, 2015 · 13 comments
Closed

Document the correct FFI equivalent to a C opaque struct #27303

crumblingstatue opened this issue Jul 26, 2015 · 13 comments

Comments

@crumblingstatue
Copy link
Contributor

With recent changes to the improper_ctypes lint, it now rejects #[repr(C)] struct Foo; for representing a C opaque struct.

#[repr(C)]
pub struct Foo;

extern "C" {
    pub fn foo(arg: *const Foo);
}

fn main() {
}
foo.rs:5:25: 5:28 warning: found zero-size struct in foreign module, consider adding a member to this struct, #[warn(improper_ctypes)] on by default
foo.rs:5    pub fn foo(arg: *const Foo);
                                   ^~~

On the #rust IRC channel, I have asked what is the proper way to represent a C opaque struct, but no definitive answer was reached.

Some of the answers were:

  • struct Foo; is incorrect.
  • struct Foo; is fine, but it can break certain LLVM optimizations.
  • Use c_void instead. This isn't satisfactory when it is desirable to represent a C opaque struct as a distinct type in your API.
  • Use a newtype wrapper around c_void. Someone said this might not be correct either.
  • Rustc should allow #repr(C) enum Foo {}. It doesn't currently allow this.

This should be properly researched and then documented in the official documentation, so users know what to use for representing C opaque struct types in Rust when interfacing with C code.

Some relevant issues:

@geofft
Copy link
Contributor

geofft commented Jul 26, 2015

It looks like the canonical answer here is a #[repr(u8)] enum containing two private variants, like libc::c_void, as described in PR #11539. But it's not clear whether that is simply the optimized option, or the only ABI-valid option for void *. std::os::raw::c_void also provides its own copy of the structure.

But @crumblingstatue wants to newtype this for type safety, and it isn't clear whether newtyping this structure (struct Foo(c_void)) gives us something of the same ABI as c_void itself. The rust repo itself doesn't seem to ever do so, but has a few type aliases (type Foo = *mut libc::c_void), which provide worse type safety than C, since these pointers can be freely assigned to each other.

A few places in the rust repo, some external docs, and some projects (including Servo) use *mut () as a void-pointer type.

I can see few options here:

  1. Stabilize the representation of a raw pointer to #[repr(C)] struct Foo; as ABI-compatible with C opaque struct pointers (which would incidentally stabilize *mut ()).
  2. Stabilize #[repr(C)] enum Foo {}, which was suggested on #rust as better because you can't construct one. Right now that doesn't compile, so this is clearly backwards-compatible.
  3. Ensure that the newtype approach, struct Foo(c_void), works, and stabilize it.
  4. Document that people should make their own two-private-variant #[repr(u8)] enums. Maybe make a macro?

Whatever the answer, it should be documented in the book's FFI section as well as in the doc comment on libc::c_void.

@alexcrichton
Copy link
Member

Note that the only reason c_void has two variants is to allow the #[repr(u8)] tag, and the only reason that's done is so LLVM can recognize malloc and free and enable the various optimizations around memory allocation. I don't believe there are any other optimizations you miss out on from something like struct Foo; or just a plain enum Foo {}.

I personally would not recommend struct Foo; as a pattern because it's not serving its purpose of an opaque type marker (you can construct an instance via Foo). In bindings I've written I typically use enum Foo {} (note the lack of repr) and it works when you only use *mut Foo in the FFI bindings.

@eefriedman
Copy link
Contributor

The reason we don't accept #[repr(C)] struct Foo; is that it isn't laid out the same way as an empty struct in C++.

@tomjakubowski
Copy link
Contributor

I personally would not recommend struct Foo; as a pattern because it's not serving its purpose of an opaque type marker (you can construct an instance via Foo). In bindings I've written I typically use enum Foo {} (note the lack of repr) and it works when you only use *mut Foo in the FFI bindings.

This is exactly what I've done in my projects, although I haven't tried with the newly rewritten improper-ctypes lint.

@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 5, 2015

Servo uses enum Foo {}.

@bluss
Copy link
Member

bluss commented Aug 5, 2015

@Ms2ger Doesn't the ffi-safe lint trigger on that too?

@tomjakubowski
Copy link
Contributor

@bluss raw pointers to Sized types are always allowed in FFI signatures, AFAIK.

steveklabnik added a commit to steveklabnik/rust that referenced this issue Aug 6, 2015
crumblingstatue added a commit to jeremyletang/rust-sfml that referenced this issue Aug 16, 2015
@goertzenator
Copy link

A consequence of using enums for opaque structs is that the generated documentation will move from the Structs section to the Enums section. This makes the documentation confusing and misleading especially when there are also "normal" structs and enums.

goertzenator added a commit to rusterlium/erlang_nif-sys that referenced this issue Aug 18, 2015
felixc added a commit to felixc/gexiv2-sys that referenced this issue Sep 14, 2015
@nox
Copy link
Contributor

nox commented Dec 31, 2015

Wouldn't struct Foo(Void) be better, for the problem mentioned by @goertzenator?

@geofft
Copy link
Contributor

geofft commented Dec 31, 2015

Oh yeah, I guess given the advice above that #[repr(u8)] isn't needed for correctness, just for the malloc and free optimization, I think you're right. (Making a newtype of libc::c_void was proposed above; the only objection was that the #[repr(u8)] wouldn't survive the newtype.)

It might be nice to have enum Void {} somewhere in libstd.

@nox
Copy link
Contributor

nox commented Dec 31, 2015

Maybe not Void, but an empty enum named Opaque or something like that. There is a void crate with a Void type that implements various traits already. We don't want such a thing.

@shepmaster
Copy link
Member

What about this to leave it named a "struct" in rustdoc:

struct Foo {
    marker: PhantomData<()>,
}

or perhaps

struct Foo {
    _unused: (),
}

Note that I'm avoiding struct Foo(PhantomData<()>) as Foo would still be a function you could attempt to call; the brace version fails earlier for let f = Foo.

@shepmaster
Copy link
Member

What about this to leave it named a "struct" in rustdoc:

Nah, this isn't a better idea, as this type is actually constructible (although it takes a smidge more work).

pzrq added a commit to daveh86/mgo_wt_database_extractor that referenced this issue Oct 19, 2016
geofft added a commit to geofft/rustonomicon that referenced this issue Apr 4, 2017
See rust-lang/rust#27303 and rust-lang/libc#48 for discussion of the
optimization, or just try compiling `unsafe {free(malloc(16))}` with various
Rust definitions of `void *` in the FFI binding.
geofft added a commit to geofft/rustonomicon that referenced this issue Apr 4, 2017
See rust-lang/rust#27303 and rust-lang/libc#48 for discussion of the
optimization, or just try compiling `unsafe {free(malloc(16))}` with various
Rust definitions of `void *` in the FFI binding.
bors-servo pushed a commit to servo/core-text-rs that referenced this issue Jul 5, 2017
Fix core-text related warnings (#62)

### Fix private-in-public warnings (error E0446)
Fixed by prepending `pub` to the offending types. Not sure if its the best way but it was the only thing that worked. Other methods I've tried to maintain privateness:
* Replace with an empty enum. That threw an error.
* Make it public and wrap within a private module. That threw an error too as `#[repr(C)]` does not accept modules

### Fix use of extern static warnings by wrapping in unsafe block (error E0133)
I think this is self-explanatory.

### Fix zero-size struct warnings on certain CT* types
Wrap around c_void as I believe the types are only used as pointers. Again, let me know if there is a better way to do this. I've tried following [this discussion](rust-lang/rust#27303) on opaque C types but I don't think there is a consensus. I came across this [discussion](Rust-SDL2/rust-sdl2#442) from the Rust SDL2 bindings and they eventually [opted](Rust-SDL2/rust-sdl2#445) for the solution that I implemented.

This is my first pull request (hence, first open source contribution) and I've been playing with Rust for the past few weeks. So please, if anything isnt up to standards or correctness, let me know! Also, to squash the other warnings, those changes would have to made in the modules where those types reside (core-graphics and core-foundation). I would gladly open a PR to fix those too if all looks good.

Thanks!

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/core-text-rs/65)
<!-- Reviewable:end -->
rthomas added a commit to rthomas/core-graphics-rs that referenced this issue Jul 27, 2017
This follows the servo pattern mentioned in
rust-lang/rust#27303 to replace the zero-sized
structs with an `enum`.
rthomas added a commit to rthomas/core-graphics-rs that referenced this issue Jul 27, 2017
This follows the servo pattern mentioned in
rust-lang/rust#27303 to replace the zero-sized
structs with an `enum`.
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

No branches or pull requests