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

Incorrect pthread_t definition on Darwin (should be a raw pointer, not an uintptr_t) #2903

Open
Tracked by #3248
thomcc opened this issue Sep 6, 2022 · 1 comment
Labels
breakage-candidate E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Milestone

Comments

@thomcc
Copy link
Member

thomcc commented Sep 6, 2022

This is a bug, but not one we can probably fix without breaking too much (it would break std's reexport of pthread_t). Aside from breaking code that treats it directly as an integer, it would mean pthread_t would suddenly become !Send and !Sync, and I wouldn't be surprised if that's a problem for some.

Anyway, on Darwin-based OSes, pthread_t should be a pointer, but is instead an integer. I noticed this when trying to pass ptr::null_mut() into a function that Apple documents as having special behavior when passed a NULL for pthread_t.

However, it's worth noting that the structure it points to is not opaque (at least not fully), and low level code on Darwin could potentially need to access it (at the moment this is likely not possible to do correctly on stable Rust, due to the lack of stable "C-unwind"). Specifically it has a list of functions that may be manipulated by code that wants to have cleanup functions run on thread cancellation. This list is directly manipulated by the pthread_cleanup_push and pthread_cleanup_pop function-style C macros, but this would be done directly by Rust code, which cannot use such macros. This is pretty niche, but is a case that's slightly relevant to rust-lang/rust#95496.

The correct definition on Darwin would look more like:

s! {
    pub struct __darwin_pthread_handler_rec {
        pub __routine: Option<unsafe extern "C" fn(arg1: *mut ::c_void)>,
        pub __arg: *mut ::c_void,
        pub __next: *mut __darwin_pthread_handler_rec,
    }
    pub struct _opaque_pthread_t {
        pub __sig: ::c_long,
        pub __cleanup_stack: *mut __darwin_pthread_handler_rec,
        pub __opaque: [::c_char; __PTHREAD_SIZE__],
    }
}

pub type __darwin_pthread_t = *mut _opaque_pthread_t;

pub type pthread_t = __darwin_pthread_t;

I suppose it's fine for code that cares about this to maintain its own bindings, though.

@workingjubilee
Copy link
Member

Hmm. Making this code incompatible in a new version of libc is actually probably fine? This was part of the reason that these modules were deprecated and the libc crate was made to replace it. Old code that uses a prior version of libc will not be broken if we do this on the 0.3 bump. It will become incompatible with the deprecated std::os::raw definition, but... what else is new? That's been a problem for all FFI typedefs, and I don't believe std's ancient mistake is warranted as a blocking reason for fixing the bindings in the crate that was specifically made so that Rust could fix those bindings going forward.

@tgross35 tgross35 added this to the 1.0 milestone Aug 12, 2024
@tgross35 tgross35 added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. and removed help wanted labels Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breakage-candidate E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

No branches or pull requests

3 participants