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

Fix some raw types in std to match what's in libc #43499

Closed
wants to merge 1 commit into from

Conversation

dhduvall
Copy link

The pthread_t type, in particular, causes failures when building cargo 0.20.0 after rust-lang/libc@8304e06b5.

The pthread_t type, in particular, causes failures when building cargo
0.20.0 after changeset 8304e06 in the libc repo.
@cuviper
Copy link
Member

cuviper commented Jul 27, 2017

Most of those raw types are deliberately wrong per #31551.

I think you can still make a case for pthread_t on its own though.

@binarycrusader
Copy link
Contributor

@cuviper can you expound a bit on the "deliberately wrong" part? There's not enough reasoning in #31551 to explain why. Should rust itself no longer be using these at all? pthread_t is certainly used at the least which is causing correctness issues. The combination of them being simultaneously deprecated and yet still in use is confusing.

@sfackler
Copy link
Member

Those type definitions are all stable and can't be changed.

@binarycrusader
Copy link
Contributor

binarycrusader commented Jul 27, 2017

They were never correct to begin with and this only affects Solaris. Surely they can be fixed since they're causing build problems for cargo.

@malbarbo
Copy link
Contributor

@binarycrusader see the comments in #41752.

@dhduvall
Copy link
Author

This is the error that prompted this fix:

error[E0308]: mismatched types
   --> src/lib.rs:587:40
    |
587 |                     libc::pthread_kill(self.thread.as_pthread_t(), libc::SIGUSR1);
    |                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected u32, found usize
    |
    = help: here are some functions which might fulfill your needs:
            - .count_ones()
            - .count_zeros()
            - .leading_zeros()
            - .trailing_zeros()

error: aborting due to previous error

error: Could not compile `jobserver`.

As best I can tell, there's no issue in the jobserver code itself. The problem is a mismatch between libc (pthread_kill(), which takes libc's notion of pthread_t, which is c_uint/u32) and std (as_pthread_t(), which returns a RawPthread, which is std's notion of pthread_t, which is usize). I don't think there's any change to the jobserver code that can fix that error.

I could simply fix pthread_t, as @cuviper suggested, and that's certainly enough to solve that particular problem, but I'd like to know why that would be okay, but fixing the others is not. I'd also be perfectly okay with a solution that involved a fix somewhere else, but I'm not sure what that would be, other than defining RawPthread to be something other than std::os::unix::raw::pthread_t:

#[allow(deprecated)]
use os::unix::raw::pthread_t;

#[allow(deprecated)]
pub type RawPthread = pthread_t;

That use of a type from the raw module seems pretty rare (if not unique?), so maybe the thing to do is simply to fix that, but I'm not sure what knock-on effects that would have, so making changes to Solaris-only code seemed safest, since it's likely that this code simply never worked.

@alexcrichton
Copy link
Member

I'd personally considered this OK to land in the sense that solaris is a pretty "low" tier in terms of guarantees right now. My guess is that this'll only fix code rather than break it, so I'd be ok landing.

@sfackler and @cuviper, do y'all disagree though?

@alexcrichton alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 27, 2017
@alexcrichton alexcrichton self-assigned this Jul 27, 2017
@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jul 27, 2017
@cuviper
Copy link
Member

cuviper commented Jul 27, 2017

The filesystem-related types are deliberately wrong mainly because LFS is messy. At least on Linux, there's just not a good way to have types that are truly FFI compatible. So the decision was widen all the MetadataExt return values to 64-bit, except where fewer bits will definitely suffice, and to widen the raw types in kind. There's more discussion in RFC 1451 and its pull request.

But I don't feel too strongly about it, and I don't know the LFS story on Solaris.

Fixing pthread_t seems fine though. It did match libc::pthread_t before the commit linked in the OP, and it makes sense to me that std should get the same fix.

@sfackler
Copy link
Member

My recollection is that we deprecated these types for exactly this reason - we couldn't change them since they're stable. Is this an exception because Solaris isn't a tier 1 platform?

@cuviper
Copy link
Member

cuviper commented Jul 27, 2017

@sfackler #31551 did change those types while deprecating them.

@alexcrichton
Copy link
Member

Ah yeah it's true, I'd mostly be worried about just fixing pthread_t which isn't deprecated and could be a problem, but I'd be fine if we backed out the other changes due to LFS business (and those types should be practically deprecated regardless)

@carols10cents carols10cents added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 31, 2017
@alexcrichton
Copy link
Member

@dhduvall would you like to change this to just fixing pthread_t? After that I think we're good to go?

@dhduvall
Copy link
Author

dhduvall commented Aug 1, 2017

My git-fu is sad. I ended up opening #43597.

@dhduvall dhduvall closed this Aug 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants