On 15.11.2012 20:38, Philippe Waroquiers wrote:
On Thu, 2012-11-15 at 06:48 +0100, Matthias Schwarzott wrote:
On 07.11.2012 16:54, Matthias Schwarzott wrote:
Hi!
I wonder if it would be good to have valgrind remember the names of the
threads.
Here is a first implementation of this change.
Thanks for looking at this.
It just enhances the struct ThreadState by a char array thread_name.
The handler of prctl writes its argument there.
According to the (maybe too quick?) discussion in
http://sourceforge.net/mailarchive/message.php?msg_id=30069236
this implementation will give wrong information for some kinds
of errors (typically when showing an error after a thread has
terminated).
Or do you think these cases can in fact not happen ?
I think for memcheck at least this cannot happen (or does the leak list
shows thread names?).
For helgrind it may be possible to copy the threadname if a thread
finishes (for later reports).
But I observed, that memcheck does not notice this sequence:
create a thread.
error
end a thread
create a thread with same tid.
error
For the second error, there is no "Thread %d" line printed, as the tid
is unchanged.
Or maybe this is just making an existing bug more visible ?
=> would be worth digging more in depth in that.
For the first thread it is initialized to "main" (without valgrind there
is the name of the executable by default, how could I get this).
If you do call prctl(PR_GET_NAME, ...) just at startup,
it returns the name of the executable (truncated to 15 characters).
Sadly for a process under valgrind, this returns just "memcheck-amd64-".
Maybe it can be fetched from VG_(args_the_exename).
The testcase I started to write is also included.
Some first little comments about the (early) patch:
+ if (tst->thread_name[0]) {
+ VG_(umsg)("Thread %d %s:\n", err->tid,
VG_(get_ThreadState)(err->tid)->thread_name);
Should (re-)use tst rather than (re-)get the thread state.
Indentation is not correct at a few places or lines are longer than 80 chars.
For the test case: is pthread_setname_np available in all the
supported platforms ? Otherwise I guess that some autoconf magic or a #ifdef or
another
The .exp expected files are still expected :).
No idea, but as this function is part of glibc I guess at least on every
linux-based platform it should be available.
Maybe it need to be checked at what glibc version this call was introduced.
Matthias
commit a45bfc513368699658bee95bf335c8f4661b0ca1
Author: Matthias Schwarzott <z...@gentoo.org>
Date: Tue Oct 30 20:39:21 2012 +0100
set thread name V2
Fixed reusing variable tst.
Fixed whitespace.
Fixed long line.
diff --git a/coregrind/m_errormgr.c b/coregrind/m_errormgr.c
index b4fec57..34b063b 100644
--- a/coregrind/m_errormgr.c
+++ b/coregrind/m_errormgr.c
@@ -603,7 +603,12 @@ static void pp_Error ( Error* err, Bool allow_db_attach, Bool xml )
if (VG_(tdict).tool_show_ThreadIDs_for_errors
&& err->tid > 0 && err->tid != last_tid_printed) {
- VG_(umsg)("Thread %d:\n", err->tid );
+ ThreadState* tst = VG_(get_ThreadState)(err->tid);
+ if (tst->thread_name[0]) {
+ VG_(umsg)("Thread %d %s:\n", err->tid, tst->thread_name);
+ } else {
+ VG_(umsg)("Thread %d:\n", err->tid );
+ }
last_tid_printed = err->tid;
}
diff --git a/coregrind/m_gdbserver/server.c b/coregrind/m_gdbserver/server.c
index 5dcb139..724569b 100644
--- a/coregrind/m_gdbserver/server.c
+++ b/coregrind/m_gdbserver/server.c
@@ -499,9 +499,16 @@ void handle_query (char *arg_own_buf, int *new_packet_len_p)
if (ti != NULL) {
tst = (ThreadState *) inferior_target_data (ti);
/* Additional info is the tid and the thread status. */
- VG_(snprintf) (status, sizeof(status), "tid %d %s",
- tst->tid,
- VG_(name_of_ThreadStatus)(tst->status));
+ if (tst->thread_name[0]) {
+ VG_(snprintf) (status, sizeof(status), "tid %d %s %s",
+ tst->tid,
+ VG_(name_of_ThreadStatus)(tst->status),
+ tst->thread_name);
+ } else {
+ VG_(snprintf) (status, sizeof(status), "tid %d %s",
+ tst->tid,
+ VG_(name_of_ThreadStatus)(tst->status));
+ }
hexify (arg_own_buf, status, strlen(status));
return;
} else {
diff --git a/coregrind/m_scheduler/scheduler.c b/coregrind/m_scheduler/scheduler.c
index ef41c64..fadc00d 100644
--- a/coregrind/m_scheduler/scheduler.c
+++ b/coregrind/m_scheduler/scheduler.c
@@ -234,8 +234,9 @@ ThreadId VG_(alloc_ThreadState) ( void )
Int i;
for (i = 1; i < VG_N_THREADS; i++) {
if (VG_(threads)[i].status == VgTs_Empty) {
- VG_(threads)[i].status = VgTs_Init;
- VG_(threads)[i].exitreason = VgSrc_None;
+ VG_(threads)[i].status = VgTs_Init;
+ VG_(threads)[i].exitreason = VgSrc_None;
+ VG_(threads)[i].thread_name[0] = 0;
return i;
}
}
@@ -614,6 +615,8 @@ ThreadId VG_(scheduler_init_phase1) ( void )
}
tid_main = VG_(alloc_ThreadState)();
+ VG_(strncpy)(VG_(threads)[tid_main].thread_name, "main",
+ sizeof(VG_(threads)[tid_main].thread_name));
/* Bleh. Unfortunately there are various places in the system that
assume that the main thread has a ThreadId of 1.
diff --git a/coregrind/m_syswrap/syswrap-linux.c b/coregrind/m_syswrap/syswrap-linux.c
index 7c94bd5..d0e1d97 100644
--- a/coregrind/m_syswrap/syswrap-linux.c
+++ b/coregrind/m_syswrap/syswrap-linux.c
@@ -939,6 +939,13 @@ POST(sys_prctl)
case VKI_PR_GET_FPEXC:
POST_MEM_WRITE(ARG2, sizeof(Int));
break;
+ case VKI_PR_SET_NAME:
+ {
+ ThreadState* tst = VG_(get_ThreadState)(tid);
+ VG_(strncpy)(tst->thread_name, (char*)ARG2, 16);
+ tst->thread_name[16] = 0;
+ }
+ break;
case VKI_PR_GET_NAME:
POST_MEM_WRITE(ARG2, VKI_TASK_COMM_LEN);
break;
diff --git a/coregrind/pub_core_threadstate.h b/coregrind/pub_core_threadstate.h
index 0bd9927..7983c39 100644
--- a/coregrind/pub_core_threadstate.h
+++ b/coregrind/pub_core_threadstate.h
@@ -353,6 +353,8 @@ typedef struct {
/* Per-thread jmp_buf to resume scheduler after a signal */
Bool sched_jmpbuf_valid;
VG_MINIMAL_JMP_BUF(sched_jmpbuf);
+
+ char thread_name[17];
}
ThreadState;
diff --git a/memcheck/tests/Makefile.am b/memcheck/tests/Makefile.am
index cf1d5cc..4bfce22 100644
--- a/memcheck/tests/Makefile.am
+++ b/memcheck/tests/Makefile.am
@@ -228,7 +228,8 @@ EXTRA_DIST = \
wrap8.vgtest wrap8.stdout.exp wrap8.stderr.exp \
wrap8.stdout.exp2 wrap8.stderr.exp2 \
writev1.stderr.exp writev1.vgtest \
- xml1.stderr.exp xml1.stdout.exp xml1.vgtest xml1.stderr.exp-s390x-mvc
+ xml1.stderr.exp xml1.stdout.exp xml1.vgtest xml1.stderr.exp-s390x-mvc \
+ threadname.vgtest
check_PROGRAMS = \
accounting \
@@ -296,7 +297,8 @@ check_PROGRAMS = \
vcpu_fbench vcpu_fnfns \
xml1 \
wrap1 wrap2 wrap3 wrap4 wrap5 wrap6 wrap7 wrap7so.so wrap8 \
- writev1
+ writev1 \
+ threadname
if DWARF4
check_PROGRAMS += dw4
@@ -323,6 +325,7 @@ dw4_CFLAGS = $(AM_CFLAGS) -gdwarf-4 -fdebug-types-section
err_disable3_LDADD = -lpthread
err_disable4_LDADD = -lpthread
+threadname_LDADD = -lpthread
error_counts_CFLAGS = $(AM_CFLAGS) @FLAG_W_NO_UNINITIALIZED@
diff --git a/memcheck/tests/threadname.c b/memcheck/tests/threadname.c
new file mode 100644
index 0000000..6e2f4a5
--- /dev/null
+++ b/memcheck/tests/threadname.c
@@ -0,0 +1,53 @@
+#define _GNU_SOURCE
+#include <stdio.h>
+#include <pthread.h>
+#include <string.h>
+#include <stdlib.h>
+#include <sys/prctl.h>
+#include <sys/types.h>
+#include <unistd.h>
+#include <assert.h>
+
+void* child_fn ( void* arg )
+{
+ pthread_setname_np(pthread_self(), "");
+ pthread_setname_np(pthread_self(), "a");
+ pthread_setname_np(pthread_self(), "ab");
+ pthread_setname_np(pthread_self(), "abc");
+ pthread_setname_np(pthread_self(), "abcd");
+ pthread_setname_np(pthread_self(), "abcdx");
+ pthread_setname_np(pthread_self(), "abcdxy");
+ pthread_setname_np(pthread_self(), "abcdxyz");
+ pthread_setname_np(pthread_self(), "abcdxyz0");
+ pthread_setname_np(pthread_self(), "abcdxyz01");
+ pthread_setname_np(pthread_self(), "abcdxyz012");
+ pthread_setname_np(pthread_self(), "abcdxyz0123");
+ pthread_setname_np(pthread_self(), "abcdxyz01234");
+ pthread_setname_np(pthread_self(), "abcdxyz012345");
+ pthread_setname_np(pthread_self(), "abcdxyz0123456");
+ pthread_setname_np(pthread_self(), "abcdxyz01234567");
+ pthread_setname_np(pthread_self(), "abcdxyz012345678");
+ pthread_setname_np(pthread_self(), "abcdxyz0123456789");
+ int* m = malloc(sizeof(int));
+ m[2]=0;
+ free(m);
+ return NULL;
+}
+
+int main(int argc, const char** argv)
+{
+ int r;
+ pthread_t child;
+ int* o = malloc(sizeof(int));
+ o[1]=0;
+ free(o);
+ r = pthread_create(&child, NULL, child_fn, NULL);
+ assert(!r);
+ sleep(1);
+ r = pthread_join(child, NULL);
+ assert(!r);
+ o = malloc(sizeof(int));
+ o[1]=0;
+ free(o);
+ return 0;
+}
diff --git a/memcheck/tests/threadname.vgtest b/memcheck/tests/threadname.vgtest
new file mode 100644
index 0000000..01b0d43
--- /dev/null
+++ b/memcheck/tests/threadname.vgtest
@@ -0,0 +1 @@
+prog: threadname
------------------------------------------------------------------------------
Monitor your physical, virtual and cloud infrastructure from a single
web console. Get in-depth insight into apps, servers, databases, vmware,
SAP, cloud infrastructure, etc. Download 30-day Free Trial.
Pricing starts from $795 for 25 servers or applications!
http://p.sf.net/sfu/zoho_dev2dev_nov
_______________________________________________
Valgrind-users mailing list
Valgrind-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/valgrind-users
