Use atomics instead of mutex in exit guard#127863
Use atomics instead of mutex in exit guard#127863tbu- wants to merge 1 commit intorust-lang:masterfrom
Conversation
Slightly fewer instructions, no blocking.
rustbot
added
S-waiting-on-review
joboet
left a comment
joboet
left a comment
There was a problem hiding this comment.
This relies on some undocumented details of pthread, I'm not sure that's a good idea.
| // We set `EXITING_THREAD_ID` to this thread's ID already | ||
| // and will return. | ||
| } | ||
| id if id == this_thread_id as usize => { |
There was a problem hiding this comment.
That's a fuzzy-provenance cast, at least on musl. Please use AtomicPtr instead.
There was a problem hiding this comment.
I'm not using the value as a pointer again. Is it still required in this case? Do you have a link where I can read up on this?
There was a problem hiding this comment.
Yes. Casting a pointer to an integer is a side-effecting operation. Also, miri will warn against this once it supports atexit, since the exposed provenance model isn't finalized yet.
There was a problem hiding this comment.
Is casting an arbitrary integer to an (invalid) pointer value a problem under this model? pthread_t is an integer on some platforms and a pointer on others.
|
|
||
| const _: () = assert!(mem::size_of::<libc::pthread_t>() <= mem::size_of::<usize>()); | ||
|
|
||
| const NONE: usize = 0; |
There was a problem hiding this comment.
Have you confirmed that zero is definitely not an allowed pthread_t value?
There was a problem hiding this comment.
According to https://man7.org/linux/man-pages/man3/pthread_equal.3.html:
POSIX.1 allows an implementation wide freedom in choosing the
type used to represent a thread ID; for example, representation
using either an arithmetic type or a structure is permitted.
Therefore, variables of type pthread_t can't portably be compared
using the C equality operator (==); use pthread_equal(3) instead.
I don't think it's a good idea to assume pthread_t can be trivially compared as integers. I can't immediately tell if the original implementation uses integer equality, but probably pthread_equal (or a PartialEq implementation based on that) should be used instead.
There was a problem hiding this comment.
Yeah, in retrospect I should have insisted on that in #126606. We only use this on Linux, so this currently works. In the long-term, I think thread::current().id() is the best solution, but that's currently not available in TLS destructors (#124881 will improve that situation and I have an idea for solving this completely).
There was a problem hiding this comment.
I don't think it's a good idea to assume
pthread_tcan be trivially compared as integers. I can't immediately tell if the original implementation uses integer equality, but probablypthread_equal(or a PartialEq implementation based on that) should be used instead.
AFAIK the passage you quoted was added because C can't use == for structs. We can see that none of the targets supported by libc use a struct for pthread_t. I think we should disregard this section of the man page.
There was a problem hiding this comment.
I used == in the original PR mostly just because libc does not (yet) expose pthread_equal. glibc and musl at least both appear to implement pthread_equal as just == from what I found.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
exit isn't signal-safe, so it mustn't be called after fork anyway.
There was a problem hiding this comment.
I updated #127912 to include a change that makes thread::current_id always succeed and always return the same ID. Once that gets merged, we can use it here.
EDIT: nevermind, current_id always returns the same ID on Linux anyway because it supports #[thread_local].
There was a problem hiding this comment.
Please use the newly added Tid and thread::current_id functions for this. To do so, please move Tid to thread/mod.rs and rename it OwningTid or something like that.
Or alternatively, replace this whole thing with a ReentrantLock<()>. Thinking about it, that's probably the best option.
|
r? @joboet |
joboet
added
S-waiting-on-author
|
@tbu |
|
No questions, just haven't done it yet. The instructions look clear to me. |
|
Postponing. |
9 participants
Slightly fewer instructions, no blocking.