Skip to content

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 21, 2020
Merged

Refactor ThreadName() to improve its portability #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,43 @@

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)
{
char thread_name[16];
return pthread_getname_np(pthread_self(), thread_name, sizeof(thread_name));
}"
HAVE_PTHREAD_GETNAME_NP)

check_cxx_source_compiles("
#include <pthread.h>
int main(int argc, char** argv)
{
uint64_t tid;
pthread_threadid_np(NULL, &tid);
return 0;
}"
HAVE_PTHREAD_THREADID_NP)

check_cxx_source_compiles("
#include <pthread.h>
#include <pthread_np.h>
int main(int argc, char** argv)
{
return pthread_getthreadid_np();
}"
HAVE_PTHREAD_GETTHREADID_NP)
cmake_pop_check_state()

capnp_generate_cpp(MP_PROXY_SRCS MP_PROXY_HDRS include/mp/proxy.capnp)

Expand Down
4 changes: 4 additions & 0 deletions include/mp/config.h.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,8 @@
#cmakedefine CMAKE_INSTALL_PREFIX "@CMAKE_INSTALL_PREFIX@"
#cmakedefine capnp_PREFIX "@capnp_PREFIX@"

#cmakedefine HAVE_PTHREAD_GETNAME_NP @HAVE_PTHREAD_GETNAME_NP@
#cmakedefine HAVE_PTHREAD_THREADID_NP @HAVE_PTHREAD_THREADID_NP@
#cmakedefine HAVE_PTHREAD_GETTHREADID_NP @HAVE_PTHREAD_GETTHREADID_NP@

#endif // MP_CONFIG_H
30 changes: 24 additions & 6 deletions src/mp/util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <mp/config.h>
#include <mp/util.h>

#include <kj/array.h>
Expand All @@ -13,12 +14,17 @@
#include <sys/types.h>
#include <sys/un.h>
#include <sys/wait.h>
#include <thread>
#include <unistd.h>

#if __linux__
#include <syscall.h>
#endif

#ifdef HAVE_PTHREAD_GETTHREADID_NP
#include <pthread_np.h>
#endif // HAVE_PTHREAD_GETTHREADID_NP

namespace mp {
namespace {

Expand All @@ -37,16 +43,28 @@ size_t MaxFd()

std::string ThreadName(const char* exe_name)
{
char thread_name[17] = {0};
char thread_name[16] = {0};
Comment on lines -40 to +46
Copy link
Collaborator

@ryanofsky ryanofsky Jan 18, 2020

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

Copy link
Contributor Author

@vasild vasild Jan 18, 2020

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.

#ifdef HAVE_PTHREAD_GETNAME_NP
pthread_getname_np(pthread_self(), thread_name, sizeof(thread_name));
uint64_t tid = 0;
#endif // HAVE_PTHREAD_GETNAME_NP

std::ostringstream buffer;
buffer << (exe_name ? exe_name : "") << "-" << getpid() << "/" << thread_name << "-";

// Prefer platform specific thread ids over the standard C++11 ones because
// the former are shorter and are the same as what gdb prints "LWP ...".
#if __linux__
tid = syscall(SYS_gettid);
Copy link
Collaborator

@ryanofsky ryanofsky Jan 18, 2020

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)

Copy link
Contributor Author

@vasild vasild Jan 18, 2020

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().

#else
buffer << syscall(SYS_gettid);
#elif defined(HAVE_PTHREAD_THREADID_NP)
uint64_t tid = 0;
pthread_threadid_np(NULL, &tid);
buffer << tid;
#elif defined(HAVE_PTHREAD_GETTHREADID_NP)
buffer << pthread_getthreadid_np();
#else
buffer << std::this_thread::get_id();
#endif
std::ostringstream buffer;
buffer << (exe_name ? exe_name : "") << "-" << getpid() << "/" << thread_name << "-" << tid;

return std::move(buffer.str());
}

Expand Down