display names of remote threads

  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

    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;
+
   item.extra = 0;

   VEC_safe_push (thread_item_t, data->items, &item);
@@ -2702,6 +2724,7 @@ end_thread (struct gdb_xml_parser *parser,
 const struct gdb_xml_attribute thread_attributes[] = {
   { "id", GDB_XML_AF_NONE, NULL, NULL },
   { "core", GDB_XML_AF_OPTIONAL, gdb_xml_parse_attr_ulongest, NULL },
+  { "name", GDB_XML_AF_OPTIONAL, NULL, NULL },
   { NULL, GDB_XML_AF_NONE, NULL, NULL }
 };

@@ -2875,6 +2898,8 @@ remote_update_thread_list (struct target_ops *ops)
 	      info->core = item->core;
 	      info->extra = item->extra;
 	      item->extra = NULL;
+	      info->name = item->name;
+	      item->name = NULL;
 	    }
 	}
     }
@@ -11729,6 +11754,7 @@ Specify the serial device it is connected to\n\
   remote_ops.to_pass_signals = remote_pass_signals;
   remote_ops.to_program_signals = remote_program_signals;
   remote_ops.to_thread_alive = remote_thread_alive;
+  remote_ops.to_thread_name = remote_thread_name;
   remote_ops.to_update_thread_list = remote_update_thread_list;
   remote_ops.to_pid_to_str = remote_pid_to_str;
   remote_ops.to_extra_thread_info = remote_threads_extra_info;
  

> Date: Thu, 14 May 2015 15:25:58 -0700
> From: Daniel Colascione <dancol@dancol.org>
> 
> commit 9b8ccc79e8a522b20ac6413751fa488622131839
> Author: Daniel Colascione <dancol@dancol.org>
> Date:   Thu May 14 15:24:08 2015 -0700
> 
>     Display names of remote threads

Please also include changes to corresponding ChangeLog files.

> 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.

Something is wrong with the last sentence.  (I actually wonder why
that sentence is needed: it's not clear to me what does it want to
convey.)

The documentation part is OK with this fixed.
  

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