Skip to content

Print thread name when dumping backtrace #6

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

Open
nslowell wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Print thread name when dumping backtrace #6

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
87b8238
Print thread name when dumping backtrace
nslowell Jun 20, 2016
eff3adb
Cleaner string handling and printing of process comm
nslowell Jun 27, 2016
1a15b0d
Standardize right side padding when printing thread names
nslowell Jun 27, 2016
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
8 changes: 6 additions & 2 deletions src/backtrace.c
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,10 @@ int backtrace_snapshot(int pid, int *tids, int *index, int nr_tids)
return -1;

for (i = 0; i < snap->num_threads; ++i) {
printf("-------------------- thread %d (%d) --------------------\n",
printf("-------------------- thread %d (%d) (",
(index != NULL ? index[i] : i+1), snap->tids[i]);
print_proc_comm(snap->tids[i]);
printf(") --------------------\n");
Copy link
Contributor

@v-nikulichev v-nikulichev Jun 24, 2016

Choose a reason for hiding this comment

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

Let's consider implementing better formatting. It was simple solution from the beginning but I think when changing this code it is the best time to polish output.

Copy link
Author

@nslowell nslowell Jun 24, 2016

Choose a reason for hiding this comment

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

I agree. I was not happy with how this looked, but I wanted to keep it a function call into proc and not have to store it in snapshot.

Do you have any suggestions?

Copy link
Contributor

@v-nikulichev v-nikulichev Jun 24, 2016

Choose a reason for hiding this comment

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

I agree, no need to store it because we use it single time at this point.

We can have 80+1 character buffer filled up with '-', then use snprintf() to print thread number, pid, and name. We could make print_proc_comm() returning pointer to static data so only single snprintf() will be needed. Not sure it is best/simplest solution, it's first coming to my mind. Or better we can prepare the title in another buffer, calculate its length and choose a position in "----" to make it center aligned.


snap->cur_thr = i;
if (backtrace_thread(&snapshot_addr_space_accessors, snap) < 0)
Expand Down Expand Up @@ -145,8 +147,10 @@ int backtrace_ptrace(int pid, int *tids, int *index, int nr_tids)
for (i = 0; i < count; ++i) {
void *upt_info;

printf("-------------------- thread %d (%d) --------------------\n",
printf("-------------------- thread %d (%d) (",
(index != NULL ? index[i] : i+1), threads[i]);
print_proc_comm(threads[i]);
Copy link

@mzraly mzraly Jan 15, 2018

Choose a reason for hiding this comment

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

Is it a good idea to call print_proc_comm here, while the process is stopped?
As print_proc_comm is currently implemented it opens, reads, and closes the comm file for each thread.
In a large server process with thousands of threads, this will add noticeably to the time frozen.
Why not collect the thread names before the call to attach_process(), the same way thread states are handled, then pass the thread name for the current thread to print_thread_heading?

printf(") --------------------\n");

if (threads[i] != pid && attach_thread(threads[i]) < 0) {
rc = -1;
Expand Down
24 changes: 24 additions & 0 deletions src/proc.c
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,30 @@ int print_proc_maps(int pid)
return system(cmd);
}

int print_proc_comm(int pid)
{
FILE *f;
char buf[32] = "";
Copy link
Contributor

@v-nikulichev v-nikulichev Jun 24, 2016

Choose a reason for hiding this comment

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

Is this initialization really needed?

Copy link
Author

@nslowell nslowell Jun 24, 2016

Choose a reason for hiding this comment

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

I believe an earlier implementation required it. It's probably not needed now, however, I feel safer making sure a string is properly terminated.

int i;
int rc = -1;

snprintf(buf, sizeof(buf), "/proc/%d/comm", pid);
if ((f = fopen(buf, "r")) == NULL) {
fprintf(stderr, "cannot open %s: %s\n", buf, strerror(errno));
return -1;
}

if (fgets(buf, sizeof(buf), f)) {
i = strcspn(buf, "\n");
Copy link
Contributor

@v-nikulichev v-nikulichev Jun 24, 2016

Choose a reason for hiding this comment

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

You don't need to put terminator by hand. fgets() does it. From GETS(3):

fgets() reads in at most one less than size characters from stream and stores them into the buffer pointed to by s. Reading stops after an EOF or a newline. If a newline is read, it is stored into the buffer. A terminating null byte ('\0') is stored after the last character in the buffer.

buf[i] = '\0';
printf("%s", buf);
rc = 0;
}

fclose(f);
return rc;
}

/*
* filter for scandir(). choose only thread identifiers
*/
Expand Down
6 changes: 6 additions & 0 deletions src/proc.h
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,12 @@ struct mem_map *create_maps(int pid);
*/
int print_proc_maps(int pid);

/*
* simple routine to print process comm for
* debugging or advanced error reporting
*/
int print_proc_comm(int pid);
Copy link
Contributor

@v-nikulichev v-nikulichev Jun 24, 2016

Choose a reason for hiding this comment

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

If we will decide to polish output, probably this should return number of bytes written or pointer to static data. Latter makes it convenient to pass value to printf().

Copy link
Contributor

@v-nikulichev v-nikulichev Jun 24, 2016

Choose a reason for hiding this comment

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

By the way, I think it's better to name it print_thread_comm() to keep current naming conventions.


/*
* get thread identifiers of the process
*/
Expand Down