Re: [PATCH v2 2/3] Display names of remote threads

[Pedro Alves <palves@redhat.com>]

On 11/25/2015 09:48 PM, Simon Marchi wrote:
> This patch adds support for thread names in the remote protocol, and
> updates gdb/gdbserver to use it.  The information is added to the XML
> description sent in response to the qXfer:threads:read packet.
> 
> Note for Eli:
> 
>   You commented on the original patch that the last sentence of the doc
>   paragraph doesn't make sense and should be removed.  Regardless if that
>   is true or not, the sentence was already there and is not added by this
>   patch.  I left it there for now, if we want to remove it, it will be in
>   a separate patch.
> 
> Note for Pedro:
> 
>   I think I went over all the comments you made on the original patch.
>   However, the code changed quite a bit, so it needs a full review anyway.
> 

Thanks!

> gdb/ChangeLog:
> 

You were probably already doing this, but since your submission process doesn't
show names in the ChangeLog entry, we can't tell -- if this was based on the
patch from Daniel, please make sure to also credit him in the ChangeLog.


> 	* linux-nat.c (linux_nat_thread_name): Replace implementation by call
> 	to linux_proc_tid_get_name.
> 	* nat/linux-procfs.c (linux_proc_tid_get_name): New function,
> 	implementation inspired by linux_nat_thread_name.
> 	* nat/linux-procfs.h (linux_proc_tid_get_name): New declaration.
> 	* remote.c (struct private_thread_info) <name>: New field.
> 	(free_private_thread_info): Free name field.
> 	(remote_thread_name): New function.
> 	(thread_item_t) <name>: New field.
> 	(clear_threads_listing_context): Free name field.
> 	(start_thread): Get name xml attribute.
> 	(thread_attributes): Add "name" attribute.
> 	(remote_update_thread_list): Copy name field.
> 	(init_remote_ops): Assign remote_thread_name callback.
> 	* target.h (target_thread_name): Update comment.
> 	* NEWS: Mention remote thread name support.
> 


> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -39542,7 +39542,7 @@ the following structure:
>  @smallexample
>  <?xml version="1.0"?>
>  <threads>
> -    <thread id="id" core="0">
> +    <thread id="id" core="0" name="name">
>      ... description ...
>      </thread>
>  </threads>
> @@ -39551,8 +39551,10 @@ the following structure:
>  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.
> +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.

"auxiliary", I think.


> diff --git a/gdb/remote.c b/gdb/remote.c
> index 2bbab62..71f8628 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -437,6 +437,7 @@ struct remote_state
>  struct private_thread_info
>  {
>    char *extra;
> +  char *name;
>    int core;
>  };
>  
> @@ -444,6 +445,7 @@ static void
>  free_private_thread_info (struct private_thread_info *info)
>  {
>    xfree (info->extra);
> +  xfree (info->name);
>    xfree (info);
>  }
>  
> @@ -2141,6 +2143,18 @@ 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 const char *
> +remote_thread_name (struct target_ops *ops, struct thread_info *info)
> +{
> +  if (info != NULL && info->priv != NULL)

I don't think info can be NULL (though info->priv can).  The linux-nat.c
version already assumes non-NULL.  I think other callbacks have that
extra check because they take a ptid as argument.

> +    return info->priv->name;
> +
> +  return NULL;
> +}

Otherwise looks good to me.

Thanks,
Pedro Alves