From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 120239 invoked by alias); 19 May 2015 11:42:05 -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 120228 invoked by uid 89); 19 May 2015 11:42:04 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 19 May 2015 11:42:01 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t4JBfu0A025472 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Tue, 19 May 2015 07:41:56 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t4JBfswq013298; Tue, 19 May 2015 07:41:54 -0400 Message-ID: <555B2181.60208@redhat.com> Date: Tue, 19 May 2015 11:42:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Daniel Colascione , gdb-patches Subject: Re: [PATCH] display names of remote threads References: <555520F6.7080400@dancol.org> In-Reply-To: <555520F6.7080400@dancol.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2015-05/txt/msg00471.txt.bz2 Hi Daniel, Thanks for doing this. On 05/14/2015 11:25 PM, Daniel Colascione wrote: > This patch adds an additional qXfer:threads:read attribute > for thread names. > > commit 9b8ccc79e8a522b20ac6413751fa488622131839 > Author: Daniel Colascione > Date: Thu May 14 15:24:08 2015 -0700 > > Display names of remote threads > Please add the info above to the commit log. Also please cross check https://sourceware.org/gdb/wiki/ContributionChecklist: - This being a new RSP packet feature, should have a NEWS entry. - Before/after gdb output would be good to have. - Missing ChangeLog. > diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo > index 4b76ce9..341ed82 100644 > --- a/gdb/doc/gdb.texinfo > +++ b/gdb/doc/gdb.texinfo > @@ -39111,17 +39111,19 @@ the following structure: > @smallexample > > > - > + > ... description ... > > > @end smallexample > > Each @samp{thread} element must have the @samp{id} attribute that > -identifies the thread (@pxref{thread-id syntax}). The > -@samp{core} attribute, if present, specifies which processor core > -the thread was last executing on. The content of the of @samp{thread} > -element is interpreted as human-readable auxilliary information. > +identifies the thread (@pxref{thread-id syntax}). The @samp{core} > +attribute, if present, specifies which processor core the thread was > +last executing on. The @samp{name} attribute, if present, specifies > +the human-readable name of the thread. The content of the of > +@samp{thread} element is interpreted as human-readable > +auxilliary information. > > @node Traceframe Info Format > @section Traceframe Info Format > diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c > index c6aa710..a72f862 100644 > --- a/gdb/gdbserver/linux-low.c > +++ b/gdb/gdbserver/linux-low.c > @@ -6255,6 +6255,7 @@ static struct target_ops linux_target_ops = { > NULL, > #endif > linux_supports_range_stepping, > + linux_common_name_of_thread, > }; > > static void > diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c > index 83529ff..7dc99f1 100644 > --- a/gdb/gdbserver/server.c > +++ b/gdb/gdbserver/server.c > @@ -1355,20 +1355,23 @@ handle_qxfer_threads_worker (struct > inferior_list_entry *inf, void *arg) > char ptid_s[100]; > int core = target_core_of_thread (ptid); > char core_s[21]; > + char *name = NULL; > > write_ptid (ptid_s, ptid); > > + buffer_xml_printf (buffer, " + > if (core != -1) > { > sprintf (core_s, "%d", core); > - buffer_xml_printf (buffer, "\n", > - ptid_s, core_s); > - } > - else > - { > - buffer_xml_printf (buffer, "\n", > - ptid_s); > + buffer_xml_printf (buffer, " core=\"%s\"", core_s); > } > + > + name = target_thread_name (ptid); > + if (name != NULL) > + buffer_xml_printf (buffer, " name=\"%s\"", name); > + > + buffer_xml_printf (buffer, "/>\n"); > } > > /* Helper for handle_qxfer_threads. */ > diff --git a/gdb/gdbserver/target.h b/gdb/gdbserver/target.h > index 126c861..edc547c 100644 > --- a/gdb/gdbserver/target.h > +++ b/gdb/gdbserver/target.h > @@ -394,6 +394,9 @@ struct target_ops > > /* Return true if target supports range stepping. */ > int (*supports_range_stepping) (void); > + > + /* Return name of thread if known. */ > + char *(*thread_name) (ptid_t); > }; > > extern struct target_ops *the_target; > @@ -569,6 +572,10 @@ ptid_t mywait (ptid_t ptid, struct > target_waitstatus *ourstatus, int options, > (the_target->core_of_thread ? (*the_target->core_of_thread) (ptid) \ > : -1) > > +#define target_thread_name(ptid) \ > + (the_target->thread_name ? (*the_target->thread_name) (ptid) \ > + : NULL) > + > int read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int > len); > > int write_inferior_memory (CORE_ADDR memaddr, const unsigned char *myaddr, > diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c > index 4a5a066..29fc340 100644 > --- a/gdb/linux-nat.c > +++ b/gdb/linux-nat.c > @@ -3924,38 +3924,7 @@ linux_nat_pid_to_str (struct target_ops *ops, > ptid_t ptid) > static char * > linux_nat_thread_name (struct target_ops *self, struct thread_info *thr) > { > - int pid = ptid_get_pid (thr->ptid); > - long lwp = ptid_get_lwp (thr->ptid); > -#define FORMAT "/proc/%d/task/%ld/comm" > - char buf[sizeof (FORMAT) + 30]; > - FILE *comm_file; > - char *result = NULL; > - > - snprintf (buf, sizeof (buf), FORMAT, pid, lwp); > - comm_file = gdb_fopen_cloexec (buf, "r"); > - if (comm_file) > - { > - /* Not exported by the kernel, so we define it here. */ > -#define COMM_LEN 16 > - static char line[COMM_LEN + 1]; > - > - if (fgets (line, sizeof (line), comm_file)) > - { > - char *nl = strchr (line, '\n'); > - > - if (nl) > - *nl = '\0'; > - if (*line != '\0') > - result = line; > - } > - > - fclose (comm_file); > - } > - > -#undef COMM_LEN > -#undef FORMAT > - > - return result; > + return linux_proc_tid_get_name (thr->ptid); > } > > /* Accepts an integer PID; Returns a string representing a file that > diff --git a/gdb/nat/linux-osdata.c b/gdb/nat/linux-osdata.c > index 0ed5d34..6cb01e6 100644 > --- a/gdb/nat/linux-osdata.c > +++ b/gdb/nat/linux-osdata.c > @@ -34,6 +34,7 @@ > > #include "xml-utils.h" > #include "buffer.h" > +#include "linux-procfs.h" > #include > #include > #include "filestuff.h" > @@ -109,6 +110,22 @@ linux_common_core_of_thread (ptid_t ptid) > return core; > } > I'd rather leave linux-osdata.c strictly for "info os foo" support, but linux_common_core_of_thread is here already, so fine to follow precedent. That said, how about making linux_proc_tid_get_name return a static buffer itself, and use that directly instead of going through linux_common_name_of_thread? Because ... > @@ -3924,38 +3924,7 @@ linux_nat_pid_to_str (struct target_ops *ops, > ptid_t ptid) > static char * > linux_nat_thread_name (struct target_ops *self, struct thread_info *thr) > { > - int pid = ptid_get_pid (thr->ptid); > - long lwp = ptid_get_lwp (thr->ptid); > -#define FORMAT "/proc/%d/task/%ld/comm" > - char buf[sizeof (FORMAT) + 30]; > - FILE *comm_file; > - char *result = NULL; > - > - snprintf (buf, sizeof (buf), FORMAT, pid, lwp); > - comm_file = gdb_fopen_cloexec (buf, "r"); > - if (comm_file) > - { > - /* Not exported by the kernel, so we define it here. */ > -#define COMM_LEN 16 > - static char line[COMM_LEN + 1]; > - > - if (fgets (line, sizeof (line), comm_file)) > - { > - char *nl = strchr (line, '\n'); > - > - if (nl) > - *nl = '\0'; > - if (*line != '\0') > - result = line; > - } > - > - fclose (comm_file); > - } > - > -#undef COMM_LEN > -#undef FORMAT > - > - return result; > + return linux_proc_tid_get_name (thr->ptid); > } ... it seems to me that before, this was returning a pointer to a static buffer, but after it returns returns a pointer to an allocated buffer, which ends up leaked by callers. > +char * > +linux_common_name_of_thread (ptid_t ptid) Misses intro comment. We prefer putting those in the .h file nowadays, and add: /* See linux-osdata.h. */ in the .c file. > +{ > + static char buf[16 /*kernel maximum */ + 1]; > + char* name = linux_proc_tid_get_name (ptid); 'char *name' > + if (name) if (name != NULL) > + { > + snprintf (buf, sizeof (buf), "%s", name); xsnprintf. > + free (name); > + } > + else > + buf[0] = '\0'; > + > + return buf; > +} > + > /* Finds the command-line of process PID and copies it into COMMAND. > At most MAXLEN characters are copied. If the command-line cannot > be found, PID is copied into command in text-form. */ > diff --git a/gdb/nat/linux-osdata.h b/gdb/nat/linux-osdata.h > index db8b445..1fdf367 100644 > --- a/gdb/nat/linux-osdata.h > +++ b/gdb/nat/linux-osdata.h > @@ -21,6 +21,7 @@ > #define COMMON_LINUX_OSDATA_H > > extern int linux_common_core_of_thread (ptid_t ptid); > +extern char *linux_common_name_of_thread (ptid_t ptid); > extern LONGEST linux_common_xfer_osdata (const char *annex, gdb_byte > *readbuf, > ULONGEST offset, ULONGEST len); > > diff --git a/gdb/nat/linux-procfs.c b/gdb/nat/linux-procfs.c > index 1e922b9..6cba0c2 100644 > --- a/gdb/nat/linux-procfs.c > +++ b/gdb/nat/linux-procfs.c > @@ -195,6 +195,38 @@ linux_proc_pid_get_ns (pid_t pid, const char *ns) > return NULL; > } > > +char * > +linux_proc_tid_get_name (ptid_t ptid) /* See foo.h. */ > +{ > + char buf[100]; > + char commbuf[64]; > + char* commval; Space before, not after '*': char *commval; > + FILE *comm_file; > + > + xsnprintf (buf, sizeof (buf), "/proc/%ld/task/%ld/comm", > + (long) ptid_get_pid (ptid), > + (long) (ptid_lwp_p (ptid) > + ? ptid_get_lwp (ptid) > + : ptid_get_pid (ptid))); > + > + comm_file = gdb_fopen_cloexec (buf, "r"); > + if (comm_file == NULL) > + return NULL; > + > + commval = fgets (commbuf, sizeof (commbuf), comm_file); > + fclose (comm_file); > + > + if (commval) if (commval != NULL) > + { > + if (commval[0] != '\0' && commval[strlen (commval) - 1] == '\n') > + commval[strlen (commval) - 1] = '\0'; > + return xstrdup (commval); > + } > + > + return NULL; > +} > + > + > /* See linux-procfs.h. */ > > void > diff --git a/gdb/nat/linux-procfs.h b/gdb/nat/linux-procfs.h > index 979ae0d..ef70aff 100644 > --- a/gdb/nat/linux-procfs.h > +++ b/gdb/nat/linux-procfs.h > @@ -58,6 +58,11 @@ extern int linux_proc_pid_is_gone (pid_t pid); > > extern char *linux_proc_pid_get_ns (pid_t pid, const char *ns); > > +/* Return an opaque string giving the thread's name or NULL if the > + information is unavailable. The returned string must be released > + with xfree. */ > +extern char *linux_proc_tid_get_name (ptid_t ptid); > + > /* Callback function for linux_proc_attach_tgid_threads. If the PTID > thread is not yet known, try to attach to it and return true, > otherwise return false. */ > diff --git a/gdb/remote.c b/gdb/remote.c > index 8f783a4..1f3c8b6 100644 > --- a/gdb/remote.c > +++ b/gdb/remote.c > @@ -382,6 +382,7 @@ struct remote_state > struct private_thread_info > { > char *extra; > + char *name; > int core; > }; > > @@ -389,6 +390,7 @@ static void > free_private_thread_info (struct private_thread_info *info) > { > xfree (info->extra); > + xfree (info->name); > xfree (info); > } > > @@ -1915,6 +1917,17 @@ remote_thread_alive (struct target_ops *ops, > ptid_t ptid) > return (rs->buf[0] == 'O' && rs->buf[1] == 'K'); > } > > +/* Return a pointer to a thread name if we know it and NULL otherwise. > + The thread_info object owns the memory for the name. */ > +static char* Add empty line between comment and function. Space before '*'. > +remote_thread_name (struct target_ops *ops, struct thread_info *info) > +{ > + if (info && info->priv) if (info != NULL && info->priv != NULL) > + return info->priv->name; > + > + return NULL; > +} > + > /* About these extended threadlist and threadinfo packets. They are > variable length packets but, the fields within them are often fixed > length. They are redundent enough to send over UDP as is the > @@ -2587,6 +2600,9 @@ typedef struct thread_item > /* The thread's extra info. May be NULL. */ > char *extra; > > + /* The thread's name. May be NULL. */ > + char *name; > + > /* The core the thread was running on. -1 if not known. */ > int core; > } thread_item_t; > @@ -2612,7 +2628,10 @@ clear_threads_listing_context (void *p) > struct thread_item *item; > > for (i = 0; VEC_iterate (thread_item_t, context->items, i, item); ++i) > - xfree (item->extra); > + { > + xfree (item->extra); > + xfree (item->name); > + } > > VEC_free (thread_item_t, context->items); > } > @@ -2683,6 +2702,9 @@ start_thread (struct gdb_xml_parser *parser, > else > item.core = -1; > > + attr = xml_find_attribute (attributes, "name"); > + item.name = attr ? xstrdup (attr->value) : NULL; attr != NULL. Thanks, Pedro Alves