Refactor ThreadName() to improve its portability #21

pull vasild wants to merge 1 commits into bitcoin-core:master from vasild:thread_name_portability changing 3 files +62 −6

vasild commented at 9:50 AM on January 18, 2020: contributor
- 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.

in CMakeLists.txt:16 in c8d7f90cee outdated

   4 | @@ -5,9 +5,19 @@
   5 |  cmake_minimum_required(VERSION 3.0)
   6 |  project("Libmultiprocess" CXX)
   7 |  include(CTest)
   8 | +include(CheckCXXSourceCompiles)
   9 |  find_package(Boost)
  10 |  find_package(CapnProto)
  11 |  
  12 | +check_cxx_source_compiles("

ryanofsky commented at 3:05 PM on January 18, 2020:

This check was failing for me due to a link error (linux requires pthreads programs to be linked with a pthreads library), so I wasn't seeing thread names after this. Following fix worked for me:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 16ecf50..cf87baf 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -4,11 +4,15 @@
 
 cmake_minimum_required(VERSION 3.0)
 project("Libmultiprocess" CXX)
+include(CMakePushCheckState)
 include(CTest)
 include(CheckCXXSourceCompiles)
 find_package(Boost)
 find_package(CapnProto)
+find_package(Threads REQUIRED)
 
+cmake_push_check_state()
+set(CMAKE_REQUIRED_LIBRARIES Threads::Threads)
 check_cxx_source_compiles("
   #include <pthread.h>
   int main(int argc, char** argv)
@@ -17,6 +21,7 @@ check_cxx_source_compiles("
     return pthread_getname_np(pthread_self(), thread_name, sizeof(thread_name));
   }"
   HAVE_PTHREAD_GETNAME_NP)
+cmake_pop_check_state()
 
 capnp_generate_cpp(MP_PROXY_SRCS MP_PROXY_HDRS include/mp/proxy.capnp)

vasild commented at 8:44 PM on January 18, 2020:

Excellent, done!

in src/mp/util.cpp:46 in c8d7f90cee outdated
```
  38 | @@ -37,14 +39,17 @@ size_t MaxFd()
  39 |  
  40 |  std::string ThreadName(const char* exe_name)
  41 |  {
  42 | -    char thread_name[17] = {0};
  43 | +    char thread_name[16] = {0};
```
ryanofsky commented at 3:07 PM on January 18, 2020:

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

vasild commented at 8:47 PM on January 18, 2020:

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.
in src/mp/util.cpp:44 in c8d7f90cee outdated
```
  43 | +    char thread_name[16] = {0};
  44 | +#ifdef HAVE_PTHREAD_GETNAME_NP
  45 |      pthread_getname_np(pthread_self(), thread_name, sizeof(thread_name));
  46 | -    uint64_t tid = 0;
  47 | -#if __linux__
  48 | -    tid = syscall(SYS_gettid);
```
ryanofsky commented at 3:11 PM on January 18, 2020:

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)

vasild commented at 8:57 PM on January 18, 2020:

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().
in src/mp/util.cpp:48 in c8d7f90cee outdated
```
  49 |  #else
  50 | -    pthread_threadid_np(NULL, &tid);
  51 | -#endif
  52 | +    // By default, all the threads inherit the program name. So use that one
  53 | +    // if we don't have pthread_getname_np().
  54 | +    snprintf(thread_name, sizeof(thread_name), "%s", exe_name);
```
ryanofsky commented at 3:17 PM on January 18, 2020:

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.

vasild commented at 9:00 PM on January 18, 2020:

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.

ryanofsky commented at 6:17 PM on January 21, 2020:

re: https://github.com/chaincodelabs/libmultiprocess/pull/21#discussion_r368248257

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

vasild commented at 10:27 AM on February 21, 2020:

https://github.com/chaincodelabs/libmultiprocess/pull/24 Don't print a dash if thread name is not known
ryanofsky approved
ryanofsky commented at 3:27 PM on January 18, 2020: collaborator

Thanks! Tested ACK c8d7f90cee4af62c06362ff8fbbd2bef3507eb87. 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

Refactor ThreadName() to improve its portability

* 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.

f1857e3e91

vasild force-pushed on Jan 18, 2020
ryanofsky commented at 6:20 PM on January 21, 2020: collaborator

Tested ACK f1857e3e919fee9bf10057aad6cd9db54a621333. Thanks! This is working well for me now
ryanofsky referenced this in commit 50b6a7f8c2 on Jan 21, 2020
ryanofsky merged this on Jan 21, 2020
ryanofsky closed this on Jan 21, 2020
vasild deleted the branch on Feb 21, 2020
bitcoin-core locked this on Jun 25, 2025

Contributors

vasild

ryanofsky

Linked (view graph)

#115 MSAN Error during connection shutdown #162 macos "Broken pipe" exceptions during tests #219 SIGABRT in ~ProxyClientBase with #29409 and rust client