Correct the definition of pthread_t on Darwin#4336
Correct the definition of pthread_t on Darwin#4336highjeans wants to merge 5 commits intorust-lang:mainfrom
pthread_t on Darwin#4336Conversation
tgross35
left a comment
tgross35
left a comment
There was a problem hiding this comment.
Sorry I never looked at this! Looks pretty good to me, just two small requests.
| if #[cfg(any( | ||
| target_os = "macos", | ||
| target_os = "ios", | ||
| target_os = "tvos", | ||
| target_os = "watchos", | ||
| target_os = "visionos", | ||
| ))] { |
There was a problem hiding this comment.
This can be simplified to cfg(target_vendor = "apple")
| } | ||
| } | ||
| } else { | ||
| pub type pthread_t = crate::uintptr_t; |
There was a problem hiding this comment.
Could you add a comment // FIXME(1.0): verify other BSDs? Glancing around, other BSDs also look like a pointer https://github.com/freebsd/freebsd-src/blob/5bffa1d2069a05c8346eb34e17a39085fe0bf09b/include/signal.h#L68 https://github.com/openbsd/src/blob/ac71a6695016645a6726c964da2a0f509d63c2c8/include/pthread.h#L112
tgross35
changed the title
pthread_t definition on Darwin (should be a raw pointer, not an uintptr_t)pthread_t on Darwin
| pub struct __darwin_pthread_handler_rec { | ||
| __routine: Option<unsafe extern "C" fn(*mut c_void)>, | ||
| __arg: *mut c_void, | ||
| __next: *mut __darwin_pthread_handler_rec, | ||
| } | ||
| pub struct _opaque_pthread_t { | ||
| __sig: c_long, | ||
| __cleanup_stack: *mut __darwin_pthread_handler_rec, | ||
| __opaque: [c_char; __PTHREAD_SIZE__], | ||
| } |
There was a problem hiding this comment.
One other thing, could you mark these #[non_exhaustive]? Doesn't really matter here since they are all private fields, but we are starting to do that for everything new
| cfg_if! { | ||
| if #[cfg(any( | ||
| target_os = "macos", | ||
| target_os = "ios", | ||
| target_os = "tvos", | ||
| target_os = "watchos", | ||
| target_os = "visionos", | ||
| ))] { | ||
| pub type __darwin_pthread_t = *mut _opaque_pthread_t; | ||
| pub type pthread_t = __darwin_pthread_t; | ||
| s! { | ||
| pub struct __darwin_pthread_handler_rec { | ||
| __routine: Option<unsafe extern "C" fn(*mut c_void)>, | ||
| __arg: *mut c_void, | ||
| __next: *mut __darwin_pthread_handler_rec, | ||
| } | ||
| pub struct _opaque_pthread_t { | ||
| __sig: c_long, | ||
| __cleanup_stack: *mut __darwin_pthread_handler_rec, | ||
| __opaque: [c_char; __PTHREAD_SIZE__], | ||
| } | ||
| } | ||
| } else { | ||
| pub type pthread_t = crate::uintptr_t; | ||
| } | ||
| } |
There was a problem hiding this comment.
Circling back, I think this could be simpler. Could you: instead update the definition to this?
extern_ty! {
enum _opaque_pthread_t {}
}
type __darwin_pthread_t = *mut _opaque_pthread_t;
pub type pthread_t = __darwin_pthread_t;We don't actually need the internal details here.
|
Hey @highjeans, I'm interested in finishing this up. I've opened a new PR here: #4989. |
4 participants
Description
This PR Fixes #2903, however I am not sure if the change to
build.rsis the right way to do it. MacOS has the_opaque_pthread_ttype, however inbuild.rsit gets classified as atyperather than a struct, resulting in the test case for it using_opaque_pthread_trather thanstruct _opaque_pthread_t. Please let me know if there is a better way to test_opaque_pthread_twithout having to modify the build.rs file.Sources
https://github.com/apple-oss-distributions/xnu/blob/8d741a5de7ff4191bf97d57b9f54c2f6d4a15585/EXTERNAL_HEADERS/sys/_pthread/_pthread_types.h#L103
https://github.com/apple-oss-distributions/libpthread/blob/main/include/sys/_pthread/_pthread_t.h#L31
Checklist
libc-test/semverhave been updated*LASTor*MAXareincluded (see #3131)
cd libc-test && cargo test --target mytarget);especially relevant for platforms that may not be checked in CI