This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2 2/3] Display names of remote threads
- From: Pedro Alves <palves at redhat dot com>
- To: Simon Marchi <simon dot marchi at ericsson dot com>, gdb-patches at sourceware dot org
- Date: Thu, 26 Nov 2015 11:32:08 +0000
- Subject: Re: [PATCH v2 2/3] Display names of remote threads
- Authentication-results: sourceware.org; auth=none
- References: <1448488138-2360-1-git-send-email-simon dot marchi at ericsson dot com> <1448488138-2360-3-git-send-email-simon dot marchi at ericsson dot 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