Unify std::os::raw::c_void and libc::c_void via libcore

[SimonSapin]

Unify std::os::raw::c_void and libc::c_void by making them both re-exports of a definition in libcore.

Rendered

[SimonSapin]

CC @rust-lang/libs

[gnzlbg]

Nice RFC, thanks. Thumbs up. All those issues mentioned were tangentially blocked on making c_void and extern type but I think that change is orthogonal to where exactly we decide to put it. They also seemed blocked on the misunderstanding that we would have to maybe also move some other ctypes (e.g. c_int) to libcore which isn't true either.

This RFC proposes core::ffi rather than core::os::raw on the basis that C-compatible types are misplaced in std::os::raw. std::os is documented as “OS-specific functionality”, but everything currently available under std::os::raw is about interoperabily with C rather than operating system functionality. (Although the exact definition of c_char, c_long, and c_ulong does vary based on the target operating system.) FFI stands for Foreign Function Interface and is about calling or being called from functions in other languages such as C. So the ffi module seems more appropriate than os for C types, and it already exists in std.

I don't personally want to bikeshed this, I think core::ffi for c_void is a fine location.

My understanding was that the reason these types where in ::os was that their size, alignment, etc. is OS / platform dependent. For example, c_int can be 32 or 64 bits (or something else), depending on the OS flavor you use.

That is, moving these types to core would make core platform dependent, but as I understood it, core should be platform independent.

The problem that extern type was trying to solve is that of making core platform dependent if we move c_void there. I think that's a problem worth solving, but since I haven't heard any arguments that suggest that moving only c_void to core would lead to any headaches, I think we could just do that later.

So I am all in with this RFC. The unresolved questions about moving ctypes in std::os to std::ffi or similar, I'd just leave them open as to be resolved by some other RFC. I don't think it is worth it to delay this for debating that here. Thanks for writing this.

[SimonSapin]

That is, moving these types to core would make core platform dependent, but as I understood it, core should be platform independent.

This is debatable, but let’s not open this subtopic in this thread since it only applies to integer C types, not c_void. This RFC does not propose anything for types other than c_void, specifically to avoid this question.

The unresolved questions about moving ctypes in std::os to std::ffi or similar, I'd just leave them open as to be resolved by some other RFC.

I agree, I’ve tweaked that paragraph to explicitly propose that.

[ExpHP]

trait VoidPointerExt {…}
impl VoidPointerExt for *mut std::os::raw::c_void {…}
impl VoidPointerExt for *mut libc::c_void {…}

With the two c_void types being unified, the two impls would overlap and fail to compile.
Hopefully such breakage is rare enough that we can manage it.

Afaict, this is the only way currently to make a function accept either type, so I doubt it is uncommon.

I presume that this is intended to be released as a minor version bump to libc... is there a migration plan for this? Some intermediate way to rewrite such code which is compatible with both old and new libc?

I doubt the semver trick can be used for something this big!

[SimonSapin]

Is making a function accept either type useful? To the point that it’s worth going through the trouble of defining a public trait in your API? Compared to adding as _ at some of the call sites.

The libc crate is currently in version 0.2.x. I’m hoping we can get away with implementing this RFC in another 0.2.y version and won’t have to make a version that Cargo considers incompatible such as 0.3.0 or 1.0.0. In this case, you can have a dependency like libc = "0.2.45" and there won’t be a need to support older 0.2.x libc.

Moving the ecosystem from 0.1 to 0.2 was a huge pain, I’m not proposing we go through that again.

If you want to keep supporting older Rust versions and accept both types there, then I suppose you’d need a build script like the one proposed in the RFC for lib.

[gnzlbg]

Is making a function accept either type useful?

That idiom is "fair" and works correctly, so I don't think the answer to this question matters at all.

This libc change can and will break crates that are not broken (this change is not a bug fix). So for me the question that matters is: How many crates does it break?

If it breaks hundreds of crates, we can't do this without doing a libc 0.3 release. If it breaks < 10 crates, I'd be ok with it, but others might not.

Anyways, this is a libc issue. And I don't think we can know the answer to these questions without doing a crater run. And even if we cannot do this change in libc 0.2, this change is still worth doing in core, and we might some day do a libc 0.3 release, and we can put this in. So none of these issues should block this RFC.

[SimonSapin]

I think the question of whether it is useful matters in trying to guess how common it is. But Crater is probably a better way to settle that.

[Ericson2314]

Re extern type I'm concerned that we can break libc again but not core. If we do unstable DynSized, we can put ?DynSized on ptr::null and friends allowing them to be used with c_void. Hopefully that wouldn't be committing to too much.

[SimonSapin]

Even if we fix up ptr::null somehow, there’s still other generic code (including outside of the standard library) that will stop accepting c_void if we change it from Sized to !Sized.

[Ericson2314]

it's true. For libc that's just a matter of 0.3, for std::os::raw::c_void that's more worrisome. Maybe the std stuff should be deprecated, effectively solving the duplication problem, and then we do something about libc later?

The extern type change may be just a nicety, but it stops users from doing things they surely didn't mean to do, so I think it is worth it in the long run. Doing the deprecation now, and 0.3 later, avoids a double change / 0.4.

[joshlf]

Re: ptr::null: I know it doesn't affect anything in this RFC as discussed above, but I thought it was worth addressing, so I'm proposing adding T: ?Sized to null and null_mut.

[alexcrichton]

I was curious about this, and one way we could test this out would be to change libc today to reexport std's version of c_void. If we could do a crater run (or equivalent) for that then I think we could get a pretty good handle on the upcoming breakage, if any, to be expected from this change. @pietroalbini, do you know if this sort of crater run is possible (diffing two versions of a crate, not two versions of rustc)

[Mark-Simulacrum]

A crater run is possible, we recently did one for lazy-static. If someone could prepare a PR for libc with the required changes I can look into running crater on it.

[alexcrichton]

Oh awesome! How should libc be prepared? Would a git branch suffice? Or something on crates.io?

[Mark-Simulacrum]

Git branch is sufficient, I believe.

[alexcrichton]

Ok! I've uploaded a branch to https://github.com/alexcrichton/libc/tree/c-void

[Mark-Simulacrum]

Okay I think I've started a crater run in check-only mode, we'll see how it goes. Generally speaking should have results in ~24-36 hours.

[Mark-Simulacrum]

The crater run (which I think worked, but can't really be sure) seems to indicate 0 regressions (https://cargobomb-reports.s3.amazonaws.com/libc-1/index.html). Are we aware of any expected regressions (that is, to test if the crater run found them)?

[retep998]

As a point of data, winapi has an std feature flag to toggle between defining c_void itself and re-exporting c_void from std and so far I have not gotten any reports of issues due to that. At worst people are simply not aware of the std feature flag and complain about having to cast between std's RawHandle and winapi's HANDLE (both raw pointers to c_void).

[alexcrichton]

Awesome, thanks @Mark-Simulacrum!

@SimonSapin I think that's a pretty convincing data point that this is extremely unlikely to be a breaking change in practice, and that definitely makes me very confident in the proposed design here! (as a minor version bump of libc)

[alexcrichton]

This has sat for a bit now and has been debated quite a bit in the past. This has also been cc'd in a large number of locations for some good visibility, so that makes me comfortable to propose FCP here! I think that this is a solid step forward in the FFI story for the standard library and definitely one we should take. While we may want to be more ambitious in the future this is at least a good starting point.

@rfcbot fcp merge

[jethrogb]

I think it would be a shame if we stabilized c_void in its new location with a suboptimal type, but maintaining the std/libc incompatibility seems like a much bigger issue. This RFC seems good enough to me.

[Ericson2314]

We could use the same configure-script-style-check to just optionally depend on the std one, no new core one needed.

[retep998]

Since crater has shown that libc can easily switch to re-exporting c_void from std why can't we just make libc re-export c_void from std when the appropriate feature is enabled? That way we get a unified c_void while being able to punt entirely on the issue of a suboptimal c_void in core.

[SimonSapin]

@jethrogb I think “has been blocked” is one interpretation of those discussions. Some have expressed a preference for changing the definition of c_void to a new kind of type whose layout is truely unknown, instead of the current two-variant enum. This kind of type has since been implemented in Nightly as e.g. extern { type c_void; }. However it’s not clear that people realized at the time that this would be a breaking change: extern types are !Sized and std::ptr::null::<T>() requires T: Sized.

As to #1783, it proposed moving all c_* types, and as mentioned in the RFC there were concerns with having operating-system-dependant APIs in libcore (which this RFC doesn’t propose), or with having a whole new crate for such a small API.

I think it would be a shame if we stabilized c_void in its new location with a suboptimal type,

Note that std::os::raw::c_void is already stable, all that this RFC is proposing is making it also available at a new location.

If/when we add a new better improved equivalent to C’s void*, regardless of its location I think it would be better to give it a name other than c_void to avoid confusion, so this RFC is also not preventing that.

[alexcrichton]

@retep998 as discussed before it's a breaking change to do that due to libc having a feature to enable std. That means that if the use_std feature is disabled and a crate tries to pass a c_void to something that takes a std::os::raw::c_void it will get a type error.

The purpose of this RFC is to solve the problem once and for all with c_void rather than applying continual band-aids. With only one definition of c_void and everyone reexporting it then it should be impossible to have conflicting types.

[Ericson2314]

@alexcrichton no it could be done with some build.rs hack unconditionally as this PR does it with core.

The actual "once and for all" solution would be making it an extern type once the temporary issues blocking use of these with ptr::null are worked out. I'd guess that would be within 2 years. Can we really do something else just until then??

[alexcrichton]

@Ericson2314 what @retep998 is proposing is not changing core at all, meaning that when libc is in #![no_std] mode it has nothing to reexport, forcing it to define its own version. That then clashes with std's version, meaning there's two types.

[retep998]

But why would someone use libc in #![no_std] mode with std at the same time? Surely they'd enable the std feature of the libc crate if they want libc::c_void to be compatible with std::os::raw::c_void?

[gnzlbg]

But why would someone use libc in #![no_std] mode with std at the same time? Surely they'd enable the std feature of the libc crate if they want libc::c_void to be compatible with std::os::raw::c_void?

@retep998 a no_std crate A can depend on libc, and some other crate B that requires std can depend on crate A.

If crate A does not re-export libc's use_std feature, then you end up there.

Why would a no_std crate depend on libc? If you just want to use mmap directly to implement an allocator (not necessarily a GlobalAlloc or Alloc), you might want to depend on libc, but you might not want to depend on liballoc or libstd.

[retep998]

@gnzlbg In the case where crate B is using c_void from crate A and expects it to be compatible with c_void from std and crate A refuses to provide any way to enable the use_std feature of libc, then crate B can depend on libc itself just so it can enable the use_std feature. As far as I know, there is no way for a crate to be unable to tell libc to enable the use_std feature when it needs c_void compatibility.

[Ericson2314]

@gnzlbg This, incidentally, is a great example of why we have std features rather than no-std features.

[Ericson2314]

@alexcrichton Also, for what it's worth, I hope in those same ~2 years (while extern type incubates) for stdlib-aware Cargo to be prioritized so we can drop all builds.rs hacks and have std and user code depend on the same libc, with its c_void. Ideal semantics DynSized + Referent<Meta=()> (or whatever they end up being called) + ideal dependency order is my goal.

[SimonSapin]

@Ericson2314 this is getting off-topic but, as we’ve discussed multiple times already, even with std-aware Cargo the libc crate used by std would still be a separate crate from libc from crates.io that everyone can use in their [dependencies], and the former copy of libc should still not be exposed to users.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

[Centril]

Huzzah! This RFC has been merged!

Tracking issue: rust-lang/rust#53856

[Ericson2314]

Uh, don't the final comments need to wind down or something? This discussion seems ongoing.

[Ericson2314]

@SimonSapin Agreed this getting off topic, but as long as the portion of libc which the standard library uses never changes, it's fine that std would have a public dependency on libc. In practice, this is a good argument for a "libctypes": std has a private dependency on libc, but a public dependency on libctypes, and libctypes will be tiny and never need a breaking change of any sort.

[Centril]

@Ericson2314 I sometimes wait with merging RFCs when new arguments are made that haven't been addressed during FCP; but it seemed to me that the libs team was aware of the new concerns and made different trade-offs.

I recommend that you continue this conversation on the tracking issue over at rust-lang/rust#53856. If new significant concerns arise in the tracking issue, then the libs team can still decide to not go through with the RFC as planned.

[Ericson2314]

I commented there.

On a meta level, I still feel like it's constantly an uphill battle to maintain the principle of small libraries with well-delineated purposes, as all mixes of prioritization and rushes (the former which I am much more sympathetic with) instead lead to things constantly being dumped in core just out of convenience.
If we could have some sort of stay on things that related them, or prioritize them because of the things that relate to them, that would be a huge relief.

But even if that flies, I worry that by the time things like alloc polymorphic and std-aware Cargo get to the front of the queue, it will be death by a thousand cuts over the accumulated decisions made without really considering possibilities. Hopefully its just a few more months until the 2018 edition is up, and then those get to the front of the queue, and not too much gets in the way between now and then. If not, I'm already at the point where I feel the std/core division is irreparable, core should become a std reexport, and a new facade over all-new crates below std/core. That could bail us out.

[alexcrichton]

@Ericson2314 keep in mind that these sorts of decisions are always tradeoffs. We live in the real world where idealistic designs can't appear overnight. Compromises need be made to solve real problems today that also leave room to solutions in the future. The c_void problem has nearly been debated to death, and nothing new that hasn't already been discussed was brought up during FCP.

The libs team of course doesn't just blindly prioritize havinvg items "dumped in core". There are pros and cons that we weigh when deciding on RFCs like this. These pros and cons have been weighed many many times and this is what we've resulted with.

[faern]

When we soon have c_void in core it will be available to everyone, even #[no_std] crates that don't even depend on libc. As such I guess we could remove c_void from libc when we do the breaking libc bump discussed in rust-lang/libc#547? Having it defined in fewer places should reduce confusion and make code more consistent.

[madsmtm]

Just to let you know, in case you would like to raise objections or provide input, the rest of the C types in std::os::raw has recently been moved to core::ffi under an unstable feature flag, tracking issue here: rust-lang/rust#94501