-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Correct the definition of pthread_t
on Darwin
#4336
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?
Conversation
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified to cfg(target_vendor = "apple")
} | ||
} | ||
} else { | ||
pub type pthread_t = crate::uintptr_t; |
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.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Description
This PR Fixes #2903, however I am not sure if the change to
build.rs
is the right way to do it. MacOS has the_opaque_pthread_t
type, however inbuild.rs
it gets classified as atype
rather than a struct, resulting in the test case for it using_opaque_pthread_t
rather thanstruct _opaque_pthread_t
. Please let me know if there is a better way to test_opaque_pthread_t
without 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/semver
have been updated*LAST
or*MAX
areincluded (see #3131)
cd libc-test && cargo test --target mytarget
);especially relevant for platforms that may not be checked in CI