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.

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

Conversation

vasild
Copy link
Contributor

@vasild vasild commented Jan 18, 2020

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

Copy link
Collaborator

@ryanofsky ryanofsky left a 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

Comment on lines -40 to +46
char thread_name[17] = {0};
char thread_name[16] = {0};
Copy link
Collaborator

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

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);
Copy link
Collaborator

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

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);
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Collaborator

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

Copy link
Contributor Author

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.
@vasild vasild force-pushed the thread_name_portability branch from c8d7f90 to f1857e3 Compare January 18, 2020 20:44
Copy link
Collaborator

@ryanofsky ryanofsky left a 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

ryanofsky added a commit that referenced this pull request Jan 21, 2020
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
@ryanofsky ryanofsky merged commit f1857e3 into bitcoin-core:master Jan 21, 2020
@vasild vasild deleted the thread_name_portability branch February 21, 2020 10:08
@bitcoin-core bitcoin-core locked and limited conversation to collaborators Jun 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants