Refactor ThreadName() to improve its portability#21
Refactor ThreadName() to improve its portability#21ryanofsky merged 1 commit intobitcoin-core:masterfrom
Conversation
| char thread_name[17] = {0}; | ||
| char thread_name[16] = {0}; |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
re: #21 (comment)
Removed. In this case we would return something like
prog-35291/-20080which 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.
#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.
vasild
force-pushed
the
thread_name_portability
branch
from
c8d7f90 to
f1857e3
Compare
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
2 participants
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.