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 assertion in Mutex::destroy() on DragonFly (#20698) #20741

Merged
merged 1 commit into from Jan 9, 2015
Merged

Fix assertion in Mutex::destroy() on DragonFly (#20698) #20741

merged 1 commit into from Jan 9, 2015

Conversation

mneumann
Copy link
Contributor

@mneumann mneumann commented Jan 8, 2015

On DragonFly pthread_mutex_destroy() returns EINVAL (22) if called on a
pthread_mutex_t that was just initialized via PTHREAD_MUTEX_INITIALIZER
and not used otherwise or initialized via pthread_mutex_init(). Change
the code to treat a return value of EINVAL on DragonFly as success, too.

This fixes issue #20698.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

// on a mutex that was just initialized with
// ffi::PTHREAD_MUTEX_INITIALIZER. Once it is used (locked/unlocked)
// or pthread_mutex_init() is called, this behaviour no longer occurs.
debug_assert!(r == 0 || r == 22);
Copy link
Member

Choose a reason for hiding this comment

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

Is there an EINVAL const somewhere in liblibc you could use instead of a raw 22?

@mneumann
Copy link
Contributor Author

mneumann commented Jan 8, 2015

@sfackler: I will look out for EINVAL and try to use it.

There are some more similar patches to come for condvar.rs and rwlock.rs. Cross-compiling to DragonFly is slow and I thought this would fix all my issues...

On DragonFly pthread_{mutex,rwlock,condvar}_destroy() returns EINVAL
when called on a pthread_{mutex,rwlock,condvar}_t that was just
initialized via PTHREAD_{MUTEX,RWLOCK,CONDVAR}_INITIALIZER and not used
in the meantime or initialized via pthread_{mutex,rwlock,condvar}_init().
Change the code to treat a return value of EINVAL on DragonFly as success.
@mneumann
Copy link
Contributor Author

mneumann commented Jan 8, 2015

Ok, now I also fixed condvar and rwlock, now it's building a working Rust compiler for DragonFly!!!

@alexcrichton
Copy link
Member

I do think that this may actually be a bug in the pthread implementation, it sounds like patterns like this are supposed to work. We've had problems in the past with buggy pthreads implementations as well.

That being said, I'm not totally opposed to having a special case. It's a little unfortunate to start growing an array of #[cfg] directives that keep growing, but it is fairly isolated.

Out of curiosity, do you know if this issue could be brought up with DragonFly maintainers?

@mneumann
Copy link
Contributor Author

mneumann commented Jan 8, 2015

@alexcrichton: The related DragonFly bug report is here, but I doubt it will change, and even if it will be "fixed", there will always be older versions of DragonFly and Rust won't work on them. The way it is implemented in DragonFly protects you against duplicate pthread_mutex_destroy() calls, which IMHO is a good thing, except that it breaks the pthread spec which is saying that PTHREAD_MUTEX_INITIALIZER should have the same effect as calling pthread_mutex_init(), which is not true on DragonFly. So I'd suggest to live with this #[cfg]-magic, hopefully it'll be the last required to get it working on DragonFly. One way to fix it in DragonFly would be to use a special sentinel value (e.g. a (void*)1 pointer; NULL is used as initializer) which denotes that the mutex was destroyed.

@alexcrichton
Copy link
Member

Alright, sounds good to me! Thanks for opening a report with them :)

@mneumann
Copy link
Contributor Author

mneumann commented Jan 8, 2015

@alexcrichton: We fixed it with this commit. It's now questionable if we still need to apply this pull request. I can update my DragonFly system, anyone else who wants to run Rust should do so too.

@alexcrichton
Copy link
Member

Nice! Do you know how quickly updates roll out to DragonFly? If it takes awhile, we could just slate this for deletion in 6 months or so.

bors added a commit that referenced this pull request Jan 9, 2015
Fix assertion in Mutex::destroy() on DragonFly (#20698)

Reviewed-by: alexcrichton
@bors bors merged commit b527494 into rust-lang:master Jan 9, 2015
@mneumann
Copy link
Contributor Author

mneumann commented Jan 9, 2015

Usually every 6 months there is a new DragonFly release. Agreed, let's remove it some time in the future. Thanks!

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.

None yet

5 participants