From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5246 invoked by alias); 16 Aug 2013 02:26:34 -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 5232 invoked by uid 89); 16 Aug 2013 02:26:33 -0000 X-Spam-SWARE-Status: No, score=-4.6 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL autolearn=ham version=3.3.2 Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 16 Aug 2013 02:26:32 +0000 Received: from svr-orw-fem-01.mgc.mentorg.com ([147.34.98.93]) by relay1.mentorg.com with esmtp id 1VA9kE-0003cB-5N from Yao_Qi@mentor.com ; Thu, 15 Aug 2013 19:26:30 -0700 Received: from SVR-ORW-FEM-03.mgc.mentorg.com ([147.34.97.39]) by svr-orw-fem-01.mgc.mentorg.com over TLS secured channel with Microsoft SMTPSVC(6.0.3790.4675); Thu, 15 Aug 2013 19:26:29 -0700 Received: from qiyao.dyndns.org (147.34.91.1) by svr-orw-fem-03.mgc.mentorg.com (147.34.97.39) with Microsoft SMTP Server id 14.2.247.3; Thu, 15 Aug 2013 19:26:27 -0700 Message-ID: <520D8D9A.3050003@codesourcery.com> Date: Fri, 16 Aug 2013 02:26:00 -0000 From: Yao Qi User-Agent: Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/20130110 Thunderbird/17.0.2 MIME-Version: 1.0 To: Will Huang CC: Subject: Re: [PATCH][PR threads/15824] when get threads name failed from info threads with linux kernel version earlier than 2.6.33 References: <00b201ce98a6$7cf69150$76e3b3f0$@huang@aliyun-inc.com> In-Reply-To: <00b201ce98a6$7cf69150$76e3b3f0$@huang@aliyun-inc.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-SW-Source: 2013-08/txt/msg00413.txt.bz2 Hi Will, I am not the 'right' person to review this patch, however, I find some thing obviously incorrect. I point them out here. 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 Each line should be prefixed with 'TAB' instead of 8 spaces. > + * 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. Only have to mention the same file once. > + > 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. Space after "from" is needed. s/Name/FIELD s/$pid/LWPID s/$tid/TID > + Returns -1 if not found. */ s/Returns/Return > + > +static int > +linux_proc_get_string (pid_t lwpid, pid_t tid, > + char *target, size_t t_size, const char *field) "char *target" should go to the previous line. > +{ > + size_t field_len = strlen (field); > + FILE *status_file; > + char buf[100]; > + int retval = -1; > + > + if (tid > 0) > + snprintf (buf, sizeof (buf), > + "/proc/%d/task/%d/status", (int) lwpid, (int) tid); Use xsnprintf, and the indentation looks wrong. > + else > + snprintf (buf, sizeof (buf), "/proc/%d/status", (int) lwpid); > + Likewise. Is it a dead path? Can 'tid' be negative? > + status_file = fopen (buf, "r"); Use gdb_fopen_cloexec? > + if (status_file == NULL) > + { > + warning (_("unable to open /proc file '%s'"), buf); > + return -1; > + } > + > + while (fgets (buf, sizeof (buf), status_file)) > + if (strncmp (buf, field, field_len) == 0 && buf[field_len] == ':') > + { > + size_t pos = field_len + 1; > + while((buf[pos] == ' ' || buf[pos] == '\t') && (pos < sizeof (buf) - > 1)) Your mailer wrap this line... > + pos++; > + > + strncpy (target, &buf[pos], t_size); > + target[t_size - 1] = '\0'; > + retval = strlen (target); > + if(retval > 0 && target[retval - 1] == '\n') > + target[retval - 1] = '\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*/ I am not good at comments either, but I suggest /* 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 -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; Comments should be updated as what I suggested above... > + "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 = linux_proc_get_thread_name (pid, lwp, comm, COMM_LEN+1); A blank line is needed. Space is needed before and after "+". The type of pid is int and type of lwp is long, but the types of parameters of linux_proc_get_thread_name are pid_t and pid_t. It is inconsistent. -- Yao (齐尧)