Skip to content

Add shims for gettid-esque functions #4397

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

tgross35
Copy link
Contributor

Various platforms provide a function to return the current OS thread ID, but they all use a slightly different name. Add shims for these functions for Apple, FreeBSD, and Windows, with tests to account for a few more platforms that are not yet supported by Miri.

These should be useful in general but should also help support printing the OS thread ID in panic messages 1.

@tgross35 tgross35 force-pushed the gettid-shims branch 4 times, most recently from 4ca7e19 to 82b76b0 Compare June 13, 2025 01:42
src/shims/env.rs Outdated
Comment on lines 124 to 138
/// Get an "OS" thread ID for any thread.
fn get_tid(&self, thread: ThreadId) -> u32 {
let this = self.eval_context_ref();
let index = thread.to_u32();

// todo: is this also true on Windows?
// Compute a TID for this thread, ensuring that the main thread has PID == TID.
this.get_pid().strict_add(index)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChrisDenton is there any relationship between the PID and TIDs on Windows?

This (preexisting) logic works for Unix because the first TID for a process is the same as the PID, but if that logic isn't true on Windows then I'll need to update this. If it is true on Windows, then I think the CreateThread shim needs to get updated to return the same TID logic in lpThreadId (hence the failing test).

Copy link
Member

Choose a reason for hiding this comment

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

On Windows thread IDs and process IDs are unrelated.

Copy link
Member

Choose a reason for hiding this comment

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

But it doesn't hurt to use the Unix scheme on Windows as well I guess?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, that makes things easier. Thanks.

Copy link
Contributor Author

@tgross35 tgross35 Jun 13, 2025

Choose a reason for hiding this comment

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

Oops, I replied before seeing your message Ralf. Apparently it seems like a Linux thing more specifically than a Unix thing, I can't find documentation for other platforms about a relationship between PID and TID.

I just updated to only offset by PID on Linux, but do you prefer I put it back on all platforms and update the ID saved by CreateThread instead?

Copy link
Contributor Author

@tgross35 tgross35 Jun 13, 2025

Choose a reason for hiding this comment

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

Just checked a handful of different platforms on cfarm:

Linux

main tid 2212321 0x21c1e1
main pid 2212321 0x21c1e1

FreeBSD:

main tid 0 0
main pid 23750 0x5cc6

OpenBSD:

main tid 137368 0x21898
main pid 93069 0x16b8d

NetBSD:

main tid 5057 0x13c1
main pid 5057 0x13c1

NetBSD checking a spawned thread

in spawn: pid 1474 0x5c2 tid 13912 0x3658
in main:  pid 1474 0x5c2 tid 1474 0x5c2

MacOS:

main tid 7763176 0x7674e8
main pid 51507 0xc933

Based on that nonscientific experiment I'll add NetBSD to the list of platforms that says tid is an offset from pid. For the others it's probably actually better that they don't line up, so users don't rely on that non-cross-platform behavior.

@tgross35 tgross35 changed the title Add shims for gettid and similar functions Add shims for gettid-esque functions Jun 13, 2025
@tgross35 tgross35 force-pushed the gettid-shims branch 5 times, most recently from 3add687 to eda5348 Compare June 13, 2025 20:08
@tgross35 tgross35 force-pushed the gettid-shims branch 2 times, most recently from 14a3826 to 72a99da Compare June 18, 2025 21:00
@tgross35
Copy link
Contributor Author

Updated to include the SYS_gettid syscall

@tgross35 tgross35 force-pushed the gettid-shims branch 3 times, most recently from 7f29fbb to 2a46fbe Compare June 18, 2025 21:35
@tgross35 tgross35 force-pushed the gettid-shims branch 2 times, most recently from a683afe to 68601cd Compare June 24, 2025 07:51
@tgross35
Copy link
Contributor Author

Rebased because of the test failures but that didn't help; I don't know why this seems to have an effect on alloc-access-tracking.rs.

@@ -66,7 +66,7 @@ impl<'tcx> MiriMachine<'tcx> {
ecx,
&["__cxa_thread_atexit_impl", "__clock_gettime64"],
)?;
Self::weak_symbol_extern_statics(ecx, &["getrandom", "statx"])?;
Self::weak_symbol_extern_statics(ecx, &["getrandom", "gettid", "statx"])?;
Copy link
Member

@RalfJung RalfJung Jun 28, 2025

Choose a reason for hiding this comment

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

You're adding a new global variable here, that's why alloc-access-tracking changes. You'll have to increment the ID there by 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems I needed to increment by 2, not sure if that is expected

Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

Lots of little comments :)
@rustbot author


// Spin until the main thread has a chance to read this thread's ID
while !CAN_JOIN.load(Ordering::Relaxed) {
thread::sleep(sleep_duration);
Copy link
Member

Choose a reason for hiding this comment

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

Just use yield_now here, so need to have arbitrary sleep durations.

Comment on lines 51 to 56
/// Specific platforms can query the tid of arbitrary threads; test that here.
#[cfg(any(target_vendor = "apple", windows))]
mod queried {
Copy link
Member

Choose a reason for hiding this comment

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

Why does this use the low-level OS APIs for thread creation, instead of the Rust std APIs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Windows and apple have a way to get the thread ID of threads other than the current one, which is being tested here. But to do this you need the pthread_t or the HANDLE, which as far as I know we can't get from std.

I'll comment this a bit better

@rustbot
Copy link
Collaborator

rustbot commented Jun 28, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Jun 28, 2025
tgross35 added 3 commits June 28, 2025 14:15
Various platforms provide a function to return the current OS thread ID,
but they all use a slightly different name. Add shims for these
functions for Apple, FreeBSD, and Windows, with tests to account for
those and a few more platforms that are not yet supported by Miri. The
syscall and extern symbol is included as well on Linux.

These should be useful in general but should also help support printing
the OS thread ID in panic messages [1].

[1]: rust-lang/rust#115746
@tgross35
Copy link
Contributor Author

I believe I addressed all of the concerns. I also updated the Windows and Mac implementations (and tests) since they return gracefully rather than crashing with null or (on Windows) invalid pointers.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Jun 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Waiting for a review to complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants