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 up SYS_gettid usage on macOS for jitdump #259

Merged
merged 1 commit into from
Mar 20, 2024
Merged

Conversation

vvuk
Copy link

@vvuk vvuk commented Mar 19, 2024

Found while creating upstream patch (which aborts during build because syscall() is deprecated in the newer SDKs). Turns out macOS SYS_gettid is completely unrelated to linux gettid (it returns effective uid/gid a thread is operating under). There is pthread_threadid_np(), but it returns a uint64_t and we need a uint32_t. This just sets the tid to 0. Profilers care about the thread ID anyway.

// effective uid/gid a thread is operating under)
// There is pthread_threadid_np(), but it returns a uint64_t.
// Given that we need a uint32_t, just return 0 here for now.
return 0;

Choose a reason for hiding this comment

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

I think this actually needs to use pthread_threadid_np for correctness - perhaps upstream needs long for tid record

Copy link
Author

Choose a reason for hiding this comment

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

It can't -- this field is part of the jitdump binary specification. From reading some other source, pthread_threadid_np is uint64, but in practice it's actually always the size of a pid_t. However for this use I really don't think it matters. All this ends up recording is what thread generated the jit code; not very useful info IMO.

@vvuk vvuk merged commit 00fe1c1 into unity-main Mar 20, 2024
2 checks passed
@vvuk vvuk deleted the fix-macos-gettid branch March 20, 2024 16:13
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.

2 participants