-
Notifications
You must be signed in to change notification settings - Fork 30
Refactor ThreadName() to improve its portability #21
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Tested ACK c8d7f90. But I did suggest some changes below since this wasn't working ideally for me. If you can update the PR, I'll wait before merging
char thread_name[17] = {0}; | ||
char thread_name[16] = {0}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This change looks right. Previous code was trying to make room for an extra null byte, but pthread_getname_np documentation does say 16 byte max includes the null byte like PR description says
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, the doc also says that the output name will be null terminated so it may seem ok to remove the initialization = {0}
but I left it because I suspect that if pthread_getname_np()
fails it may leave the name untouched.
pthread_getname_np(pthread_self(), thread_name, sizeof(thread_name)); | ||
uint64_t tid = 0; | ||
#if __linux__ | ||
tid = syscall(SYS_gettid); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like keep using gettid
on linux if possible. On linux, tid thread numbers are different from pthread numbers, and match up with the thread numbers shown in gdb, so it's easier to match up logs with gdb backtraces. Tid numbers are also shorter by default and easier to visually distinguish (20080
vs 140369161336576
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Done. And also resurrected the other call to pthread_threadid_np()
(+ a proper check for its presence).
I checked that on FreeBSD std::this_thread::get_id()
returns some long id that is different from what gdb prints (similar to what you say for linux), but thr_self()
and pthread_getthreadid_np()
print the same id as gdb. So I added pthread_getthreadid_np()
too, and if none is available then fallback to std::this_thread::get_id()
.
src/mp/util.cpp
Outdated
#endif | ||
// By default, all the threads inherit the program name. So use that one | ||
// if we don't have pthread_getname_np(). | ||
snprintf(thread_name, sizeof(thread_name), "%s", exe_name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this isn't safe because exe_name can be null here. Probably better in any case just to leave thread_name empty if it's not known, so exe_name isn't repeated twice below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. In this case we would return something like prog-35291/-20080
which looks a bit strange and may break some tool that tries to parse it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #21 (comment)
Removed. In this case we would return something like
prog-35291/-20080
which looks a bit strange and may break some tool that tries to parse it.
I'm not too concerned about appearance of the dash, but omitting it when the thread name is empty could be a possible improvement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#24 Don't print a dash if thread name is not known
* Test for the presence of pthread_getname_np() during cmake time and only use it if it is available. At least FreeBSD does not have it. * Similar with pthread_threadid_np() - use it only if it is available. Also detect and possibly use pthread_getthreadid_np(). Fallback to C++11 standard way to retrieve a thread id if none of the others are available. C++11 thread ids are longer and different than the ones printed by gdb. * According to pthread_getname_np(3) the thread name length is restricted to 16 characters, including the terminating null byte. So reduce our thread_name[] from 17 to 16 chars.
c8d7f90
to
f1857e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK f1857e3. Thanks! This is working well for me now
f1857e3 Refactor ThreadName() to improve its portability (Vasil Dimov) Pull request description: * Test for the presence of pthread_getname_np() during cmake time and only use it if it is available. At least FreeBSD does not have it. When not available, fall back to (the first 15 chars of) the executable name, as if pthread_setname_np() has never been called. * Use std::this_thread::get_id() to retrieve the thread id in a portable way instead of pthread_threadid_np() or syscall() (none of which is available on FreeBSD but it has its own pthread_getthreadid_np()). C++11 is here to help. * According to pthread_getname_np(3) the thread name length is restricted to 16 characters, including the terminating null byte. So reduce our thread_name[] from 17 to 16 chars. ACKs for top commit: ryanofsky: Tested ACK f1857e3. Thanks! This is working well for me now Tree-SHA512: 7b57b67bb67ecac2e25d83a4c1a3cc0fb833a96c68af5082cc83b40f5c158e4ae36696f6a6bd13409b6d1754712b7ec3e69743acf6e88686497c8a6dc6a7a1da
Test for the presence of pthread_getname_np() during cmake time and
only use it if it is available. At least FreeBSD does not have it.
When not available, fall back to (the first 15 chars of) the
executable name, as if pthread_setname_np() has never been called.
Use std::this_thread::get_id() to retrieve the thread id in a portable
way instead of pthread_threadid_np() or syscall() (none of which is
available on FreeBSD but it has its own pthread_getthreadid_np()).
C++11 is here to help.
According to pthread_getname_np(3) the thread name length is
restricted to 16 characters, including the terminating null byte.
So reduce our thread_name[] from 17 to 16 chars.