publish TID in 2 formats known formats on a crash #1008
Conversation
amandeepgautam
force-pushed
the
master
branch
from
a3b9ae5
to
7423db0
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1008 +/- ##
==========================================
- Coverage 64.25% 64.21% -0.04%
==========================================
Files 17 17
Lines 3321 3323 +2
Branches 1123 1124 +1
==========================================
Hits 2134 2134
- Misses 760 762 +2
Partials 427 427 ☔ View full report in Codecov by Sentry. |
| formatter.AppendString("/"); | ||
| formatter.AppendUint64(static_cast<uint64>(GetTID()), 10); |
There was a problem hiding this comment.
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.
| formatter.AppendString("/"); | |
| formatter.AppendUint64(static_cast<uint64>(GetTID()), 10); | |
| formatter.AppendString(" ("); | |
| formatter.AppendUint64(static_cast<uint64>(GetTID()), 16); | |
| formatter.AppendString(")"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Closing as abandoned. You can rebase anytime if you want to continue working on this PR. |
Fix for: #872