-
-
Notifications
You must be signed in to change notification settings - Fork 15.2k
Redefine c_void
#159935
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
base: main
Are you sure you want to change the base?
Redefine c_void
#159935
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,10 @@ | |
| #![stable(feature = "core_ffi", since = "1.30.0")] | ||
| #![allow(non_camel_case_types)] | ||
|
|
||
| use crate::fmt; | ||
| use crate::panic::RefUnwindSafe; | ||
| #[stable(feature = "c_str_module", since = "1.88.0")] | ||
| pub mod c_str; | ||
| #[doc(inline)] | ||
| #[stable(feature = "core_c_str", since = "1.64.0")] | ||
| pub use self::c_str::CStr; | ||
|
|
@@ -18,14 +22,6 @@ pub use self::c_str::FromBytesUntilNulError; | |
| #[doc(inline)] | ||
| #[stable(feature = "core_c_str", since = "1.64.0")] | ||
| pub use self::c_str::FromBytesWithNulError; | ||
| use crate::fmt; | ||
|
|
||
| #[stable(feature = "c_str_module", since = "1.88.0")] | ||
| pub mod c_str; | ||
|
|
||
| mod va_list; | ||
| #[stable(feature = "c_variadic", since = "CURRENT_RUSTC_VERSION")] | ||
| pub use self::va_list::{VaArgSafe, VaList}; | ||
|
|
||
| mod primitives; | ||
| #[stable(feature = "core_ffi_c", since = "1.64.0")] | ||
|
|
@@ -36,35 +32,38 @@ pub use self::primitives::{ | |
| #[unstable(feature = "c_size_t", issue = "88345")] | ||
| pub use self::primitives::{c_ptrdiff_t, c_size_t, c_ssize_t}; | ||
|
|
||
| // N.B., for LLVM to recognize the void pointer type and by extension | ||
| // functions like malloc(), we need to have it represented as i8* in | ||
| // LLVM bitcode. The enum used here ensures this and prevents misuse | ||
| // of the "raw" type by only having private variants. We need two | ||
| // variants, because the compiler complains about the repr attribute | ||
| // otherwise and we need at least one variant as otherwise the enum | ||
| // would be uninhabited and at least dereferencing such pointers would | ||
| // be UB. | ||
| mod va_list; | ||
| #[stable(feature = "c_variadic", since = "CURRENT_RUSTC_VERSION")] | ||
| pub use self::va_list::{VaArgSafe, VaList}; | ||
|
|
||
| #[doc = include_str!("c_void.md")] | ||
| #[lang = "c_void"] | ||
| #[repr(u8)] | ||
| #[repr(transparent)] | ||
| #[stable(feature = "core_c_void", since = "1.30.0")] | ||
| pub enum c_void { | ||
| #[unstable( | ||
| feature = "c_void_variant", | ||
| reason = "temporary implementation detail", | ||
| issue = "none" | ||
| )] | ||
| #[doc(hidden)] | ||
| __variant1, | ||
| #[unstable( | ||
| feature = "c_void_variant", | ||
| reason = "temporary implementation detail", | ||
| issue = "none" | ||
| )] | ||
| #[doc(hidden)] | ||
| __variant2, | ||
| pub struct c_void { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mejrs makes a good point on zulip
Let's add a test using this pattern so that we catch it when/if this changes. (Even if we don't make this change, we should add the test.)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you are doing this, that means you are matching on a value or reference to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not necessarily. The code might just be dead code. |
||
| // Using this weird type ensures a size of 1, | ||
| // while minimizing UB if a user incorrectly tries | ||
| // to dereference a pointer to `c_void`, | ||
| // or reborrow it as a reference. | ||
| #[cfg(not(miri))] | ||
| _inner: crate::pin::UnsafePinned<crate::mem::MaybeUninit<u8>>, | ||
|
|
||
| // However, if running in Miri, | ||
| // we want to maximize detection of UB, | ||
| // so we make `c_void` uninhabited. | ||
|
Comment on lines
+50
to
+52
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is worth the risk of divergence from the behavior during regular compilation.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Compare the risks of not doing this. Either we:
I think the theoretical downside of us maybe having overlooked some way in which the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Miri is not a general tool to detect misuse. It is a very specific tool to detect language UB. Whatever misuse they do is apparently not language UB, so Miri is the wrong tool to find it. I am fine with having extra checks under I understand your arguments in favor of this change, but I don't think they are convincing enough. There are way too many things that happen inside the compiler that may go different for those different type definitions, that's just not a game I am willing to play. That's still a clear "thanks but no" from my side for any
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Plenty is! And plenty isn't only because of unspecified details of the current layout. If you don't like the approach this PR takes, which of the alternatives I listed would you prefer?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's up to libs or libs-api to decide, I don't have a strong opinion beyond "Miri should check the actual code we usually run".
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It needs to mock the C side, but the Rust side should be faithful.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How would you feel about making the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's still far from identical as such a field makes quite the difference for layout computation. Might might do ill-advised nonsense like
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And we can't just make it straight-up uninhabited always, right? Because that breaks other stuff/makes it incorrect? (I haven't thought about this long enough to remember the answer, it's out of cache apparently.)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because we don't want to actually start triggering nasal demons in the (many) crates that incorrectly do this, but by chance narrowly avoid UB, or trigger only "benign" UB |
||
| #[cfg(miri)] | ||
| _inner: u8, | ||
| #[cfg(miri)] | ||
| _uninhabited: !, | ||
| // Ensure no `Freeze`, for consistency with `not(miri)` | ||
| #[cfg(miri)] | ||
| _nonfreeze: crate::cell::SyncUnsafeCell<()>, | ||
| } | ||
|
|
||
| // for backward compatibility. | ||
| #[stable(feature = "core_c_void", since = "1.30.0")] | ||
| impl RefUnwindSafe for c_void {} | ||
|
Comment on lines
+57
to
+59
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR removes the following trait implementations:
One of these is a stable trait, so this is a breaking change. Should we also implement
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that was an oversight, good catch |
||
|
|
||
| #[stable(feature = "std_debug", since = "1.16.0")] | ||
| impl fmt::Debug for c_void { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
|
|
||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| use core::ffi::c_void; | ||
|
|
||
| fn main() { | ||
| let mut b: u8 = 0; | ||
| let p: *const c_void = (&raw mut b).cast_const().cast(); | ||
| let _r: &c_void = unsafe { &*p }; //~ERROR: constructing invalid value | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| error: Undefined Behavior: constructing invalid value of type &std::ffi::c_void: encountered a reference pointing to uninhabited type `std::ffi::c_void` | ||
| --> tests/fail/validity/c-void-validity.rs:LL:CC | ||
| | | ||
| LL | let _r: &c_void = unsafe { &*p }; | ||
| | ^^^ Undefined Behavior occurred here | ||
| | | ||
| = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior | ||
| = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information | ||
|
|
||
| note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace | ||
|
|
||
| error: aborting due to 1 previous error | ||
|
|
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We auto-generate this file with |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the doc path for this type from
core/ffi/enum.c_void.htmltocore/ffi/struct.c_void.html. If we make this change, we should introduce a redirect to keep links to the old path working.View changes since the review