Skip to content
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

Jump to bottom

publish TID in 2 formats known formats on a crash #1008

Closed
wants to merge 1 commit into from
Closed

publish TID in 2 formats known formats on a crash #1008

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
2 changes: 2 additions & 0 deletions src/signalhandler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,8 @@ void DumpSignalInfo(int signal_number, siginfo_t *siginfo) {
pthread_t id = pthread_self();
formatter.AppendUint64(
reinterpret_cast<uint64>(reinterpret_cast<const char*>(id)), 16);
formatter.AppendString("/");
formatter.AppendUint64(static_cast<uint64>(GetTID()), 10);
Comment on lines +214 to +215
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest using a format similar to the backtrace. Instead of delineating the PID and TID by /, a PID (TID) formatting (TID being to base 16 with the 0x prefix) seems to be more appropriate.

Suggested change
formatter.AppendString("/");
formatter.AppendUint64(static_cast<uint64>(GetTID()), 10);
formatter.AppendString(" (");
formatter.AppendUint64(static_cast<uint64>(GetTID()), 16);
formatter.AppendString(")");

Copy link
Author

@amandeepgautam amandeepgautam Dec 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing it in base 16 may not be ideal as glog internally publishes base 10 POSIX thread IDs. For example, a line would be:

I1231 01:02:58.835251 1681 kkkk.cc:1591] kkkkkk

So, if I use 0x format, one would have to convert it to 1681 (which would be one more step) to know which thread ID it was.

Let me know what you think.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, in the suggested format we would have 2 nested parenthesis like:
(TID 0x7f7666a8b980 (0x2f424...))

I find 2 nested brackets slightly odd. But am okay with what you say.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we can modify the comment to be:
received by PID 2216 in kernel thread ID 0x7f7666a8b980 (POSXI ID: <...>) from PID 2216

from the current version:
received by PID 2216 (TID 0x7f7666a8b980) from PID 2216

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only referred to TID to be base 16 which is the format currently used by glog. The base 10 format of POSIX thread ID should remain. It is important that the format is consistent throughout with respect to radix, ID order, possible labels, etc.

Nested parentheses are of course best avoided. You can merge the identifiers and put them into a single pair of round brackets joined by , (comma and a single space).

Copy link
Author

@amandeepgautam amandeepgautam Dec 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only referred to TID to be base 16 which is the format currently used by glog. The base 10 format of POSIX thread ID should remain.

I am slightly confused now. Please bear with me as I am not familiar with the codebase. As per this line, GetTID seems to be the one returning unsigned int. So static_cast<uint64>(GetTID()), 10); which is my original implementation seems reasonable. In the suggestion in the comment, we have static_cast<uint64>(GetTID()), 16); which seems to be suggesting that I should convert it to base16, which is not my intention. Can you please let me know what am I missing?

Nested parentheses are of course best avoided. You can merge the identifiers and put them into a single pair of round brackets joined by , (comma and a single space).

Ack.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are correct. I looked through the code and the TID formatting is indeed inconsistent. I therefore suggest keeping the existing format variants and use base 10 representation for the kernel thread IDs consistently instead.

Please note that #1019 removed the use of POSIX threads. There are now conflicts in your branch which you should rebase and resolve.

formatter.AppendString(") ");
// Only linux has the PID of the signal sender in si_pid.
#ifdef GLOG_OS_LINUX
Expand Down