From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4068 invoked by alias); 16 Aug 2013 05:32:36 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 4043 invoked by uid 89); 16 Aug 2013 05:32:34 -0000 X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,MSGID_MULTIPLE_AT,SPF_PASS autolearn=no version=3.3.2 Received: from out548-7.mail.aliyun.com (HELO out548-7.mail.aliyun.com) (42.121.48.7) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 16 Aug 2013 05:32:32 +0000 Received: from ALIYUN032621(182.92.247.1) by smtp.aliyun-inc.com(127.0.0.1); Fri, 16 Aug 2013 13:32:27 +0800 From: "Will Huang" To: "'Yao Qi'" Cc: References: <00b201ce98a6$7cf69150$76e3b3f0$@huang@aliyun-inc.com> <520D8D9A.3050003@codesourcery.com> In-Reply-To: <520D8D9A.3050003@codesourcery.com> Subject: RE: [PATCH][PR threads/15824] when get threads name failed from info threads with linux kernel version earlier than 2.6.33 Date: Fri, 16 Aug 2013 05:32:00 -0000 Message-ID: <016701ce9a41$f39086d0$dab19470$@huang@aliyun-inc.com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-SW-Source: 2013-08/txt/msg00415.txt.bz2 Thanks for Yao Qi's review. Patch is update. Test OS:Linux 2.6.32-220.23.2.ali1089.el5.x86_64 GNU gdb (GDB) 7.6 Under linux kernel 2.6.32( or earlier version) there is no proc file named = /proc/$pid/task/$tid/comm.=20 GDB init their threads' name from this proc file. So info threads can't pri= nt correct thread names when this proc file not exist. This can be solve by using procfs /proc/$pid/task= /$tid/status instead if comm=20 file not exists. Status file has a Name line which starts by "Name:". diff -uNr ./gdb.org/ChangeLog ./gdb/ChangeLog --- ./gdb.org/ChangeLog 2013-04-26 19:43:30.000000000 +0800 +++ ./gdb/ChangeLog 2013-08-16 11:48:20.000000000 +0800 @@ -1,3 +1,11 @@ +2013-08-14 Will Huang + + PR threads/15824 + * linux-nat.c (linux_nat_thread_name): Add get thread name from + procfs "/proc/$pid/task/$tid/status". + * commo/linux-procfs.c (linux_proc_get_thread_name + linux_proc_get_string): New. + 2013-04-26 Joel Brobecker * NEWS: Change "since GDB 7.5" into "in GDB 7.6". diff -uNr ./gdb.org/common/linux-procfs.c ./gdb/common/linux-procfs.c --- ./gdb.org/common/linux-procfs.c 2013-01-01 14:32:54.000000000 +0800 +++ ./gdb/common/linux-procfs.c 2013-08-16 11:48:49.000000000 +0800 @@ -55,6 +55,55 @@ return retval; } +/* Return the string length of FILED /proc/LWPID/task/TID/status. + Return -1 if not found. */ + +static int +linux_proc_get_string (pid_t lwpid, pid_t tid, char *target, + size_t t_size, const char *field) +{ + size_t field_len =3D strlen (field); + FILE *status_file; + char buf[100]; + int retval =3D -1; + + if (tid > 0) + snprintf (buf, sizeof (buf), "/proc/%d/task/%d/status", + (int) lwpid, (int) tid); + else + { + /*if TID is zero, ingnore it*/ + snprintf (buf, sizeof (buf), "/proc/%d/status", (int) lwpid); + } + + status_file =3D fopen (buf, "r"); + if (status_file =3D=3D NULL) + { + warning (_("unable to open /proc file '%s'"), buf); + return -1; + } + + while (fgets (buf, sizeof (buf), status_file)) + if (strncmp (buf, field, field_len) =3D=3D 0 && buf[field_len] =3D=3D = ':') + { + size_t pos =3D field_len + 1; + while((buf[pos] =3D=3D ' ' || buf[pos] =3D=3D '\t') + && (pos < sizeof (buf) - 1)) + pos++; + + strncpy (target, &buf[pos], t_size); + target[t_size - 1] =3D '\0'; + retval =3D strlen (target); + if(retval > 0 && target[retval - 1] =3D=3D '\n') + target[retval - 1] =3D '\0'; + + break; + } + + fclose (status_file); + return retval; +} + /* Return the TGID of LWPID from /proc/pid/status. Returns -1 if not found. */ @@ -118,3 +167,13 @@ { return linux_proc_pid_has_state (pid, "Z (zombie)"); } + +/* Save the name of thread PID/TID into buffer NAME. SIZE is the size + of buffer NAME. Return the length of the name if success, + otherwise return -1. */ + +int +linux_proc_get_thread_name (pid_t pid, pid_t tid, char *name, size_t size) +{ + return linux_proc_get_string (pid, tid, name, size, "Name"); +} diff -uNr ./gdb.org/common/linux-procfs.h ./gdb/common/linux-procfs.h --- ./gdb.org/common/linux-procfs.h 2013-01-01 14:32:54.000000000 +0800 +++ ./gdb/common/linux-procfs.h 2013-08-16 11:48:49.000000000 +0800 @@ -40,4 +40,12 @@ extern int linux_proc_pid_is_zombie (pid_t pid); + +/* Save the name of thread PID/TID into buffer NAME. SIZE is the size + of buffer NAME. Return the length of the name if success, + otherwise return -1. */ + +extern int linux_proc_get_thread_name (pid_t pid, pid_t tid, + char *name, size_t size); + #endif /* COMMON_LINUX_PROCFS_H */ diff -uNr ./gdb.org/linux-nat.c ./gdb/linux-nat.c --- ./gdb.org/linux-nat.c 2013-02-13 22:59:49.000000000 +0800 +++ ./gdb/linux-nat.c 2013-08-16 13:22:31.000000000 +0800 @@ -4294,6 +4294,14 @@ fclose (comm_file); } + else + { + static char comm[COMM_LEN + 1]; + int size =3D linux_proc_get_thread_name (pid, lwp, comm, COMM_LEN + = 1); + if (size > 0) + result =3D comm; + + } #undef COMM_LEN #undef FORMAT diff -uNr ./gdb.org/testsuite/ChangeLog ./gdb/testsuite/ChangeLog --- ./gdb.org/testsuite/ChangeLog 2013-04-25 20:22:26.000000000 +0800 +++ ./gdb/testsuite/ChangeLog 2013-08-16 11:49:02.000000000 +0800 @@ -1,3 +1,9 @@ +2013-08-14 Will Huang + + PR threads/15824 + * gdb.threads/manythreads.exp: Add test for get thread + original name + 2013-04-25 Sergio Durigan Junior * gdb.arch/arm-bl-branch-dest.c: New file. diff -uNr ./gdb.org/testsuite/gdb.threads/manythreads.exp ./gdb/testsuite/g= db.threads/manythreads.exp --- ./gdb.org/testsuite/gdb.threads/manythreads.exp 2013-01-01 14:41:27= .000000000 +0800 +++ ./gdb/testsuite/gdb.threads/manythreads.exp 2013-08-16 11:49:26.0000000= 00 +0800 @@ -93,6 +93,7 @@ } } +gdb_test "info threads" ".*manythreads.*" "check thread original name" gdb_test_no_output "thread name zardoz" "give a name to the thread" gdb_test "info threads" ".*zardoz.*" "check thread name" Thanks, Will Huang > -----Original Message----- > From: Yao Qi [mailto:yao@codesourcery.com] > Sent: Friday, August 16, 2013 10:26 AM > To: Will Huang > Cc: gdb-patches@sourceware.org > Subject: Re: [PATCH][PR threads/15824] when get threads name failed from = info > threads with linux kernel version earlier than 2.6.33 >=20 > Hi Will, > I am not the 'right' person to review this patch, however, I find some th= ing > obviously incorrect. I point them out here. >=20 > On 08/14/2013 12:26 PM, Will Huang wrote: > > diff -ruN ./gdb.org/ChangeLog ./gdb/ChangeLog > > --- ./gdb.org/ChangeLog 2013-04-26 19:43:30.000000000 +0800 > > +++ ./gdb/ChangeLog 2013-08-14 11:09:08.000000000 +0800 > > @@ -1,3 +1,11 @@ > > +2013-08-14 Will Huang > > + > > + PR threads/15824 >=20 > Each line should be prefixed with 'TAB' instead of 8 spaces. >=20 > > + * linux-nat.c (linux_nat_thread_name): Add get thread name from > > + procfs "/proc/$pid/task/$tid/status". > > + * commo/linux-procfs.c (linux_proc_get_thread_name): New. > > + * commo/linux-procfs.c (linux_proc_get_string): New. > ^^^^^^^^^^^^^^^^^^^^^^^ Duplicated. >=20 > Only have to mention the same file once. >=20 > > + > > 2013-04-26 Joel Brobecker > > > > * NEWS: Change "since GDB 7.5" into "in GDB 7.6". > > diff -ruN ./gdb.org/common/linux-procfs.c ./gdb/common/linux-procfs.c > > --- ./gdb.org/common/linux-procfs.c 2013-01-01 14:32:54.000000000 += 0800 > > +++ ./gdb/common/linux-procfs.c 2013-08-12 16:51:33.000000000 +0800 > > @@ -55,6 +55,51 @@ > > return retval; > > } > > > > +/* Return the string length of Name from/proc/$pid/task/$tid/status. >=20 > Space after "from" is needed. > s/Name/FIELD > s/$pid/LWPID > s/$tid/TID >=20 > > + Returns -1 if not found. */ >=20 > s/Returns/Return >=20 > > + > > +static int > > +linux_proc_get_string (pid_t lwpid, pid_t tid, > > + char *target, size_t t_size, const char > > +*field) >=20 > "char *target" should go to the previous line. >=20 > > +{ > > + size_t field_len =3D strlen (field); > > + FILE *status_file; > > + char buf[100]; > > + int retval =3D -1; > > + > > + if (tid > 0) > > + snprintf (buf, sizeof (buf), > > + "/proc/%d/task/%d/status", (int) lwpid, (int) tid); >=20 > Use xsnprintf, and the indentation looks wrong. >=20 > > + else > > + snprintf (buf, sizeof (buf), "/proc/%d/status", (int) lwpid); > > + >=20 > Likewise. Is it a dead path? Can 'tid' be negative? >=20 > > + status_file =3D fopen (buf, "r"); >=20 > Use gdb_fopen_cloexec? >=20 > > + if (status_file =3D=3D NULL) > > + { > > + warning (_("unable to open /proc file '%s'"), buf); > > + return -1; > > + } > > + > > + while (fgets (buf, sizeof (buf), status_file)) > > + if (strncmp (buf, field, field_len) =3D=3D 0 && buf[field_len] =3D= =3D ':') > > + { > > + size_t pos =3D field_len + 1; > > + while((buf[pos] =3D=3D ' ' || buf[pos] =3D=3D '\t') && (pos < s= izeof > > + (buf) - > > 1)) >=20 > Your mailer wrap this line... >=20 > > + pos++; > > + > > + strncpy (target, &buf[pos], t_size); > > + target[t_size - 1] =3D '\0'; > > + retval =3D strlen (target); > > + if(retval > 0 && target[retval - 1] =3D=3D '\n') > > + target[retval - 1] =3D '\0'; > > + > > + break; > > + } > > + > > + fclose (status_file); > > + return retval; > > +} > > + > > /* Return the TGID of LWPID from /proc/pid/status. Returns -1 if not > > found. */ > > > > @@ -118,3 +163,11 @@ > > { > > return linux_proc_pid_has_state (pid, "Z (zombie)"); > > } > > + > > +/*Get thread name identified by PID & TID*/ >=20 > I am not good at comments either, but I suggest >=20 > /* Save the name of thread PID/TID into buffer NAME. SIZE is the size > of buffer NAME. Return the length of the name if success, > otherwise return -1. */ >=20 > > + > > +int > > +linux_proc_get_thread_name (pid_t pid, pid_t tid, char *name, size_t > > +size) { > > + return linux_proc_get_string (pid, tid, name, size, "Name"); } > > diff -ruN ./gdb.org/common/linux-procfs.h ./gdb/common/linux-procfs.h > > --- ./gdb.org/common/linux-procfs.h 2013-01-01 14:32:54.000000000 > +0800 > > +++ ./gdb/common/linux-procfs.h 2013-08-12 16:50:39.000000000 +0800 > > @@ -40,4 +40,10 @@ > > > > extern int linux_proc_pid_is_zombie (pid_t pid); > > > > +/* Return -1 if thread Name of tid didn't found; >=20 > Comments should be updated as what I suggested above... >=20 > > + "char *name" for thread name (need be allocated by caller). */ > > + > > +extern int linux_proc_get_thread_name (pid_t pid, pid_t tid, > > + char *name, size_t size); > > + > > #endif /* COMMON_LINUX_PROCFS_H */ > > diff -ruN ./gdb.org/linux-nat.c ./gdb/linux-nat.c > > --- ./gdb.org/linux-nat.c 2013-02-13 22:59:49.000000000 +0800 > > +++ ./gdb/linux-nat.c 2013-08-12 16:47:26.000000000 +0800 > > @@ -4294,6 +4294,14 @@ > > > > fclose (comm_file); > > } > > + else > > + { > > + static char comm[COMM_LEN + 1]; > > + int size =3D linux_proc_get_thread_name (pid, lwp, comm, > > + COMM_LEN+1); >=20 > A blank line is needed. Space is needed before and after "+". >=20 > The type of pid is int and type of lwp is long, but the types of paramete= rs of > linux_proc_get_thread_name are pid_t and pid_t. It is inconsistent. >=20 > -- > Yao (=E9=BD=90=E5=B0=A7)