This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] display names of remote threads
- From: Pedro Alves <palves at redhat dot com>
- To: Daniel Colascione <dancol at dancol dot org>, gdb-patches <gdb-patches at sourceware dot org>
- Date: Tue, 19 May 2015 12:41:53 +0100
- Subject: Re: [PATCH] display names of remote threads
- Authentication-results: sourceware.org; auth=none
- References: <555520F6 dot 7080400 at dancol dot org>
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 <dancol@dancol.org>
> 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
> <?xml version="1.0"?>
> <threads>
> - <thread id="id" core="0">
> + <thread id="id" core="0" name="name">
> ... description ...
> </thread>
> </threads>
> @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, "<thread id=\"%s\"", ptid_s);
> +
> if (core != -1)
> {
> sprintf (core_s, "%d", core);
> - buffer_xml_printf (buffer, "<thread id=\"%s\" core=\"%s\"/>\n",
> - ptid_s, core_s);
> - }
> - else
> - {
> - buffer_xml_printf (buffer, "<thread id=\"%s\"/>\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 <dirent.h>
> #include <sys/stat.h>
> #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