Use atomics instead of mutex in exit guard #127863

tbu- · 2024-07-17T13:02:15Z

Slightly fewer instructions, no blocking.

rustbot · 2024-07-17T13:02:22Z

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

joboet

This relies on some undocumented details of pthread, I'm not sure that's a good idea.

joboet · 2024-07-17T13:05:32Z

library/std/src/sys/exit_guard.rs

+                    // We set `EXITING_THREAD_ID` to this thread's ID already
+                    // and will return.
+                }
+                id if id == this_thread_id as usize => {


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

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?

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.

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.

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

joboet · 2024-07-17T13:06:41Z

library/std/src/sys/exit_guard.rs

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


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

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.

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

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.

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.

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

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

joboet

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 · 2024-07-30T15:49:13Z

r? @joboet

alex-semenyuk · 2024-09-28T17:33:32Z

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

tbu- · 2024-10-07T16:08:22Z

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

tbu- · 2024-11-12T16:17:41Z

Postponing.

Use atomics instead of mutex in exit guard

fd2c6ca

Slightly fewer instructions, no blocking.

rustbot assigned Amanieu Jul 17, 2024

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

joboet requested changes Jul 17, 2024

View reviewed changes

joboet requested changes Jul 20, 2024

View reviewed changes

rustbot assigned joboet and unassigned Amanieu Jul 30, 2024

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

tbu- closed this Nov 12, 2024


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

		const NONE: usize = 0;

Use atomics instead of mutex in exit guard #127863

Use atomics instead of mutex in exit guard #127863

Uh oh!

Conversation

tbu- commented Jul 17, 2024

Uh oh!

rustbot commented Jul 17, 2024

Uh oh!

joboet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joboet Jul 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joboet left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Amanieu commented Jul 30, 2024

Uh oh!

alex-semenyuk commented Sep 28, 2024

Uh oh!

tbu- commented Oct 7, 2024

Uh oh!

tbu- commented Nov 12, 2024

Uh oh!

Uh oh!

jieyouxu Jul 17, 2024 •

edited

Loading

joboet Jul 20, 2024 •

edited

Loading

joboet left a comment •

edited

Loading