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

c_void should not be an enum with public variants #48

Closed
steveklabnik opened this issue Nov 7, 2015 · 5 comments
Closed

c_void should not be an enum with public variants #48

steveklabnik opened this issue Nov 7, 2015 · 5 comments

Comments

@steveklabnik
Copy link
Member

originally filed as rust-lang/rust#20274

@alexcrichton alexcrichton changed the title c_void seems problematic c_void should not be an enum with public variants Nov 20, 2015
@geofft
Copy link

geofft commented Apr 4, 2017

I don't think there's another approach - @tomjakubowski suggested #[repr(u8)] struct on the original bug, but unfortunately that's not valid syntax. And as soon as I add so much as a () member to one of the enums, it ceases to really be a u8 as far as LLVM's optimizations are concerned, although it's still size 1. (Hm, this seems vaguely like a bug.) Try playing around with this sample on rust.godbolt.org.

That said, I don't think this is a problem: it isn't memory-unsafe to be able to construct a c_void, so I don't think libc needs to do anything about this. If you go and say let x = c_void::__variant1, it's pretty clear you're doing something silly. But there's nothing that takes a c_void itself, or a &c_void or anything.; everything just takes *mut c_void or *const c_void, and you're already allowed to just cast any pointer to. I can't think of a way in which the existence of that variable x causes problems. So probably it's best to leave it like it is.

(It is more of a problem for libraries which are using the same trick to represent an opaque struct, i.e., what C means by struct foo; in a header file. If the C library wants you to call a constructor function to get a pointer to something, you shouldn't be able to just generate a member of that type. But for c_void, that's not a concern. And I'd argue that those libraries don't need or want the malloc/free optimization.)

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.
@dtolnay
Copy link
Member

dtolnay commented Jul 10, 2017

In my opinion there is no reason to change this. The implementation of c_void is just trivia for compiler experts and has no practical consequence.

If other people feel strongly that a struct is more appropriate, I think it is now possible with rust-lang/rust#43036:

#[repr(transparent)]
pub struct c_void(Void);

#[repr(u8)]
enum Void {
    Irrelevant,
    Likewise,
}

@gnzlbg
Copy link
Contributor

gnzlbg commented Feb 3, 2018

@dtolnay IIRC this should just be:

extern {
    type c_void;
}

but I might be misunderstanding what extern types were introduced for (I did not follow the RFC discussion about that back then).

@gnzlbg
Copy link
Contributor

gnzlbg commented Feb 3, 2018

Anyways any change in this direction should be done together with other breaking changes, so maybe one should label this as "worth experimenting with it for the 0.3 release" (not that a 0.3 release will ever happen until the whole ctypes situation progresses anyways).

@gnzlbg
Copy link
Contributor

gnzlbg commented Nov 22, 2018

libc now just re-exports core::ffi::c_void

@gnzlbg gnzlbg closed this as completed Nov 22, 2018
danielverkamp pushed a commit to danielverkamp/libc that referenced this issue Apr 28, 2020
…rrect

Add a test for x86 runtime support
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

4 participants