[PATCH v2 0/3] Remote thread name support

[PATCH v2 0/3] Remote thread name support

[Simon Marchi]

This patch set is a follow-up of the patch sent by Daniel Colascione:

https://sourceware.org/ml/gdb-patches/2015-05/msg00370.html

Simon Marchi (3):
  Constify thread name return path
  Display names of remote threads
  Add test for thread names

 gdb/NEWS                            |  5 +++
 gdb/doc/gdb.texinfo                 |  8 ++--
 gdb/gdbserver/linux-low.c           |  3 +-
 gdb/gdbserver/server.c              | 16 ++++----
 gdb/gdbserver/target.h              |  8 ++++
 gdb/linux-nat.c                     | 35 +---------------
 gdb/nat/linux-procfs.c              | 42 +++++++++++++++++++
 gdb/nat/linux-procfs.h              |  7 ++++
 gdb/python/py-infthread.c           |  2 +-
 gdb/remote.c                        | 29 +++++++++++++-
 gdb/target-delegates.c              | 10 ++---
 gdb/target.c                        |  2 +-
 gdb/target.h                        |  8 ++--
 gdb/testsuite/README                |  3 ++
 gdb/testsuite/gdb.threads/names.c   | 80 +++++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.threads/names.exp | 38 ++++++++++++++++++
 gdb/thread.c                        |  4 +-
 17 files changed, 242 insertions(+), 58 deletions(-)
 create mode 100644 gdb/testsuite/gdb.threads/names.c
 create mode 100644 gdb/testsuite/gdb.threads/names.exp

-- 
2.5.1

[PATCH v2 3/3] Add test for thread names

[Simon Marchi]

I couldn't find a test that verified the thread name functionality, so I
created a new one.

A target board can define gdb,no_thread_names if it doesn't support thread
names and wants to skip the tests that uses them.

This test has been made with Linux in mind.  Not all platforms use
pthread_setname_np to set the thread name, but some #ifdefs can be added
later in order to support other platforms.

Tested on x86-64 Ubuntu 14.04, native and remote.

gdb/testsuite/ChangeLog:

	* gdb.threads/names.exp: New file.
	* gdb.threads/names.c: New file.
	* README: Mention gdb,no_thread_names.
---
 gdb/testsuite/README                |  3 ++
 gdb/testsuite/gdb.threads/names.c   | 80 +++++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.threads/names.exp | 38 ++++++++++++++++++
 3 files changed, 121 insertions(+)
 create mode 100644 gdb/testsuite/gdb.threads/names.c
 create mode 100644 gdb/testsuite/gdb.threads/names.exp

diff --git a/gdb/testsuite/README b/gdb/testsuite/README
index 70f65cd..77ac74e 100644
--- a/gdb/testsuite/README
+++ b/gdb/testsuite/README
@@ -369,6 +369,9 @@ gdb,predefined_tsv
 
   The predefined trace state variables the board has.
 
+gdb,no_thread_names
+
+  The target doesn't support thread names.
 
 Testsuite Organization
 **********************
diff --git a/gdb/testsuite/gdb.threads/names.c b/gdb/testsuite/gdb.threads/names.c
new file mode 100644
index 0000000..6b08ff7
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/names.c
@@ -0,0 +1,80 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2015 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <unistd.h>
+#include <stdlib.h>
+#include <pthread.h>
+#include <assert.h>
+
+#define NUM_THREADS 3
+
+struct thread_data
+{
+  const char *name;
+  pthread_barrier_t *barrier;
+};
+
+static void *
+thread_func (void *varg)
+{
+  struct thread_data *arg = (struct thread_data *) varg;
+
+  pthread_setname_np (pthread_self (), arg->name);
+
+  pthread_barrier_wait (arg->barrier);
+
+  while (1)
+    sleep (1);
+}
+
+static void
+all_threads_ready (void)
+{
+}
+
+int
+main (int argc, char **argv)
+{
+  pthread_t threads[NUM_THREADS];
+  struct thread_data args[NUM_THREADS];
+  pthread_barrier_t barrier;
+  int i;
+  const char *names[] = { "carrot", "potato", "celery" };
+
+  /* Make sure that NAMES contains NUM_THREADS elements.  */
+  assert (sizeof (names) == sizeof(names[0]) * NUM_THREADS);
+
+  assert (0 == pthread_barrier_init (&barrier, NULL, NUM_THREADS + 1));
+
+  pthread_setname_np (pthread_self (), "main");
+
+  for (i = 0; i < NUM_THREADS; i++)
+    {
+      struct thread_data *arg = &args[i];
+
+      arg->name = names[i];
+      arg->barrier = &barrier;
+
+      assert (0 == pthread_create (&threads[i], NULL, thread_func, arg));
+    }
+
+  pthread_barrier_wait (&barrier);
+
+  all_threads_ready ();
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.threads/names.exp b/gdb/testsuite/gdb.threads/names.exp
new file mode 100644
index 0000000..b056df3
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/names.exp
@@ -0,0 +1,38 @@
+# Copyright (C) 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Verify that thread name features work properly (e.g. they show up in info
+# threads).
+
+if [target_info exists gdb,no_thread_names] {
+  continue
+}
+
+standard_testfile
+
+if [prepare_for_testing "failed to prepare" $testfile $srcfile {debug pthreads}] {
+    return -1
+}
+
+if ![runto "all_threads_ready"] {
+    continue
+}
+
+set expected_thread_list "\\* 1   .*\"main\" all_threads_ready.*\n"
+append expected_thread_list "  2   .*\"carrot\".*\n"
+append expected_thread_list "  3   .*\"potato\".*\n"
+append expected_thread_list "  4   .*\"celery\".*"
+
+gdb_test "info threads" "$expected_thread_list" "list threads"
-- 
2.5.1

Re: [PATCH v2 3/3] Add test for thread names

[Pedro Alves]

On 11/25/2015 09:48 PM, Simon Marchi wrote:

> +int
> +main (int argc, char **argv)
> +{
> +  pthread_t threads[NUM_THREADS];
> +  struct thread_data args[NUM_THREADS];
> +  pthread_barrier_t barrier;
> +  int i;
> +  const char *names[] = { "carrot", "potato", "celery" };

Add an alarm call so the process doesn't run forever if something
goes wrong and it gets detached / reparented to init.

> +
> +  /* Make sure that NAMES contains NUM_THREADS elements.  */
> +  assert (sizeof (names) == sizeof(names[0]) * NUM_THREADS);
> +
> +  assert (0 == pthread_barrier_init (&barrier, NULL, NUM_THREADS + 1));

There should be no side-effects in assert calls:

     res = pthread_barrier_init (&barrier, NULL, NUM_THREADS + 1));
     assert (res == 0);

> +
> +  pthread_setname_np (pthread_self (), "main");
> +
> +  for (i = 0; i < NUM_THREADS; i++)
> +    {
> +      struct thread_data *arg = &args[i];
> +
> +      arg->name = names[i];
> +      arg->barrier = &barrier;
> +
> +      assert (0 == pthread_create (&threads[i], NULL, thread_func, arg));

Ditto.

> +    }
> +
> +  pthread_barrier_wait (&barrier);
> +
> +  all_threads_ready ();
> +
> +  return 0;
> +}

OK with above fixed.

> +set expected_thread_list "\\* 1   .*\"main\" all_threads_ready.*\n"
> +append expected_thread_list "  2   .*\"carrot\".*\n"
> +append expected_thread_list "  3   .*\"potato\".*\n"
> +append expected_thread_list "  4   .*\"celery\".*"
> +
> +gdb_test "info threads" "$expected_thread_list" "list threads"

Note you can do this with multi_line.  E.g.:

gdb_test "info threads" \
    [multi_line \
         "\\* 1   .*\"main\" all_threads_ready.*" \
         "  2   .*\"carrot\".*" \
	 "  3   .*\"potato\".*" \
	 "  4   .*\"celery\".*"] \
    "list threads"

Thanks,
Pedro Alves

Re: [PATCH v2 3/3] Add test for thread names

[Simon Marchi]

On 15-11-26 06:45 AM, Pedro Alves wrote:
> On 11/25/2015 09:48 PM, Simon Marchi wrote:
> 
>> +int
>> +main (int argc, char **argv)
>> +{
>> +  pthread_t threads[NUM_THREADS];
>> +  struct thread_data args[NUM_THREADS];
>> +  pthread_barrier_t barrier;
>> +  int i;
>> +  const char *names[] = { "carrot", "potato", "celery" };
> 
> Add an alarm call so the process doesn't run forever if something
> goes wrong and it gets detached / reparented to init.

Since the main function doesn't wait forever, I don't know if it's necessary.
If gdb crashes and leaves the process running, the process should end by
itself (unless something is really wrong with the threads and the barrier).

I'll add it anyway, it doesn't hurt and it's a good practice.

>> +
>> +  /* Make sure that NAMES contains NUM_THREADS elements.  */
>> +  assert (sizeof (names) == sizeof(names[0]) * NUM_THREADS);
>> +
>> +  assert (0 == pthread_barrier_init (&barrier, NULL, NUM_THREADS + 1));
> 
> There should be no side-effects in assert calls:
> 
>      res = pthread_barrier_init (&barrier, NULL, NUM_THREADS + 1));
>      assert (res == 0);

Right, if asserts are disabled, that code is left out.  All fixed.

>> +
>> +  pthread_setname_np (pthread_self (), "main");
>> +
>> +  for (i = 0; i < NUM_THREADS; i++)
>> +    {
>> +      struct thread_data *arg = &args[i];
>> +
>> +      arg->name = names[i];
>> +      arg->barrier = &barrier;
>> +
>> +      assert (0 == pthread_create (&threads[i], NULL, thread_func, arg));
> 
> Ditto.
> 
>> +    }
>> +
>> +  pthread_barrier_wait (&barrier);
>> +
>> +  all_threads_ready ();
>> +
>> +  return 0;
>> +}
> 
> OK with above fixed.
> 
>> +set expected_thread_list "\\* 1   .*\"main\" all_threads_ready.*\n"
>> +append expected_thread_list "  2   .*\"carrot\".*\n"
>> +append expected_thread_list "  3   .*\"potato\".*\n"
>> +append expected_thread_list "  4   .*\"celery\".*"
>> +
>> +gdb_test "info threads" "$expected_thread_list" "list threads"
> 
> Note you can do this with multi_line.  E.g.:
> 
> gdb_test "info threads" \
>     [multi_line \
>          "\\* 1   .*\"main\" all_threads_ready.*" \
>          "  2   .*\"carrot\".*" \
> 	 "  3   .*\"potato\".*" \
> 	 "  4   .*\"celery\".*"] \
>     "list threads"

Ok.


I modified names.c significantly, so could you give it another quick look?

Changes:

- Added alarm call.
- Added some assert calls
- I was seeing some sigsegv and sigill in __run_exit_handlers when running the binary,
  I think it was because I was not joining the threads.  I added a second barrier call
  and am now joining the threads properly.
- Define _GNU_SOURCE to remove warning about pthread_setname_np.


From 41b75663742b06550911bdf2b6aee704fca66645 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Thu, 26 Nov 2015 09:49:04 -0500
Subject: [PATCH] Add test for thread names

I couldn't find a test that verified the thread name functionality, so I
created a new one.

A target board can define gdb,no_thread_names if it doesn't support thread
names and wants to skip the tests that uses them.

This test has been made with Linux in mind.  Not all platforms use
pthread_setname_np to set the thread name, but some #ifdefs can be added
later in order to support other platforms.

Tested on x86-64 Ubuntu 14.04, native and remote.

gdb/testsuite/ChangeLog:

	* gdb.threads/names.exp: New file.
	* gdb.threads/names.c: New file.
	* README: Mention gdb,no_thread_names.
---
 gdb/testsuite/ChangeLog             |  6 +++
 gdb/testsuite/README                |  3 ++
 gdb/testsuite/gdb.threads/names.c   | 97 +++++++++++++++++++++++++++++++++++++
 gdb/testsuite/gdb.threads/names.exp | 38 +++++++++++++++
 4 files changed, 144 insertions(+)
 create mode 100644 gdb/testsuite/gdb.threads/names.c
 create mode 100644 gdb/testsuite/gdb.threads/names.exp

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 9f6d7e6..461565f 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@
+2015-11-26  Simon Marchi  <simon.marchi@ericsson.com>
+
+	* gdb.threads/names.exp: New file.
+	* gdb.threads/names.c: New file.
+	* README: Mention gdb,no_thread_names.
+
 2015-11-26  Markus Metzger  <markus.t.metzger@intel.com>

 	PR 19297
diff --git a/gdb/testsuite/README b/gdb/testsuite/README
index 70f65cd..77ac74e 100644
--- a/gdb/testsuite/README
+++ b/gdb/testsuite/README
@@ -369,6 +369,9 @@ gdb,predefined_tsv

   The predefined trace state variables the board has.

+gdb,no_thread_names
+
+  The target doesn't support thread names.

 Testsuite Organization
 **********************
diff --git a/gdb/testsuite/gdb.threads/names.c b/gdb/testsuite/gdb.threads/names.c
new file mode 100644
index 0000000..130ddc1
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/names.c
@@ -0,0 +1,97 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2015 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#define _GNU_SOURCE
+#include <unistd.h>
+#include <stdlib.h>
+#include <pthread.h>
+#include <assert.h>
+
+#define NUM_THREADS 3
+
+struct thread_data
+{
+  const char *name;
+  pthread_barrier_t *barrier;
+};
+
+static void *
+thread_func (void *varg)
+{
+  struct thread_data *arg = (struct thread_data *) varg;
+  int res;
+
+  res = pthread_setname_np (pthread_self (), arg->name);
+  assert (res == 0);
+
+  pthread_barrier_wait (arg->barrier);
+
+  pthread_barrier_wait (arg->barrier);
+
+  return NULL;
+}
+
+static void
+all_threads_ready (void)
+{
+}
+
+int
+main (int argc, char **argv)
+{
+  pthread_t threads[NUM_THREADS];
+  struct thread_data args[NUM_THREADS];
+  pthread_barrier_t barrier;
+  int i, res;
+  const char *names[] = { "carrot", "potato", "celery" };
+
+  alarm (20);
+
+  /* Make sure that NAMES contains NUM_THREADS elements.  */
+  assert (sizeof (names) == sizeof (names[0]) * NUM_THREADS);
+
+  res = pthread_barrier_init (&barrier, NULL, NUM_THREADS + 1);
+  assert (res == 0);;
+
+  res = pthread_setname_np (pthread_self (), "main");
+  assert (res == 0);
+
+  for (i = 0; i < NUM_THREADS; i++)
+    {
+      struct thread_data *arg = &args[i];
+
+      arg->name = names[i];
+      arg->barrier = &barrier;
+
+      res = pthread_create (&threads[i], NULL, thread_func, arg);
+      assert (res == 0);
+    }
+
+  pthread_barrier_wait (&barrier);
+
+  all_threads_ready ();
+
+  pthread_barrier_wait (&barrier);
+
+  for (i = 0; i < NUM_THREADS; i++)
+    {
+      res = pthread_join (threads[i], NULL);
+      assert (res == 0);
+    }
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.threads/names.exp b/gdb/testsuite/gdb.threads/names.exp
new file mode 100644
index 0000000..f6bbdb4
--- /dev/null
+++ b/gdb/testsuite/gdb.threads/names.exp
@@ -0,0 +1,38 @@
+# Copyright (C) 2015 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Verify that thread name features work properly (e.g. they show up in info
+# threads).
+
+if [target_info exists gdb,no_thread_names] {
+    continue
+}
+
+standard_testfile
+
+if [prepare_for_testing "failed to prepare" $testfile $srcfile {debug pthreads}] {
+    return -1
+}
+
+if ![runto "all_threads_ready"] {
+    continue
+}
+
+gdb_test "info threads" \
+    [multi_line "\\* 1   .*\"main\" all_threads_ready.*" \
+		"  2   .*\"carrot\".*"  \
+		"  3   .*\"potato\".*"  \
+		"  4   .*\"celery\".*" ] \
+    "list threads"
-- 
2.5.1

Re: [PATCH v2 3/3] Add test for thread names

[Pedro Alves]

On 11/26/2015 03:59 PM, Simon Marchi wrote:

> I modified names.c significantly, so could you give it another quick look?

Looks great, thanks.

> +  /* Make sure that NAMES contains NUM_THREADS elements.  */
> +  assert (sizeof (names) == sizeof (names[0]) * NUM_THREADS);

BTW, the standard pattern would be:

  assert (sizeof (names) / sizeof (names[0]) == NUM_THREADS);

which at least my brain processes immediately like ARRAY_SIZE,
while with the * form, I have to stop and think.

Thanks,
Pedro Alves

Re: [PATCH v2 3/3] Add test for thread names

[Simon Marchi]

On 15-11-26 11:57 AM, Pedro Alves wrote:
> On 11/26/2015 03:59 PM, Simon Marchi wrote:
> 
>> I modified names.c significantly, so could you give it another quick look?
> 
> Looks great, thanks.
> 
>> +  /* Make sure that NAMES contains NUM_THREADS elements.  */
>> +  assert (sizeof (names) == sizeof (names[0]) * NUM_THREADS);
> 
> BTW, the standard pattern would be:
> 
>   assert (sizeof (names) / sizeof (names[0]) == NUM_THREADS);

You are right, it's better like that.

> which at least my brain processes immediately like ARRAY_SIZE,
> while with the * form, I have to stop and think.
> 
> Thanks,
> Pedro Alves

Pushed, thanks!

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

[Simon Marchi]

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.

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

gdb/gdbserver/ChangeLog:

	* linux-low.c (linux_target_ops): Use linux_proc_tid_get_name.
	* server.c (handle_qxfer_threads_worker): Refactor to include thread
	name in reply.
	* target.h (struct target_ops) <thread_name>: New field.
	(target_thread_name): New macro.

gdb/doc/ChangeLog:

	* gdb.texinfo (Thread List Format): Mention thread names.
---
 gdb/NEWS                  |  5 +++++
 gdb/doc/gdb.texinfo       |  8 +++++---
 gdb/gdbserver/linux-low.c |  3 ++-
 gdb/gdbserver/server.c    | 16 +++++++++-------
 gdb/gdbserver/target.h    |  8 ++++++++
 gdb/linux-nat.c           | 33 +--------------------------------
 gdb/nat/linux-procfs.c    | 42 ++++++++++++++++++++++++++++++++++++++++++
 gdb/nat/linux-procfs.h    |  7 +++++++
 gdb/remote.c              | 29 ++++++++++++++++++++++++++++-
 gdb/target.h              |  4 ++--
 10 files changed, 109 insertions(+), 46 deletions(-)

diff --git a/gdb/NEWS b/gdb/NEWS
index 31072b7..5f704fe 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -94,6 +94,11 @@ set remote exec-event-feature-packet
 show remote exec-event-feature-packet
   Set/show the use of the remote exec event feature.
 
+ * Thread names in remote protocol
+
+   The reply to qXfer:threads:read may now include a name attribute for each
+   thread.
+
 *** Changes in GDB 7.10
 
 * Support for process record-replay and reverse debugging on aarch64*-linux*
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 1917008..5840c47 100644
--- 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.
 
 @node Traceframe Info Format
 @section Traceframe Info Format
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index e3a56a7..a70868c 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -7043,7 +7043,8 @@ static struct target_ops linux_target_ops = {
   linux_mntns_unlink,
   linux_mntns_readlink,
   linux_breakpoint_kind_from_pc,
-  linux_sw_breakpoint_from_kind
+  linux_sw_breakpoint_from_kind,
+  linux_proc_tid_get_name,
 };
 
 static void
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 7d6c9cc..0105b99 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -1456,20 +1456,22 @@ 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];
+  const char *name = target_thread_name (ptid);
 
   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);
     }
+
+  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 358a8ab..8903da5 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -453,6 +453,10 @@ struct target_ops
      specific meaning like the Z0 kind parameter.
      SIZE is set to the software breakpoint's length in memory.  */
   const gdb_byte *(*sw_breakpoint_from_kind) (int kind, int *size);
+
+  /* Return the thread's name, or NULL if the target is unable to determine it.
+     The returned value must not be freed by the caller.  */
+  const char *(*thread_name) (ptid_t thread);
 };
 
 extern struct target_ops *the_target;
@@ -663,6 +667,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 2421687..9bc1324 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4100,38 +4100,7 @@ linux_nat_pid_to_str (struct target_ops *ops, ptid_t ptid)
 static const 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-procfs.c b/gdb/nat/linux-procfs.c
index 24bcb01..78cbb03 100644
--- a/gdb/nat/linux-procfs.c
+++ b/gdb/nat/linux-procfs.c
@@ -187,6 +187,48 @@ linux_proc_pid_is_zombie (pid_t pid)
 
 /* See linux-procfs.h.  */
 
+const char *
+linux_proc_tid_get_name (ptid_t ptid)
+{
+#define TASK_COMM_LEN 16  /* As defined in the kernel's sched.h.  */
+
+  static char comm_buf[TASK_COMM_LEN];
+  char comm_path[100];
+  FILE *comm_file;
+  const char *comm_val;
+  pid_t pid = ptid_get_pid (ptid);
+  pid_t tid = ptid_lwp_p (ptid) ? ptid_get_lwp (ptid) : ptid_get_pid (ptid);
+
+  xsnprintf (comm_path, sizeof (comm_path),
+	     "/proc/%ld/task/%ld/comm", (long) pid, (long) tid);
+
+  comm_file = gdb_fopen_cloexec (comm_path, "r");
+  if (comm_file == NULL)
+    return NULL;
+
+  comm_val = fgets (comm_buf, sizeof (comm_buf), comm_file);
+  fclose (comm_file);
+
+  if (comm_val != NULL)
+    {
+      int i;
+
+      /* Make sure there is no newline at the end.  */
+      for (i = 0; i < sizeof (comm_buf); i++)
+	{
+	  if (comm_buf[i] == '\n')
+	    {
+	      comm_buf[i] = '\0';
+	      break;
+	    }
+	}
+    }
+
+  return comm_val;
+}
+
+/* See linux-procfs.h.  */
+
 void
 linux_proc_attach_tgid_threads (pid_t pid,
 				linux_proc_attach_lwp_func attach_lwp)
diff --git a/gdb/nat/linux-procfs.h b/gdb/nat/linux-procfs.h
index f9cad39..f28c1ba 100644
--- a/gdb/nat/linux-procfs.h
+++ b/gdb/nat/linux-procfs.h
@@ -54,6 +54,13 @@ extern int linux_proc_pid_is_zombie_nowarn (pid_t pid);
 
 extern int linux_proc_pid_is_gone (pid_t pid);
 
+/* Return a string giving the thread's name or NULL if the
+   information is unavailable.  The returned value points to a statically
+   allocated buffer.  The value therefore becomes invalid at the next
+   linux_proc_tid_get_name call.  */
+
+extern const 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 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)
+    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
@@ -2821,6 +2835,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;
@@ -2847,7 +2864,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);
 }
@@ -2951,6 +2971,9 @@ start_thread (struct gdb_xml_parser *parser,
   else
     item.core = -1;
 
+  attr = xml_find_attribute (attributes, "name");
+  item.name = attr != NULL ? xstrdup (attr->value) : NULL;
+
   item.extra = 0;
 
   VEC_safe_push (thread_item_t, data->items, &item);
@@ -2971,6 +2994,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 }
 };
 
@@ -3149,6 +3173,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;
 	    }
 	}
     }
@@ -12738,6 +12764,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;
diff --git a/gdb/target.h b/gdb/target.h
index ac28a41..65b717c 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1820,8 +1820,8 @@ extern char *normal_pid_to_str (ptid_t ptid);
 #define target_extra_thread_info(TP) \
      (current_target.to_extra_thread_info (&current_target, TP))
 
-/* Return the thread's name.  A NULL result means that the target
-   could not determine this thread's name.  */
+/* Return the thread's name, or NULL if the target is unable to determine it.
+   The returned value must not be freed by the caller.  */
 
 extern const char *target_thread_name (struct thread_info *);
 
-- 
2.5.1

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

[Pedro Alves]

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

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

[Simon Marchi]

On 15-11-26 06:32 AM, Pedro Alves wrote:
> 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.

Yes of course.

I don't put the name and date line of the ChangeLog entry, because the date is
meaningless anyway.  In those cases where it's not just me, I could include it
to be explicit.

>> +auxilliary information.
> 
> "auxiliary", I think.

Right, fixed.

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

Ok, removed the info != NULL check.

>> +    return info->priv->name;
>> +
>> +  return NULL;
>> +}
> 
> Otherwise looks good to me.
> 
> Thanks,
> Pedro Alves
> 

Thanks, pushed with these fixes.


From 79efa585c51f0657b319beb1e213d5721eaacdcc Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Thu, 26 Nov 2015 09:49:04 -0500
Subject: [PATCH] Display names of remote threads

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.

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

gdb/gdbserver/ChangeLog:

	* linux-low.c (linux_target_ops): Use linux_proc_tid_get_name.
	* server.c (handle_qxfer_threads_worker): Refactor to include thread
	name in reply.
	* target.h (struct target_ops) <thread_name>: New field.
	(target_thread_name): New macro.

gdb/doc/ChangeLog:

	* gdb.texinfo (Thread List Format): Mention thread names.
---
 gdb/ChangeLog             | 20 ++++++++++++++++++++
 gdb/NEWS                  |  5 +++++
 gdb/doc/ChangeLog         |  4 ++++
 gdb/doc/gdb.texinfo       |  8 +++++---
 gdb/gdbserver/ChangeLog   |  9 +++++++++
 gdb/gdbserver/linux-low.c |  3 ++-
 gdb/gdbserver/server.c    | 16 +++++++++-------
 gdb/gdbserver/target.h    |  8 ++++++++
 gdb/linux-nat.c           | 33 +--------------------------------
 gdb/nat/linux-procfs.c    | 42 ++++++++++++++++++++++++++++++++++++++++++
 gdb/nat/linux-procfs.h    |  7 +++++++
 gdb/remote.c              | 29 ++++++++++++++++++++++++++++-
 gdb/target.h              |  4 ++--
 13 files changed, 142 insertions(+), 46 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1d9cb4f..0871500 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,23 @@
+2015-11-26  Daniel Colascione  <dancol@dancol.org>
+2015-11-26  Simon Marchi  <simon.marchi@ericsson.com>
+
+	* 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.
+
 2015-11-26  Simon Marchi  <simon.marchi@ericsson.com>

 	* linux-nat.c (linux_nat_thread_name): Constify return value.
diff --git a/gdb/NEWS b/gdb/NEWS
index 31072b7..5f704fe 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -94,6 +94,11 @@ set remote exec-event-feature-packet
 show remote exec-event-feature-packet
   Set/show the use of the remote exec event feature.

+ * Thread names in remote protocol
+
+   The reply to qXfer:threads:read may now include a name attribute for each
+   thread.
+
 *** Changes in GDB 7.10

 * Support for process record-replay and reverse debugging on aarch64*-linux*
diff --git a/gdb/doc/ChangeLog b/gdb/doc/ChangeLog
index 80df9ad..7e747dc 100644
--- a/gdb/doc/ChangeLog
+++ b/gdb/doc/ChangeLog
@@ -1,3 +1,7 @@
+2015-11-26  Simon Marchi  <simon.marchi@ericsson.com>
+
+	* gdb.texinfo (Thread List Format): Mention thread names.
+
 2015-11-24  Pedro Alves  <palves@redhat.com>

 	PR 17539
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 1917008..c4d8a18 100644
--- 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
+auxiliary information.

 @node Traceframe Info Format
 @section Traceframe Info Format
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index e265798..6e2a95d 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,3 +1,12 @@
+2015-11-26  Daniel Colascione  <dancol@dancol.org>
+2015-11-26  Simon Marchi  <simon.marchi@ericsson.com>
+
+	* linux-low.c (linux_target_ops): Use linux_proc_tid_get_name.
+	* server.c (handle_qxfer_threads_worker): Refactor to include thread
+	name in reply.
+	* target.h (struct target_ops) <thread_name>: New field.
+	(target_thread_name): New macro.
+
 2015-11-23  Joel Brobecker  <brobecker@adacore.com>

 	* regcache.h (regcache_invalidate_pid): Add declaration.
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index e3a56a7..a70868c 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -7043,7 +7043,8 @@ static struct target_ops linux_target_ops = {
   linux_mntns_unlink,
   linux_mntns_readlink,
   linux_breakpoint_kind_from_pc,
-  linux_sw_breakpoint_from_kind
+  linux_sw_breakpoint_from_kind,
+  linux_proc_tid_get_name,
 };

 static void
diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 7d6c9cc..0105b99 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -1456,20 +1456,22 @@ 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];
+  const char *name = target_thread_name (ptid);

   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);
     }
+
+  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 358a8ab..8903da5 100644
--- a/gdb/gdbserver/target.h
+++ b/gdb/gdbserver/target.h
@@ -453,6 +453,10 @@ struct target_ops
      specific meaning like the Z0 kind parameter.
      SIZE is set to the software breakpoint's length in memory.  */
   const gdb_byte *(*sw_breakpoint_from_kind) (int kind, int *size);
+
+  /* Return the thread's name, or NULL if the target is unable to determine it.
+     The returned value must not be freed by the caller.  */
+  const char *(*thread_name) (ptid_t thread);
 };

 extern struct target_ops *the_target;
@@ -663,6 +667,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 2421687..9bc1324 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4100,38 +4100,7 @@ linux_nat_pid_to_str (struct target_ops *ops, ptid_t ptid)
 static const 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-procfs.c b/gdb/nat/linux-procfs.c
index 24bcb01..78cbb03 100644
--- a/gdb/nat/linux-procfs.c
+++ b/gdb/nat/linux-procfs.c
@@ -187,6 +187,48 @@ linux_proc_pid_is_zombie (pid_t pid)

 /* See linux-procfs.h.  */

+const char *
+linux_proc_tid_get_name (ptid_t ptid)
+{
+#define TASK_COMM_LEN 16  /* As defined in the kernel's sched.h.  */
+
+  static char comm_buf[TASK_COMM_LEN];
+  char comm_path[100];
+  FILE *comm_file;
+  const char *comm_val;
+  pid_t pid = ptid_get_pid (ptid);
+  pid_t tid = ptid_lwp_p (ptid) ? ptid_get_lwp (ptid) : ptid_get_pid (ptid);
+
+  xsnprintf (comm_path, sizeof (comm_path),
+	     "/proc/%ld/task/%ld/comm", (long) pid, (long) tid);
+
+  comm_file = gdb_fopen_cloexec (comm_path, "r");
+  if (comm_file == NULL)
+    return NULL;
+
+  comm_val = fgets (comm_buf, sizeof (comm_buf), comm_file);
+  fclose (comm_file);
+
+  if (comm_val != NULL)
+    {
+      int i;
+
+      /* Make sure there is no newline at the end.  */
+      for (i = 0; i < sizeof (comm_buf); i++)
+	{
+	  if (comm_buf[i] == '\n')
+	    {
+	      comm_buf[i] = '\0';
+	      break;
+	    }
+	}
+    }
+
+  return comm_val;
+}
+
+/* See linux-procfs.h.  */
+
 void
 linux_proc_attach_tgid_threads (pid_t pid,
 				linux_proc_attach_lwp_func attach_lwp)
diff --git a/gdb/nat/linux-procfs.h b/gdb/nat/linux-procfs.h
index f9cad39..f28c1ba 100644
--- a/gdb/nat/linux-procfs.h
+++ b/gdb/nat/linux-procfs.h
@@ -54,6 +54,13 @@ extern int linux_proc_pid_is_zombie_nowarn (pid_t pid);

 extern int linux_proc_pid_is_gone (pid_t pid);

+/* Return a string giving the thread's name or NULL if the
+   information is unavailable.  The returned value points to a statically
+   allocated buffer.  The value therefore becomes invalid at the next
+   linux_proc_tid_get_name call.  */
+
+extern const 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 2bbab62..a80e548 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->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
@@ -2821,6 +2835,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;
@@ -2847,7 +2864,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);
 }
@@ -2951,6 +2971,9 @@ start_thread (struct gdb_xml_parser *parser,
   else
     item.core = -1;

+  attr = xml_find_attribute (attributes, "name");
+  item.name = attr != NULL ? xstrdup (attr->value) : NULL;
+
   item.extra = 0;

   VEC_safe_push (thread_item_t, data->items, &item);
@@ -2971,6 +2994,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 }
 };

@@ -3149,6 +3173,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;
 	    }
 	}
     }
@@ -12738,6 +12764,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;
diff --git a/gdb/target.h b/gdb/target.h
index ac28a41..65b717c 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1820,8 +1820,8 @@ extern char *normal_pid_to_str (ptid_t ptid);
 #define target_extra_thread_info(TP) \
      (current_target.to_extra_thread_info (&current_target, TP))

-/* Return the thread's name.  A NULL result means that the target
-   could not determine this thread's name.  */
+/* Return the thread's name, or NULL if the target is unable to determine it.
+   The returned value must not be freed by the caller.  */

 extern const char *target_thread_name (struct thread_info *);

-- 
2.5.1

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

[Pedro Alves]

On 11/26/2015 03:52 PM, Simon Marchi wrote:
> On 15-11-26 06:32 AM, Pedro Alves wrote:
>> 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.
> 
> Yes of course.
> 
> I don't put the name and date line of the ChangeLog entry, because the date is
> meaningless anyway.  In those cases where it's not just me, I could include it
> to be explicit.

Yeah, please.

> Thanks, pushed with these fixes.

Awesome!  We're getting close to local/remote parity.
Would you mind editing
<https://sourceware.org/gdb/wiki/LocalRemoteFeatureParity> to mention this
one is resolved in 7.11?

Thanks,
Pedro Alves

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

[Simon Marchi]

On 15-11-26 11:29 AM, Pedro Alves wrote:
> On 11/26/2015 03:52 PM, Simon Marchi wrote:
>> On 15-11-26 06:32 AM, Pedro Alves wrote:
>>> 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.
>>
>> Yes of course.
>>
>> I don't put the name and date line of the ChangeLog entry, because the date is
>> meaningless anyway.  In those cases where it's not just me, I could include it
>> to be explicit.
> 
> Yeah, please.
> 
>> Thanks, pushed with these fixes.
> 
> Awesome!  We're getting close to local/remote parity.
> Would you mind editing
> <https://sourceware.org/gdb/wiki/LocalRemoteFeatureParity> to mention this
> one is resolved in 7.11?
> 
> Thanks,
> Pedro Alves
> 

Done.

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

[Yao Qi]

Simon Marchi <simon.marchi@ericsson.com> writes:

> +  attr = xml_find_attribute (attributes, "name");
> +  item.name = attr != NULL ? xstrdup (attr->value) : NULL;
> +

This breaks the C++ build,

/home/yao/SourceCode/gnu/gdb/git/gdb/remote.c: In function ‘void start_thread(gdb_xml_parser*, const gdb_xml_element*, void*, VEC_gdb_xml_value_s*)’:
/home/yao/SourceCode/gnu/gdb/git/gdb/remote.c:2975:50: error: invalid conversion from ‘void*’ to ‘const char*’ [-fpermissive]
   item.name = attr != NULL ? xstrdup (attr->value) : NULL;
                                                  ^
In file included from /home/yao/SourceCode/gnu/gdb/git/gdb/common/common-defs.h:64:0,
                 from /home/yao/SourceCode/gnu/gdb/git/gdb/defs.h:28,
                 from /home/yao/SourceCode/gnu/gdb/git/gdb/remote.c:22:
/home/yao/SourceCode/gnu/gdb/git/gdb/../include/libiberty.h:323:14: note: initializing argument 1 of ‘char* xstrdup(const char*)’
 extern char *xstrdup (const char *) ATTRIBUTE_MALLOC ATTRIBUTE_RETURNS_NONNULL;

and I think you've already received the mail from buildbot.
http://gdb-build.sergiodj.net/builders/Fedora-x86_64-cxx-build-m64/builds/1413

-- 
Yao (齐尧)

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

[Simon Marchi]

On 15-11-27 09:37 AM, Yao Qi wrote:
> Simon Marchi <simon.marchi@ericsson.com> writes:
> 
>> +  attr = xml_find_attribute (attributes, "name");
>> +  item.name = attr != NULL ? xstrdup (attr->value) : NULL;
>> +
> 
> This breaks the C++ build,
> 
> /home/yao/SourceCode/gnu/gdb/git/gdb/remote.c: In function ‘void start_thread(gdb_xml_parser*, const gdb_xml_element*, void*, VEC_gdb_xml_value_s*)’:
> /home/yao/SourceCode/gnu/gdb/git/gdb/remote.c:2975:50: error: invalid conversion from ‘void*’ to ‘const char*’ [-fpermissive]
>    item.name = attr != NULL ? xstrdup (attr->value) : NULL;
>                                                   ^
> In file included from /home/yao/SourceCode/gnu/gdb/git/gdb/common/common-defs.h:64:0,
>                  from /home/yao/SourceCode/gnu/gdb/git/gdb/defs.h:28,
>                  from /home/yao/SourceCode/gnu/gdb/git/gdb/remote.c:22:
> /home/yao/SourceCode/gnu/gdb/git/gdb/../include/libiberty.h:323:14: note: initializing argument 1 of ‘char* xstrdup(const char*)’
>  extern char *xstrdup (const char *) ATTRIBUTE_MALLOC ATTRIBUTE_RETURNS_NONNULL;
> 
> and I think you've already received the mail from buildbot.
> http://gdb-build.sergiodj.net/builders/Fedora-x86_64-cxx-build-m64/builds/1413
> 


Ahh, silly me, I didn't try to build with C++.

Strange, I didn't receive anything from Buildbot.

I just pushed this to fix it:


From e19616610d7327664f99215a69cb326682742dc3 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@ericsson.com>
Date: Fri, 27 Nov 2015 10:14:42 -0500
Subject: [PATCH] remote.c: Add missing cast
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Fixes in C++:

/home/emaisin/src/binutils-gdb/gdb/remote.c: In function ‘void start_thread(gdb_xml_parser*, const gdb_xml_element*, void*, VEC_gdb_xml_value_s*)’:
/home/emaisin/src/binutils-gdb/gdb/remote.c:2975:59: error: invalid conversion from ‘void*’ to ‘const char*’ [-fpermissive]
   item.name = attr != NULL ? (char *) xstrdup (attr->value) : NULL;
                                                           ^
In file included from /home/emaisin/src/binutils-gdb/gdb/common/common-defs.h:64:0,
                 from /home/emaisin/src/binutils-gdb/gdb/defs.h:28,
                 from /home/emaisin/src/binutils-gdb/gdb/remote.c:22:
/home/emaisin/src/binutils-gdb/gdb/../include/libiberty.h:323:14: error:   initializing argument 1 of ‘char* xstrdup(const char*)’ [-fpermissive]
 extern char *xstrdup (const char *) ATTRIBUTE_MALLOC ATTRIBUTE_RETURNS_NONNULL;
              ^
make[2]: *** [remote.o] Error 1

gdb/ChangeLog:

	* remote.c (start_thread): Add cast.
---
 gdb/ChangeLog | 4 ++++
 gdb/remote.c  | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 54642e1..fd84223 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,7 @@
+2015-11-27  Simon Marchi  <simon.marchi@ericsson.com>
+
+	* remote.c (start_thread): Add cast.
+
 2015-11-27  Yao Qi  <yao.qi@linaro.org>

 	* nat/aarch64-linux-hw-point.c (aarch64_dr_state_remove_one_point):
diff --git a/gdb/remote.c b/gdb/remote.c
index a80e548..90be8b6 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -2972,7 +2972,7 @@ start_thread (struct gdb_xml_parser *parser,
     item.core = -1;

   attr = xml_find_attribute (attributes, "name");
-  item.name = attr != NULL ? xstrdup (attr->value) : NULL;
+  item.name = attr != NULL ? xstrdup ((const char *) attr->value) : NULL;

   item.extra = 0;

-- 
2.5.1

[commit] Fix build error (Re: [PATCH v2 2/3] Display names of remote threads)

[Ulrich Weigand]

Simon Marchi wrote:

> 	(thread_item_t) <name>: New field.

This causes build errors for me when using a GCC 4.4.7 host compiler:

gdb/remote.c: In function ‘remote_update_thread_list’:
gdb/remote.c:2855: error: ‘item.name’ may be used uninitialized in this function
gdb/remote.c:3079: note: ‘item.name’ was declared here

There were in fact two places where a struct thread_item with an uninitialized
name field was added to a vector.

I've checked in the following patch to fix the build error.

Bye,
Ulrich

ChangeLog:

	* remote.c (remote_newthread_step): Initialize item.name.
	(remote_get_threads_with_qthreadinfo): Likewise.

diff --git a/gdb/remote.c b/gdb/remote.c
index 9d44ce1..c60f173 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -2914,6 +2914,7 @@ remote_newthread_step (threadref *ref, void *data)
 
   item.ptid = ptid_build (pid, threadref_to_int (ref), 0);
   item.core = -1;
+  item.name = NULL;
   item.extra = NULL;
 
   VEC_safe_push (thread_item_t, context->items, &item);
@@ -3079,6 +3080,7 @@ remote_get_threads_with_qthreadinfo (struct target_ops *ops,
 
 		  item.ptid = read_ptid (bufp, &bufp);
 		  item.core = -1;
+		  item.name = NULL;
 		  item.extra = NULL;
 
 		  VEC_safe_push (thread_item_t, context->items, &item);

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [commit] Fix build error (Re: [PATCH v2 2/3] Display names of remote threads)

[Simon Marchi]

On 15-12-01 12:08 PM, Ulrich Weigand wrote:
> Simon Marchi wrote:
> 
>> 	(thread_item_t) <name>: New field.
> 
> This causes build errors for me when using a GCC 4.4.7 host compiler:
> 
> gdb/remote.c: In function â?~remote_update_thread_listâ?T:
> gdb/remote.c:2855: error: â?~item.nameâ?T may be used uninitialized in this function
> gdb/remote.c:3079: note: â?~item.nameâ?T was declared here
> 
> There were in fact two places where a struct thread_item with an uninitialized
> name field was added to a vector.
> 
> I've checked in the following patch to fix the build error.
> 
> Bye,
> Ulrich
> 
> ChangeLog:
> 
> 	* remote.c (remote_newthread_step): Initialize item.name.
> 	(remote_get_threads_with_qthreadinfo): Likewise.
> 
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 9d44ce1..c60f173 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -2914,6 +2914,7 @@ remote_newthread_step (threadref *ref, void *data)
>  
>    item.ptid = ptid_build (pid, threadref_to_int (ref), 0);
>    item.core = -1;
> +  item.name = NULL;
>    item.extra = NULL;
>  
>    VEC_safe_push (thread_item_t, context->items, &item);
> @@ -3079,6 +3080,7 @@ remote_get_threads_with_qthreadinfo (struct target_ops *ops,
>  
>  		  item.ptid = read_ptid (bufp, &bufp);
>  		  item.core = -1;
> +		  item.name = NULL;
>  		  item.extra = NULL;
>  
>  		  VEC_safe_push (thread_item_t, context->items, &item);
> 

Thanks!

[PATCH v2 1/3] Constify thread name return path

[Simon Marchi]

Since this code path returns a string owned by the target (we don't know how
it's allocated, could be a static read-only string), it's safer if we return
a constant string.  If, for some reasons, the caller wishes to modify the
string, it should make itself a copy.

gdb/ChangeLog:

	* linux-nat.c (linux_nat_thread_name): Constify return value.
	* target.h (struct target_ops) <to_thread_name>: Likewise.
	(target_thread_name): Likewise.
	* target.c (target_thread_name): Likewise.
	* target-delegates.c (debug_thread_name): Regenerate.
	* python/py-infthread.c (thpy_get_name): Constify local variables.
	* thread.c (print_thread_info): Likewise.
	(thread_find_command): Likewise.
---
 gdb/linux-nat.c           |  2 +-
 gdb/python/py-infthread.c |  2 +-
 gdb/target-delegates.c    | 10 +++++-----
 gdb/target.c              |  2 +-
 gdb/target.h              |  4 ++--
 gdb/thread.c              |  4 ++--
 6 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index 841ec39..2421687 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -4097,7 +4097,7 @@ linux_nat_pid_to_str (struct target_ops *ops, ptid_t ptid)
   return normal_pid_to_str (ptid);
 }
 
-static char *
+static const char *
 linux_nat_thread_name (struct target_ops *self, struct thread_info *thr)
 {
   int pid = ptid_get_pid (thr->ptid);
diff --git a/gdb/python/py-infthread.c b/gdb/python/py-infthread.c
index 4d0a020..e5db354 100644
--- a/gdb/python/py-infthread.c
+++ b/gdb/python/py-infthread.c
@@ -62,7 +62,7 @@ static PyObject *
 thpy_get_name (PyObject *self, void *ignore)
 {
   thread_object *thread_obj = (thread_object *) self;
-  char *name;
+  const char *name;
 
   THPY_REQUIRE_VALID (thread_obj);
 
diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c
index 253c9d7..b41b316 100644
--- a/gdb/target-delegates.c
+++ b/gdb/target-delegates.c
@@ -1530,23 +1530,23 @@ debug_extra_thread_info (struct target_ops *self, struct thread_info *arg1)
   return result;
 }
 
-static char *
+static const char *
 delegate_thread_name (struct target_ops *self, struct thread_info *arg1)
 {
   self = self->beneath;
   return self->to_thread_name (self, arg1);
 }
 
-static char *
+static const char *
 tdefault_thread_name (struct target_ops *self, struct thread_info *arg1)
 {
   return NULL;
 }
 
-static char *
+static const char *
 debug_thread_name (struct target_ops *self, struct thread_info *arg1)
 {
-  char * result;
+  const char * result;
   fprintf_unfiltered (gdb_stdlog, "-> %s->to_thread_name (...)\n", debug_target.to_shortname);
   result = debug_target.to_thread_name (&debug_target, arg1);
   fprintf_unfiltered (gdb_stdlog, "<- %s->to_thread_name (", debug_target.to_shortname);
@@ -1554,7 +1554,7 @@ debug_thread_name (struct target_ops *self, struct thread_info *arg1)
   fputs_unfiltered (", ", gdb_stdlog);
   target_debug_print_struct_thread_info_p (arg1);
   fputs_unfiltered (") = ", gdb_stdlog);
-  target_debug_print_char_p (result);
+  target_debug_print_const_char_p (result);
   fputs_unfiltered ("\n", gdb_stdlog);
   return result;
 }
diff --git a/gdb/target.c b/gdb/target.c
index 2365cd3..b43c12a 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -2265,7 +2265,7 @@ target_pid_to_str (ptid_t ptid)
   return (*current_target.to_pid_to_str) (&current_target, ptid);
 }
 
-char *
+const char *
 target_thread_name (struct thread_info *info)
 {
   return current_target.to_thread_name (&current_target, info);
diff --git a/gdb/target.h b/gdb/target.h
index e80bee5..ac28a41 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -639,7 +639,7 @@ struct target_ops
       TARGET_DEFAULT_FUNC (default_pid_to_str);
     char *(*to_extra_thread_info) (struct target_ops *, struct thread_info *)
       TARGET_DEFAULT_RETURN (NULL);
-    char *(*to_thread_name) (struct target_ops *, struct thread_info *)
+    const char *(*to_thread_name) (struct target_ops *, struct thread_info *)
       TARGET_DEFAULT_RETURN (NULL);
     void (*to_stop) (struct target_ops *, ptid_t)
       TARGET_DEFAULT_IGNORE ();
@@ -1823,7 +1823,7 @@ extern char *normal_pid_to_str (ptid_t ptid);
 /* Return the thread's name.  A NULL result means that the target
    could not determine this thread's name.  */
 
-extern char *target_thread_name (struct thread_info *);
+extern const char *target_thread_name (struct thread_info *);
 
 /* Attempts to find the pathname of the executable file
    that was run to create a specified process.
diff --git a/gdb/thread.c b/gdb/thread.c
index b47d990..f8103bd 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1122,7 +1122,7 @@ print_thread_info (struct ui_out *uiout, char *requested_threads, int pid)
   struct thread_info *tp;
   ptid_t current_ptid;
   struct cleanup *old_chain;
-  char *extra_info, *name, *target_id;
+  const char *extra_info, *name, *target_id;
   int current_thread = -1;
 
   update_thread_list ();
@@ -1781,7 +1781,7 @@ static void
 thread_find_command (char *arg, int from_tty)
 {
   struct thread_info *tp;
-  char *tmp;
+  const char *tmp;
   unsigned long match = 0;
 
   if (arg == NULL || *arg == '\0')
-- 
2.5.1

Re: [PATCH v2 1/3] Constify thread name return path

[Pedro Alves]

On 11/25/2015 09:48 PM, Simon Marchi wrote:
> Since this code path returns a string owned by the target (we don't know how
> it's allocated, could be a static read-only string), it's safer if we return
> a constant string.  If, for some reasons, the caller wishes to modify the
> string, it should make itself a copy.
> 
> gdb/ChangeLog:
> 
> 	* linux-nat.c (linux_nat_thread_name): Constify return value.
> 	* target.h (struct target_ops) <to_thread_name>: Likewise.
> 	(target_thread_name): Likewise.
> 	* target.c (target_thread_name): Likewise.
> 	* target-delegates.c (debug_thread_name): Regenerate.
> 	* python/py-infthread.c (thpy_get_name): Constify local variables.
> 	* thread.c (print_thread_info): Likewise.
> 	(thread_find_command): Likewise.

OK.

Thanks,
Pedro Alves

Re: [PATCH v2 1/3] Constify thread name return path

[Simon Marchi]

On 15-11-26 06:21 AM, Pedro Alves wrote:
> On 11/25/2015 09:48 PM, Simon Marchi wrote:
>> Since this code path returns a string owned by the target (we don't know how
>> it's allocated, could be a static read-only string), it's safer if we return
>> a constant string.  If, for some reasons, the caller wishes to modify the
>> string, it should make itself a copy.
>>
>> gdb/ChangeLog:
>>
>> 	* linux-nat.c (linux_nat_thread_name): Constify return value.
>> 	* target.h (struct target_ops) <to_thread_name>: Likewise.
>> 	(target_thread_name): Likewise.
>> 	* target.c (target_thread_name): Likewise.
>> 	* target-delegates.c (debug_thread_name): Regenerate.
>> 	* python/py-infthread.c (thpy_get_name): Constify local variables.
>> 	* thread.c (print_thread_info): Likewise.
>> 	(thread_find_command): Likewise.
> 
> OK.
> 
> Thanks,
> Pedro Alves
> 

Thanks, pushed.

gdb fails to build (was: Re: [PATCH v2 0/3] Remote thread name support)

[Tobias Burnus]

Hello all,

today, I fail to build gdb using GCC 6; looking at the changes, the culprit
seems to be this patch set. (I don't know whether it caused the issue or
just revealed it.) In any case, using yesterday's GDB with yesterday's GCC
worked.

The error I see is for remove.c - using stddef.h via common-defs.h:

gcc -g -O2   -I. -I../../gdb -I../../gdb/common -I../../gdb/config -DLOCALEDIR="\"gdb/gdb-inst/share/locale\"" -DHAVE_CONFIG_H -I../../gdb/../include/opcode -I../../gdb/../opcodes/.. -I../../gdb/../readline/.. -I../../gdb/../zlib -I../bfd -I../../gdb/../bfd -I../../gdb/../include -I../libdecnumber -I../../gdb/../libdecnumber  -I../../gdb/gnulib/import -Ibuild-gnulib/import   -DTUI=1  -Ipython/python/include/python2.7 -Ipython/python/include/python2.7 -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wpointer-sign -Wmissing-prototypes -Wdeclaration-after-statement -Wmissing-parameter-type -Wold-style-declaration -Wold-style-definition -Wformat-nonliteral -Werror -c -o remote.o -MT remote.o -MMD -MP -MF .deps/remote.Tpo ../../gdb/remote.c

In file included from build-gnulib/import/stdio.h:53:0,
                 from ../../gdb/common/common-defs.h:31,
                 from ../../gdb/defs.h:28,
                 from ../../gdb/remote.c:22:
build-gnulib/import/stddef.h:104:3: error: conflicting types for ‘max_align_t’
 } max_align_t;
   ^

In file included from build-gnulib/import/stddef.h:55:0,
                 from build-gnulib/import/stdio.h:53,
                 from ../../gdb/common/common-defs.h:31,
                 from ../../gdb/defs.h:28,
                 from ../../gdb/remote.c:22:
gcc/gcc-trunk/lib/gcc/x86_64-pc-linux-gnu/6.0.0/include/stddef.h:429:3: note: previous declaration of ‘max_align_t’ was here
 } max_align_t;
   ^


The definitions are as follows. GCC 6:

typedef struct {
  long long __max_align_ll __attribute__((__aligned__(__alignof__(long long))));
  long double __max_align_ld __attribute__((__aligned__(__alignof__(long double))));
} max_align_t;



GDB's build-gnulib:

typedef union
{
  char *__p _GL_STDDEF_ALIGNAS (char *);
  double __d _GL_STDDEF_ALIGNAS (double);
  long double __ld _GL_STDDEF_ALIGNAS (long double);
  long int __i _GL_STDDEF_ALIGNAS (long int);
} max_align_t;


Cheers,

Tobias

Re: gdb fails to build

[Simon Marchi]

On 15-11-27 08:31 AM, Tobias Burnus wrote:
> Hello all,
> 
> today, I fail to build gdb using GCC 6; looking at the changes, the culprit
> seems to be this patch set. (I don't know whether it caused the issue or
> just revealed it.) In any case, using yesterday's GDB with yesterday's GCC
> worked.
> 
> The error I see is for remove.c - using stddef.h via common-defs.h:
> 
> gcc -g -O2   -I. -I../../gdb -I../../gdb/common -I../../gdb/config -DLOCALEDIR="\"gdb/gdb-inst/share/locale\"" -DHAVE_CONFIG_H -I../../gdb/../include/opcode -I../../gdb/../opcodes/.. -I../../gdb/../readline/.. -I../../gdb/../zlib -I../bfd -I../../gdb/../bfd -I../../gdb/../include -I../libdecnumber -I../../gdb/../libdecnumber  -I../../gdb/gnulib/import -Ibuild-gnulib/import   -DTUI=1  -Ipython/python/include/python2.7 -Ipython/python/include/python2.7 -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wpointer-sign -Wmissing-prototypes -Wdeclaration-after-statement -Wmissing-parameter-type -Wold-style-declaration -Wold-style-definition -Wformat-nonliteral -Werror -c -o remote.o -MT remote.o -MMD -MP -MF .deps/remote.Tpo ../../gdb/remote.c
> 
> In file included from build-gnulib/import/stdio.h:53:0,
>                  from ../../gdb/common/common-defs.h:31,
>                  from ../../gdb/defs.h:28,
>                  from ../../gdb/remote.c:22:
> build-gnulib/import/stddef.h:104:3: error: conflicting types for ‘max_align_t’
>  } max_align_t;
>    ^
> 
> In file included from build-gnulib/import/stddef.h:55:0,
>                  from build-gnulib/import/stdio.h:53,
>                  from ../../gdb/common/common-defs.h:31,
>                  from ../../gdb/defs.h:28,
>                  from ../../gdb/remote.c:22:
> gcc/gcc-trunk/lib/gcc/x86_64-pc-linux-gnu/6.0.0/include/stddef.h:429:3: note: previous declaration of ‘max_align_t’ was here
>  } max_align_t;
>    ^
> 
> 
> The definitions are as follows. GCC 6:
> 
> typedef struct {
>   long long __max_align_ll __attribute__((__aligned__(__alignof__(long long))));
>   long double __max_align_ld __attribute__((__aligned__(__alignof__(long double))));
> } max_align_t;
> 
> 
> 
> GDB's build-gnulib:
> 
> typedef union
> {
>   char *__p _GL_STDDEF_ALIGNAS (char *);
>   double __d _GL_STDDEF_ALIGNAS (double);
>   long double __ld _GL_STDDEF_ALIGNAS (long double);
>   long int __i _GL_STDDEF_ALIGNAS (long int);
> } max_align_t;
> 
> 
> Cheers,
> 
> Tobias

I am trying to build gcc from the tree to see if I can reproduce the failure.  However,
I really can't see how this patch could cause this kind of failure...  We'll see.

Re: gdb fails to build

[Simon Marchi]

On 15-11-27 10:49 AM, Simon Marchi wrote:
> On 15-11-27 08:31 AM, Tobias Burnus wrote:
>> Hello all,
>>
>> today, I fail to build gdb using GCC 6; looking at the changes, the culprit
>> seems to be this patch set. (I don't know whether it caused the issue or
>> just revealed it.) In any case, using yesterday's GDB with yesterday's GCC
>> worked.
>>
>> The error I see is for remove.c - using stddef.h via common-defs.h:
>>
>> gcc -g -O2   -I. -I../../gdb -I../../gdb/common -I../../gdb/config -DLOCALEDIR="\"gdb/gdb-inst/share/locale\"" -DHAVE_CONFIG_H -I../../gdb/../include/opcode -I../../gdb/../opcodes/.. -I../../gdb/../readline/.. -I../../gdb/../zlib -I../bfd -I../../gdb/../bfd -I../../gdb/../include -I../libdecnumber -I../../gdb/../libdecnumber  -I../../gdb/gnulib/import -Ibuild-gnulib/import   -DTUI=1  -Ipython/python/include/python2.7 -Ipython/python/include/python2.7 -Wall -Wpointer-arith -Wno-unused -Wunused-value -Wunused-function -Wno-switch -Wno-char-subscripts -Wempty-body -Wpointer-sign -Wmissing-prototypes -Wdeclaration-after-statement -Wmissing-parameter-type -Wold-style-declaration -Wold-style-definition -Wformat-nonliteral -Werror -c -o remote.o -MT remote.o -MMD -MP -MF .deps/remote.Tpo ../../gdb/remote.c
>>
>> In file included from build-gnulib/import/stdio.h:53:0,
>>                  from ../../gdb/common/common-defs.h:31,
>>                  from ../../gdb/defs.h:28,
>>                  from ../../gdb/remote.c:22:
>> build-gnulib/import/stddef.h:104:3: error: conflicting types for ‘max_align_t’
>>  } max_align_t;
>>    ^
>>
>> In file included from build-gnulib/import/stddef.h:55:0,
>>                  from build-gnulib/import/stdio.h:53,
>>                  from ../../gdb/common/common-defs.h:31,
>>                  from ../../gdb/defs.h:28,
>>                  from ../../gdb/remote.c:22:
>> gcc/gcc-trunk/lib/gcc/x86_64-pc-linux-gnu/6.0.0/include/stddef.h:429:3: note: previous declaration of ‘max_align_t’ was here
>>  } max_align_t;
>>    ^
>>
>>
>> The definitions are as follows. GCC 6:
>>
>> typedef struct {
>>   long long __max_align_ll __attribute__((__aligned__(__alignof__(long long))));
>>   long double __max_align_ld __attribute__((__aligned__(__alignof__(long double))));
>> } max_align_t;
>>
>>
>>
>> GDB's build-gnulib:
>>
>> typedef union
>> {
>>   char *__p _GL_STDDEF_ALIGNAS (char *);
>>   double __d _GL_STDDEF_ALIGNAS (double);
>>   long double __ld _GL_STDDEF_ALIGNAS (long double);
>>   long int __i _GL_STDDEF_ALIGNAS (long int);
>> } max_align_t;
>>
>>
>> Cheers,
>>
>> Tobias
> 
> I am trying to build gcc from the tree to see if I can reproduce the failure.  However,
> I really can't see how this patch could cause this kind of failure...  We'll see.

I just tested building gdb (622b9eb1a6047bd3ad3e1a3f120cf7318ac25b57) using gcc master:

/opt/gcc/bin/gcc -v
Using built-in specs.
COLLECT_GCC=/opt/gcc/bin/gcc
COLLECT_LTO_WRAPPER=/opt/gcc/libexec/gcc/x86_64-pc-linux-gnu/6.0.0/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: /home/emaisin/src/gcc/configure --enable-languages=c,c++ --disable-bootstrap --prefix=/opt/gcc
Thread model: posix
gcc version 6.0.0 20151127 (experimental) (GCC)


and it works fine.  Have you done a build from scratch or an incremental one?  I would suggest
retrying from scratch.  If you try to incrementally build, but with different compilers, then the
options configure found for the first compiler (such as whether stddef.h defines max_align_t)
might not work for the second compiler.

In gdb/build-gnulib/config.log, I have:

 965 configure:16981: checking for max_align_t
 966 configure:16981: result: yes

I suppose that yours says "no".

Simon

Re: gdb fails to build

[Tobias Burnus]

On Fri, Nov 27, 2015 at 02:05:53PM -0500, Simon Marchi wrote:
> I just tested building gdb (622b9eb1a6047bd3ad3e1a3f120cf7318ac25b57) using gcc master:
> /opt/gcc/bin/gcc -v
> gcc version 6.0.0 20151127 (experimental) (GCC)
> and it works fine.  Have you done a build from scratch or an incremental one?
>
> In gdb/build-gnulib/config.log, I have:
> 
>  965 configure:16981: checking for max_align_t
>  966 configure:16981: result: yes
> 
> I suppose that yours says "no".

I think I found the issue of mine: I accidentally start a new
shell in my build script:
  time (make "$@" && make install)
which overrides the set PATH - such that ./configure runs with
/usr/bin/gcc while make runs with gcc 6.

Thus, I had "result: no" but before your patch, it didn't fail.

Sorry for the false alarm!

 * * *

Still, using GCC 6 through out still fails - albeit due to an other issue:

make[4]: Entering directory `/data/local_users/tobiasb/gdb/binutils-gdb/build/gold'
g++ -DHAVE_CONFIG_H -I. -I../../gold  -I../../gold -I../../gold/../include -I../../gold/../elfcpp -DLOCALEDIR="\"/data/local_users/tobiasb/gdb/gdb-inst/share/locale\"" -DBINDIR="\"/data/local_users/tobiasb/gdb/gdb-inst/bin\"" -DTOOLBINDIR="\"/data/local_users/tobiasb/gdb/gdb-inst/x86_64-pc-linux-gnu/bin\"" -DTOOLLIBDIR="\"/data/local_users/tobiasb/gdb/gdb-inst/x86_64-pc-linux-gnu/lib\""   -W -Wall    -Werror -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -frandom-seed=dirsearch.o -I../../gold/../zlib -g -O2 -MT dirsearch.o -MD -MP -MF .deps/dirsearch.Tpo -c -o dirsearch.o ../../gold/dirsearch.cc
../../gold/dirsearch.cc:125:1: error: ‘{anonymous}::Dir_caches::~Dir_caches()’ defined but not used [-Werror=unused-function]
 Dir_caches::~Dir_caches()
 ^~~~~~~~~~

That's with GDB 622b9eb1a6047bd3ad3e1a3f120cf7318ac25b57
and GCC 335ce86cb6cea8046993ab93d573316fd9ff798c (r231023).

I wonder why that didn't show up with before
when configuring with GCC 4.4 and building GCC 6.

I assume it didn't show up for you as you didn't use
'--enable-ld --enable-gold' - or did you also build gold?

Cheers,

Tobias

Re: gdb fails to build

[Simon Marchi]

On 15-11-27 05:35 PM, Tobias Burnus wrote:
> On Fri, Nov 27, 2015 at 02:05:53PM -0500, Simon Marchi wrote:
>> I just tested building gdb (622b9eb1a6047bd3ad3e1a3f120cf7318ac25b57) using gcc master:
>> /opt/gcc/bin/gcc -v
>> gcc version 6.0.0 20151127 (experimental) (GCC)
>> and it works fine.  Have you done a build from scratch or an incremental one?
>>
>> In gdb/build-gnulib/config.log, I have:
>>
>>  965 configure:16981: checking for max_align_t
>>  966 configure:16981: result: yes
>>
>> I suppose that yours says "no".
> 
> I think I found the issue of mine: I accidentally start a new
> shell in my build script:
>   time (make "$@" && make install)
> which overrides the set PATH - such that ./configure runs with
> /usr/bin/gcc while make runs with gcc 6.
> 
> Thus, I had "result: no" but before your patch, it didn't fail.
> 
> Sorry for the false alarm!

Ah that makes sense.

>  * * *
> 
> Still, using GCC 6 through out still fails - albeit due to an other issue:
> 
> make[4]: Entering directory `/data/local_users/tobiasb/gdb/binutils-gdb/build/gold'
> g++ -DHAVE_CONFIG_H -I. -I../../gold  -I../../gold -I../../gold/../include -I../../gold/../elfcpp -DLOCALEDIR="\"/data/local_users/tobiasb/gdb/gdb-inst/share/locale\"" -DBINDIR="\"/data/local_users/tobiasb/gdb/gdb-inst/bin\"" -DTOOLBINDIR="\"/data/local_users/tobiasb/gdb/gdb-inst/x86_64-pc-linux-gnu/bin\"" -DTOOLLIBDIR="\"/data/local_users/tobiasb/gdb/gdb-inst/x86_64-pc-linux-gnu/lib\""   -W -Wall    -Werror -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -frandom-seed=dirsearch.o -I../../gold/../zlib -g -O2 -MT dirsearch.o -MD -MP -MF .deps/dirsearch.Tpo -c -o dirsearch.o ../../gold/dirsearch.cc
> ../../gold/dirsearch.cc:125:1: error: ‘{anonymous}::Dir_caches::~Dir_caches()’ defined but not used [-Werror=unused-function]
>  Dir_caches::~Dir_caches()
>  ^~~~~~~~~~
> 
> That's with GDB 622b9eb1a6047bd3ad3e1a3f120cf7318ac25b57
> and GCC 335ce86cb6cea8046993ab93d573316fd9ff798c (r231023).
> 
> I wonder why that didn't show up with before
> when configuring with GCC 4.4 and building GCC 6.
> 
> I assume it didn't show up for you as you didn't use
> '--enable-ld --enable-gold' - or did you also build gold?
> 
> Cheers,
> 
> Tobias
> 

No I don't build gold.  I generally use the following configure line.  Since I am interested
in gdb, I disable pretty much everything else.

~/src/binutils-gdb/configure \
  --enable-targets=all \
  CFLAGS="-g3 -O0" CXXFLAGS="-g3 -O0" \
  --with-python=/usr/bin/python3 \
  --disable-ld --disable-gold --disable-gas --disable-binutils --disable-gprof

I can also see some similar warnings causing failure in bfd and opcodes, which is required to
build gdb (things like unused function/variable, variable may be used before assigned).  I then
go in the offending subdirectory and build the file without -Werror.  For you example, it would
be:

  make CFLAGS="-Wno-error" dirsearch.o

in $(builddir)/gold.  The right thing to do, however, would be to fix the warning at the source.
So if time was not an issue, I would go and fix all these little warnings, but unfortunately I
don't have that luxury :(.