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

Set thread names for game threads #666

Merged
merged 8 commits into from
Sep 7, 2024

Conversation

p0358
Copy link
Contributor

@p0358 p0358 commented Feb 12, 2024

Adds nice thread names that can be visible in crash dumps, non-attachable debuggers and generally in all places where old method of throwing exceptions to attached debugger on game start wouldn't work:
image

Can't believe it wasn't already there lol, makes debugging pain without.

Also the method to convert charsets is bad, but that's not my fault, seems Northstar doesn't have any util func for that apparently either, and this "method" was used in some other places in the codebase where it didn't belong. As thread names are generally static it should be fine in this place, but still.

Tested to work as you can see on the screenshot.

@GeckoEidechse GeckoEidechse added needs testing Changes from the PR still need to be tested needs code review Changes from PR still need to be reviewed in code labels Feb 18, 2024
@GeckoEidechse
Copy link
Member

Uh, where can I see the thread names? ^^"

@p0358
Copy link
Contributor Author

p0358 commented Mar 3, 2024

Many tools, like x64dbg, Very Sleepy debugger, probably in various "process explorer" kind of programs too.

Also worth noting for future reference from RoyalBlue:

if you want names for miles threads as well you need to hook a func there as well because miles only names threads when a debugger is present on thread start
its mileswin64 + 0x3EBD0 a1 is thread handle a2 is the name

https://discord.com/channels/920776187884732556/950322078945538058/1206634307976699905

tested some more and Bink IO is set with just your change already so only the miles hook needs to be done

The above and possibly bink could also be added, but unfortunately for now I cannot be arsed to do that. They might be done in a separate PR though, so I leave that for future reference here. This PR alone makes the situation much nicer already by naming game threads...

@p0358
Copy link
Contributor Author

p0358 commented Mar 3, 2024

Oh also one more thing, this doesn't set the name of MainThread as the hook is done too late. The thread that does the hooking is 99% the main thread, but that should be tested and then when the hook is performed it should set the name of its own thread to "MainThread" then.

@r3muxd
Copy link

r3muxd commented Apr 29, 2024

the real fix for this is to use tier0 to make threads so your TLS gets set up correctly
https://www.unknowncheats.me/forum/counterstrike-global-offensive/335391-fixing-tls-multithreaded-engine-calls.html
for more info as to why this is important read here
particularly you will get weird problems with CMaterialSystem as it uses a CTHREADLOCALPTR for the render context

@GeckoEidechse
Copy link
Member

I was told the comment above is not really relevant to the PR...

@GeckoEidechse
Copy link
Member

Fails to build after updating with main. Any ideas @p0358 ^^

@p0358
Copy link
Contributor Author

p0358 commented Jul 11, 2024

I guess the CModule class was refactored in master

@GeckoEidechse
Copy link
Member

Any chance you could update the PR so that we can merge 👉👈

@GeckoEidechse
Copy link
Member

CI still failing btw @p0358

@p0358
Copy link
Contributor Author

p0358 commented Aug 14, 2024

obraz

This works now too. There's also MilesMixer thread, but it seems it's not being created, thus it's not on the list? idk why but welp

p0358 and others added 2 commits August 14, 2024 23:53
Co-authored-by: Maya <11448698+RoyalBlue1@users.noreply.github.com>
@p0358
Copy link
Contributor Author

p0358 commented Aug 14, 2024

Merge with rebase maybe, I rebased it to two distinct commits on top of main

@GeckoEidechse
Copy link
Member

Will consider, thanks <3

Copy link
Member

@GeckoEidechse GeckoEidechse left a comment

Choose a reason for hiding this comment

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

Verified that it still runs fine on Linux. This is more of a breakage check than a functionality check though tbh...

@github-actions github-actions bot added the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Aug 28, 2024
@github-actions github-actions bot removed the merge conflicts Blocked by merge conflicts, waiting on the author to resolve label Sep 4, 2024
@ASpoonPlaysGames
Copy link
Contributor

Would appreciate if someone could give this code review, since I have switched the hooks over to be manually hooked instead of using autohook.

Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames left a comment

Choose a reason for hiding this comment

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

Works on my machine:
image
image

@GeckoEidechse GeckoEidechse removed the needs testing Changes from the PR still need to be tested label Sep 5, 2024
@GeckoEidechse GeckoEidechse added the almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge label Sep 5, 2024
Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames left a comment

Choose a reason for hiding this comment

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

Code looks good (although I am a bit biased since I converted the hooks to manually hooking) other than the stuff indicated by the comments. Fixing that would be beyond the scope of the PR though so it shouldn't block anything

@ASpoonPlaysGames ASpoonPlaysGames added READY TO MERGE This mergeable right now and removed needs code review Changes from PR still need to be reviewed in code almost ready to merge Apart from any small remaining other issues addressed by other labels, this would be ready to merge labels Sep 7, 2024
@ASpoonPlaysGames
Copy link
Contributor

@F1F7Y Any chance you could sanity check the stuff that I wrote? Would prefer to not be reviewing my own code in a PR if possible

@p0358
Copy link
Contributor Author

p0358 commented Sep 7, 2024

Hehe I can't physically do an approving review on my own PR lol. But yeah it does seem to look okay...

@GeckoEidechse GeckoEidechse merged commit 8824340 into R2Northstar:main Sep 7, 2024
4 checks passed
wolf109909 pushed a commit to R2NorthstarCN/NorthstarLauncher that referenced this pull request Sep 8, 2024
Adds nice thread names that can be visible in crash dumps, non-attachable debuggers and generally in all places where old method of throwing exceptions to attached debugger on game start wouldn't work
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
READY TO MERGE This mergeable right now
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants