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

Use atomics instead of mutex in exit guard #127863

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Jul 17, 2024

Slightly fewer instructions, no blocking.

Slightly fewer instructions, no blocking.
@rustbot
Copy link
Collaborator

rustbot commented Jul 17, 2024

r? @Amanieu

rustbot has assigned @Amanieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 17, 2024
Copy link
Member

@joboet joboet left a comment

Choose a reason for hiding this comment

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

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 => {
Copy link
Member

Choose a reason for hiding this comment

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

That's a fuzzy-provenance cast, at least on musl. Please use AtomicPtr instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

You could use the strict provenance APIs. I.e. addr


const _: () = assert!(mem::size_of::<libc::pthread_t>() <= mem::size_of::<usize>());

const NONE: usize = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Have you confirmed that zero is definitely not an allowed pthread_t value?

Copy link
Member

@jieyouxu jieyouxu Jul 17, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

exit isn't signal-safe, so it mustn't be called after fork anyway.

Copy link
Member

@joboet joboet Jul 20, 2024

Choose a reason for hiding this comment

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

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].

Copy link
Member

@joboet joboet left a comment

Choose a reason for hiding this comment

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

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.

@Amanieu
Copy link
Member

Amanieu commented Jul 30, 2024

r? @joboet

@rustbot rustbot assigned joboet and unassigned Amanieu Jul 30, 2024
@joboet joboet 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 Aug 21, 2024
@alex-semenyuk
Copy link
Member

@tbu
From wg-triage. Do you have questions based on comments to proceed with this PR?

@tbu-
Copy link
Contributor Author

tbu- commented Oct 7, 2024

No questions, just haven't done it yet. The instructions look clear to me.

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 Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants