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

Add pthread_self #591

Merged
merged 1 commit into from
Jun 5, 2017
Merged

Add pthread_self #591

merged 1 commit into from
Jun 5, 2017

Conversation

king6cong
Copy link
Contributor

No description provided.

@Susurrus
Copy link
Contributor

Thanks, @king6cong!

However I don't believe this is the right approach to this. If we can't support gettid directly on Mac, I don't believe we should emulate it for the sole reason of abstracting away platform differences. Nix only attempts to be a very small shim layer over unsafe libc interfaces. Any higher-level abstractions like this should be left to downstream crates. So instead of doing this, can you implement pthread_self() for all suitable platforms. Since that works on Linux and Mac someone could use that instead for cross-platform support.

@king6cong
Copy link
Contributor Author

So instead of doing this, can you implement pthread_self() for all suitable platforms. Since that works on Linux and Mac someone could use that instead for cross-platform support.

To confirm , you mean that we should keep pthread_self out of nix or take it in in another form? 😃

@Susurrus
Copy link
Contributor

I think we should provide a safe wrapper about pthread_self instead of pretending that pthread_self is gettid on Mac. It should be also noted that according to the man page:

The thread ID returned by pthread_self() is not the same thing as the kernel thread ID returned by a call to gettid(2)

Right now there's a bunch of pthread stuff is in src/sys/signal, but I'm not sure if this should be put there. I think putting it in a new src/sys/pthread.rs file is the correct way to do this. There's also previous conversation about this here.

@king6cong
Copy link
Contributor Author

@Susurrus updated

@Susurrus
Copy link
Contributor

That looks like a step in the right direction, but tests are failing.

@Susurrus
Copy link
Contributor

@king6cong Can you also add an entry to the CHANGELOG for this?

@king6cong
Copy link
Contributor Author

@Susurrus Yeah, haven't found the reason that tests fails, maybe tweak around ci setup to show failure details first.

@Susurrus
Copy link
Contributor

Do the tests pass locally for you? If you have access to a mac and can test on one too, that might point us in the right direction.

@king6cong
Copy link
Contributor Author

@Susurrus Yeah, it occurred to me that I could use my x86 mac to test i686 failure. Updated, Thanks!

Copy link
Contributor

@Susurrus Susurrus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor issues, mostly the unix thing needs to get sorted out.

CHANGELOG.md Outdated
@@ -6,6 +6,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
## [Unreleased]

### Added
- Added `nix::sys::pthread::test_pthread_self`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should show nix::sys::pthread::pthread_self instead.

src/sys/mod.rs Outdated
@@ -84,3 +84,6 @@ pub mod statfs;
target_arch = "arm")),
)]
pub mod statvfs;

#[cfg(unix)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't all the platforms covered by nix unix? This seems unnecessary.

test/sys/mod.rs Outdated
@@ -12,3 +12,5 @@ mod test_uio;

#[cfg(target_os = "linux")]
mod test_epoll;
#[cfg(unix)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, aren't all nix-supported platforms unix?

@@ -0,0 +1,6 @@
use nix::sys::pthread::*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an empty line below this one.

@@ -0,0 +1,12 @@
use libc::{self, pthread_t};


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete one of these blank lines.

@Susurrus
Copy link
Contributor

@king6cong I can't tell because the old code is gone, but what was the cause of the failures and what did you change to fix them?

@king6cong
Copy link
Contributor Author

king6cong commented Apr 21, 2017

@Susurrus Updated. libc::pthread_self() should return pthread_t instead of pid_t, that's the reason that tests on several platforms failed

@Susurrus Susurrus changed the title support gettid on macos Add pthread_self Apr 21, 2017
@Susurrus
Copy link
Contributor

Great, thanks for clearing that up. Makes sense why it fixed things.

I wanted to ask, do you have a specific use case for this that this addition is all that's required for it to work? I'm going to block this on #590, so we can be sure that all platforms are passing before adding this, so we have a little time here in case there was any missing functionality you also needed. and wanted to add from pthread.h.

@Susurrus
Copy link
Contributor

Susurrus commented Jun 3, 2017

Can you make a new type Pthread, export it from nix, and also use that as the return value for pthread_self? I'd like nix to have all of its datatypes self-contained for better ergonomics for users.

@king6cong
Copy link
Contributor Author

Updated

@Susurrus
Copy link
Contributor

Susurrus commented Jun 4, 2017

This looks good. We're going to wait until the new FreeBSD buildbot is integrated (#618) and this will be our first test.

@asomers
Copy link
Member

asomers commented Jun 5, 2017

Slight change of plans. We're no longer waiting for #618 .
bors r+

bors bot added a commit that referenced this pull request Jun 5, 2017
591: Add pthread_self r=asomers
@bors
Copy link
Contributor

bors bot commented Jun 5, 2017

Build succeeded

@bors bors bot merged commit 445c247 into nix-rust:master Jun 5, 2017
@Susurrus
Copy link
Contributor

Susurrus commented Jun 5, 2017

@King6Kong Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants