Correct the definition of pthread_t on Darwin#4989
Conversation
|
Hi @tgross35, Could you help me understand why |
|
Hi @chengr4, thanks for finishing this! I really wanted to do this myself but I got caught up in a bunch of work for my university so I ended up forgetting about this. One thing to note, since now Also make sure to run the style checker in |
9fade92 to
d79f324
Compare
d79f324 to
37afe49
Compare
|
@highjeans Thank for answering; I totally forgot 🫠. Again, Thanks for leaving the PR for me to study this crate. Actually, I failed bad `ELAST` value at byte 0: rust: 106 (0x6a) != c 107 (0x6b)
rust bytes: 6a 00 00 00
c bytes: 6b 00 00 00
bad `HOST_VM_INFO64_COUNT` value at byte 0: rust: 38 (0x26) != c 40 (0x28)
rust bytes: 26 00 00 00
c bytes: 28 00 00 00
bad `vm_statistics64_data_t` size: rust: 152 != c 160
bad `vm_statistics64` size: rust: 152 != c 160
size of `vm_statistics64_data_t` is 160 in C and 152 in Rust
size of `struct vm_statistics64` is 160 in C and 152 in RustCan you please guide me how to evaluate and fix this bug? |
pthread_t on Darwin
|
@chengr4 because your system has different version(s) than CI, I think. It's fine as long as CI passes (unless it's unspported). |
|
ok will do it after 6/12 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
8924db4 to
6b7c9b6
Compare
This comment has been minimized.
This comment has been minimized.
6b7c9b6 to
3f7fcfb
Compare
|
@JohnTitor if commit mesage is not clear enough, help me fix it. Thanks |
There was a problem hiding this comment.
Regarding the commit: whoever submits the final patch should be listed as the author, which in this case is you. You can change the author field with git commit --amend --reset-author.
However, of course you also need to give credit to anyone who contributed to the patch, which you can add with:
Co-authored-by: highjeans <foo@bar.com>
at the end of the commit (get their email from the commit log before you reset it)
| extern_ty! { | ||
| pub enum _opaque_pthread_t {} | ||
| } |
There was a problem hiding this comment.
This macro's syntax changed recently, you'll need to rebase then this can become pub type _opaque_pthread_t;.
However, could you try making it non-pub? Since we don't care about the internals. You may need a #[allow(private_interfaces)] on pthread_t.
There was a problem hiding this comment.
I tried making _opaque_pthread_t non-pub, but it caused numerous compiler errors and warnings regarding private_interfaces
E.g.
error: type `_opaque_pthread_t` is private
--> src/new/apple/libpthread/pthread_/introspection.rs:27:17
|
27 | thread: pthread_t,
| ^^^^^^^^^ private type
error: type `_opaque_pthread_t` is private
--> src/new/apple/libpthread/pthread_/introspection.rs:33:17
|
33 | thread: pthread_t,
| ^^^^^^^^^ private type
error: type `_opaque_pthread_t` is private
--> src/new/apple/libpthread/pthread_/pthread_spis.rs:9:17
|
9 | thread: *mut crate::pthread_t,
| ^^^^^^^^^^^^^^^^^^^^^ private type| // FIXME(1.0): verify other BSDs? Glancing around, other BSDs also look like a pointer | ||
| pub type pthread_t = crate::uintptr_t; |
There was a problem hiding this comment.
Feel free to change these here since you've already done the work
There was a problem hiding this comment.
I can take it over after the apples look good to you.
|
Reminder, once the PR becomes ready for a review, use |
3f7fcfb to
2e0023b
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
- Including: Update symbols list
- References for verifying other BSDs
- https://github.com/freebsd/freebsd-src/blob/5bffa1d2069a05c8346eb34e17a39085fe0bf09b/include/signal.h#L68
- https://github.com/openbsd/src/blob/ac71a6695016645a6726c964da2a0f509d63c2c8/include/pthread.h#L112
- Thank @highjeans for starting this work.
Co-authored-by: highjeans <vivan.waghela23@gmail.com>
2e0023b to
dd997ea
Compare
Description
This PR fixes #2903. This PR picks up the work started by @highjeans in #4336 (close #4336) which had stalled. I just followed up the comments from @tgross35 in the PR and tried to finish it.
Closes: #4336
Sources
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