Page MenuHomeWildfire Games

Add thread names on Linux
Needs ReviewPublic

Authored by linkmauve on Dec 18 2019, 1:41 PM.

Details

Reviewers
vladislavbelov
Summary

This helps with debugging which thread is doing what.

Also add the thread name of the UPnP thread.

Test Plan
  • Run pyrogenesis.
  • Run ps H -o 'tid comm' $(pidof pyrogenesis)
  • See that all threads are named.

Event Timeline

linkmauve created this revision.Dec 18 2019, 1:41 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/783/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/lib/sysdep/os/linux/ldbg.cpp
|   1| /*·Copyright·(C)·2010·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2010"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1299/display/redirect

vladislavbelov added inline comments.
source/lib/sysdep/os/linux/ldbg.cpp
165

It's not a portable function in nonstandard GNU extensions. So it's not guaranteed to be present.

linkmauve updated this revision to Diff 10666.Dec 20 2019, 12:44 PM
  • Update the copyright year.
  • Replace glibc’s non-portable pthread_setname_np() with an implementation that will work on all Linux libc.
linkmauve marked an inline comment as done.Dec 20 2019, 12:45 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1325/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/809/display/redirect

linkmauve updated this revision to Diff 10667.Dec 20 2019, 12:50 PM

Include missing unistd.h header for gettid().

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1326/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/810/display/redirect

linkmauve updated this revision to Diff 10668.Dec 20 2019, 12:55 PM

Use syscall() directly instead of a wrapper.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/811/display/redirect

linkmauve updated this revision to Diff 10669.Dec 20 2019, 12:58 PM

Check for fopen()’s failure, thanks Angen for the suggestion!

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/812/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1327/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1328/display/redirect

linkmauve updated this revision to Diff 10670.Dec 20 2019, 1:15 PM

Make magic constants less magic by binding them to a name.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/813/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1329/display/redirect

vladislavbelov added inline comments.Dec 26 2019, 4:01 PM
source/lib/sysdep/os/linux/ldbg.cpp
169

It'd be good to attach a comment, what's going there. Why do we need to open some file and which limitations do/did we have here. Why didn't we use the posix*_np function.

linkmauve updated this revision to Diff 10876.Jan 4 2020, 6:17 PM
  • Add a comment explaining the function.
  • Update the copyright year.
Vulcan added a comment.Jan 4 2020, 6:20 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/947/display/redirect

Vulcan added a comment.Jan 4 2020, 6:22 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/43/display/redirect

Vulcan added a comment.Jan 4 2020, 6:23 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1465/display/redirect

source/lib/sysdep/os/linux/ldbg.cpp
159

We use const char* in its declaration, so we might fix it here.

164–177

Can't we use TASK_COMM_LEN here?

165

|/proc/self/task//comm| = 21. On some platforms typeof(tid) = uint64_t, length of maximum possible id has 20 symbol length. It means that 32 isn't enough here.

169

It works only since Linux 2.6.33, maybe we need a comment about that.

linkmauve added inline comments.Jan 22 2020, 6:28 PM
source/lib/sysdep/os/linux/ldbg.cpp
164–177

This constant isn’t defined on my system, where do you get it from?

165

Oh, I wasn’t aware that pid_t could be 64-bit, I’ll fix that.

linkmauve added inline comments.Jan 22 2020, 6:29 PM
source/lib/sysdep/os/linux/ldbg.cpp
165

Out of curiosity, which platform is that?

linkmauve added inline comments.Jan 22 2020, 6:31 PM
source/lib/sysdep/os/linux/ldbg.cpp
165

In this case though, the %u four lines below would have to be changed too on this platform; is there a PRI* macro for pid_t?

linkmauve updated this revision to Diff 11146.Jan 22 2020, 6:35 PM
  • Add a comment about prehistoric kernels not supporting thread names.
  • Change the order of const and char.
linkmauve marked 3 inline comments as done.Jan 22 2020, 6:36 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1629/display/redirect

vladislavbelov added inline comments.Jan 26 2020, 8:34 PM
source/lib/sysdep/os/linux/ldbg.cpp
164–177

I think I was wrong here, because I can't find that documentation page where I saw it. Maybe just use std::stringstream everywhere? Since it shouldn't be called often.

165

I don't know about these modifiers, I can suggest to use a uint64_t as a proper type for most possible cases or just use std::stringstream, which is safe and easier.