[PATCH 2/2] Improve alignment of "info threads" output, align "Target Id" column

[PATCH 2/2] Improve alignment of "info threads" output, align "Target Id" column

[Pedro Alves]

It's long annoyed me that "info threads"'s columns are misaligned.

Particularly the "Target Id" column's content is usually longer than
the specified column width, so the table ends up with the "Frame"
column misaligned.  For example, currently we get this:

 (gdb) info threads
   Id   Target Id         Frame
   1    Thread 0x7ffff7fb5740 (LWP 9056) "threads" 0x00007ffff7bc28ad in __pthread_join (threadid=140737345763072, thread_return=0x7fffffffd3e8) at pthread_join.c:90
   2    Thread 0x7ffff7803700 (LWP 9060) "function0" thread_function0 (arg=0x0) at threads.c:90
 * 3    Thread 0x7ffff7002700 (LWP 9061) "threads" thread_function1 (arg=0x1) at threads.c:106

The fact that the "Frame" heading is in a weird spot is particularly
annoying.

This commit turns the above into into this:

 (gdb) info threads
   Id   Target Id                                    Frame
   1    Thread 0x7ffff7fb5740 (LWP 7548) "threads"   0x00007ffff7bc28ad in __pthread_join (threadid=140737345763072, thread_return=0x7fffffffd3e8) at pthread_join.c:90
   2    Thread 0x7ffff7803700 (LWP 7555) "function0" thread_function0 (arg=0x0) at threads.c:91
 * 3    Thread 0x7ffff7002700 (LWP 7557) "threads"   thread_function1 (arg=0x1) at threads.c:104

It does that by computing the max width of the "Target Id" column and
using that as column width when creating the table.

This results in calling several target_pid_to_str /
target_extra_thread_info / target_thread_name twice for each thread,
but I think that it doesn't matter in practice performance-wise,
because the remote target caches the info, and with native targets it
shouldn't be noticeable.  It could matter if we have many threads
(say, thousands), but then "info threads" is practically useless in
such a scenario anyway -- better thread filtering and aggregation
would be necessary.

(Note: I have an old branch somewhere where I attempted at making
gdb's "info threads"-like tables follow a model/view design, so that a
general framework took care of issues like these, but it's incomplete
and a much bigger change.  This patch doesn't prevent going in that
direction in the future, of course.)

gdb/ChangeLog:
2018-06-08  Pedro Alves  <palves@redhat.com>

	* thread.c (thread_target_id_str): New, factored out from ...
	(print_thread_info_1): ... here.  Use it to compute the max
	"Target ID" column width.

gdb/testsuite/ChangeLog:
2018-06-08  Pedro Alves  <palves@redhat.com>

	* gdb.threads/names.exp: Adjust expected "info threads" output.
---
 gdb/testsuite/gdb.threads/names.exp |  2 +-
 gdb/thread.c                        | 61 ++++++++++++++++++++++++-------------
 2 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/gdb/testsuite/gdb.threads/names.exp b/gdb/testsuite/gdb.threads/names.exp
index 44e21e65e9..fbc10f5506 100644
--- a/gdb/testsuite/gdb.threads/names.exp
+++ b/gdb/testsuite/gdb.threads/names.exp
@@ -31,7 +31,7 @@ if ![runto "all_threads_ready"] {
 }
 
 gdb_test "info threads" \
-    [multi_line "\\* 1   .*\"main\" all_threads_ready.*" \
+    [multi_line "\\* 1   .*\"main\"\[ \]\+all_threads_ready.*" \
 		"  2   .*\"carrot\".*"  \
 		"  3   .*\"potato\".*"  \
 		"  4   .*\"celery\".*" ] \
diff --git a/gdb/thread.c b/gdb/thread.c
index f5a29f5cc1..4b52a58cf6 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1129,6 +1129,26 @@ should_print_thread (const char *requested_threads, int default_inf_num,
   return 1;
 }
 
+/* Return the string to display in "info threads"'s "Target Id"
+   column, for TP.  */
+
+static std::string
+thread_target_id_str (thread_info *tp)
+{
+  const char *target_id = target_pid_to_str (tp->ptid);
+  const char *extra_info = target_extra_thread_info (tp);
+  const char *name = tp->name != nullptr ? tp->name : target_thread_name (tp);
+
+  if (extra_info != nullptr && name != nullptr)
+    return string_printf ("%s \"%s\" (%s)", target_id, name, extra_info);
+  else if (extra_info != nullptr)
+    return string_printf ("%s (%s)", target_id, extra_info);
+  else if (name != nullptr)
+    return string_printf ("%s \"%s\"", target_id, name);
+  else
+    return target_id;
+}
+
 /* Like print_thread_info, but in addition, GLOBAL_IDS indicates
    whether REQUESTED_THREADS is a list of global or per-inferior
    thread ids.  */
@@ -1140,7 +1160,6 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads,
 {
   struct thread_info *tp;
   ptid_t current_ptid;
-  const char *extra_info, *name, *target_id;
   struct inferior *inf;
   int default_inf_num = current_inferior ()->num;
 
@@ -1159,6 +1178,7 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads,
     else
       {
 	int n_threads = 0;
+	size_t thread_id_len = 17;
 
 	for (tp = thread_list; tp; tp = tp->next)
 	  {
@@ -1166,6 +1186,12 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads,
 				      global_ids, pid, tp))
 	      continue;
 
+	    if (!uiout->is_mi_like_p ())
+	      {
+		thread_id_len = std::max (thread_id_len,
+					  thread_target_id_str (tp).size ());
+	      }
+
 	    ++n_threads;
 	  }
 
@@ -1186,7 +1212,7 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads,
 	uiout->table_header (4, ui_left, "id-in-tg", "Id");
 	if (show_global_ids)
 	  uiout->table_header (4, ui_left, "id", "GId");
-	uiout->table_header (17, ui_left, "target-id", "Target Id");
+	uiout->table_header (thread_id_len, ui_left, "target-id", "Target Id");
 	uiout->table_header (1, ui_left, "frame", "Frame");
 	uiout->table_body ();
       }
@@ -1224,33 +1250,24 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads,
 	   shared by several fields.  For MI, we do the right thing
 	   instead.  */
 
-	target_id = target_pid_to_str (tp->ptid);
-	extra_info = target_extra_thread_info (tp);
-	name = tp->name ? tp->name : target_thread_name (tp);
-
 	if (uiout->is_mi_like_p ())
 	  {
-	    uiout->field_string ("target-id", target_id);
-	    if (extra_info)
+	    uiout->field_string ("target-id", target_pid_to_str (tp->ptid));
+
+	    const char *extra_info = target_extra_thread_info (tp);
+	    if (extra_info != nullptr)
 	      uiout->field_string ("details", extra_info);
-	    if (name)
+
+	    const char *name = (tp->name != nullptr
+				? tp->name
+				: target_thread_name (tp));
+	    if (name != NULL)
 	      uiout->field_string ("name", name);
 	  }
 	else
 	  {
-	    std::string contents;
-
-	    if (extra_info && name)
-	      contents = string_printf ("%s \"%s\" (%s)", target_id,
-					name, extra_info);
-	    else if (extra_info)
-	      contents = string_printf ("%s (%s)", target_id, extra_info);
-	    else if (name)
-	      contents = string_printf ("%s \"%s\"", target_id, name);
-	    else
-	      contents = target_id;
-
-	    uiout->field_string ("target-id", contents.c_str ());
+	    uiout->field_string ("target-id",
+				 thread_target_id_str (tp).c_str ());
 	  }
 
 	if (tp->state == THREAD_RUNNING)
-- 
2.14.3

[PATCH 1/2] remote & target_extra_thread_info, use cache w/ qThreadExtraInfo and qP too

[Pedro Alves]

The following patch will make "info threads" call target_extra_thread_info
more frequently.  When I looked at the remote implementation, I noticed
that if we're not using qXfer:threads:read, then we'd be increasing the
remote protocol traffic.  This commit prevents that from happening.

Also, it removes a gratuitous local static buffer, which seems good on
its own.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* remote.c (remote_target::extra_thread_info): Delete
	'display_buf' and 'n' locals.  from the cache, regardless of
	packet mechanims is in use.  Use cache for qThreadExtra and qP
	methods too.
---
 gdb/remote.c | 51 +++++++++++++++++++++++----------------------------
 1 file changed, 23 insertions(+), 28 deletions(-)

diff --git a/gdb/remote.c b/gdb/remote.c
index 79ae8f66c7..400c75b661 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -3826,12 +3826,9 @@ const char *
 remote_target::extra_thread_info (thread_info *tp)
 {
   struct remote_state *rs = get_remote_state ();
-  int result;
   int set;
   threadref id;
   struct gdb_ext_thread_info threadinfo;
-  static char display_buf[100];	/* arbitrary...  */
-  int n = 0;                    /* position in display_buf */
 
   if (rs->remote_desc == 0)		/* paranoia */
     internal_error (__FILE__, __LINE__,
@@ -3843,17 +3840,18 @@ remote_target::extra_thread_info (thread_info *tp)
        server doesn't know about it.  */
     return NULL;
 
+  std::string &extra = get_remote_thread_info (tp)->extra;
+
+  /* If already have cached info, use it.  */
+  if (!extra.empty ())
+    return extra.c_str ();
+
   if (packet_support (PACKET_qXfer_threads) == PACKET_ENABLE)
     {
-      struct thread_info *info = find_thread_ptid (tp->ptid);
-
-      if (info != NULL && info->priv != NULL)
-	{
-	  const std::string &extra = get_remote_thread_info (info)->extra;
-	  return !extra.empty () ? extra.c_str () : NULL;
-	}
-      else
-	return NULL;
+      /* If we're using qXfer:threads:read, then the extra info is
+	 included in the XML.  So if we didn't have anything cached,
+	 it's because there's really no extra info.  */
+      return NULL;
     }
 
   if (rs->use_threadextra_query)
@@ -3869,10 +3867,9 @@ remote_target::extra_thread_info (thread_info *tp)
       getpkt (&rs->buf, &rs->buf_size, 0);
       if (rs->buf[0] != 0)
 	{
-	  n = std::min (strlen (rs->buf) / 2, sizeof (display_buf));
-	  result = hex2bin (rs->buf, (gdb_byte *) display_buf, n);
-	  display_buf [result] = '\0';
-	  return display_buf;
+	  extra.resize (strlen (rs->buf) / 2);
+	  hex2bin (rs->buf, (gdb_byte *) &extra[0], extra.size ());
+	  return extra.c_str ();
 	}
     }
 
@@ -3885,22 +3882,20 @@ remote_target::extra_thread_info (thread_info *tp)
     if (threadinfo.active)
       {
 	if (*threadinfo.shortname)
-	  n += xsnprintf (&display_buf[0], sizeof (display_buf) - n,
-			  " Name: %s,", threadinfo.shortname);
+	  string_appendf (extra, " Name: %s", threadinfo.shortname);
 	if (*threadinfo.display)
-	  n += xsnprintf (&display_buf[n], sizeof (display_buf) - n,
-			  " State: %s,", threadinfo.display);
+	  {
+	    if (!extra.empty ())
+	      extra += ',';
+	    string_appendf (extra, " State: %s", threadinfo.display);
+	  }
 	if (*threadinfo.more_display)
-	  n += xsnprintf (&display_buf[n], sizeof (display_buf) - n,
-			  " Priority: %s", threadinfo.more_display);
-
-	if (n > 0)
 	  {
-	    /* For purely cosmetic reasons, clear up trailing commas.  */
-	    if (',' == display_buf[n-1])
-	      display_buf[n-1] = ' ';
-	    return display_buf;
+	    if (!extra.empty ())
+	      extra += ',';
+	    string_appendf (extra, " Priority: %s", threadinfo.more_display);
 	  }
+	return extra.c_str ();
       }
   return NULL;
 }
-- 
2.14.3

[PATCH 0/2] Improve alignment of "info threads" output, align "Target Id" column

[Pedro Alves]

Another bit taken from the multi-target branch.

It's long annoyed me that "info threads"'s columns are misaligned.  In
the multi-target branch, I experimented with adding target info to
"info threads", and the misalignment only got worse.  So I finally bit
the bullet and decided to address the alignment issue.  See patch #2.
Patch #1 is preparatory work to that avoids adding remote protocol
traffic.

Pedro Alves (2):
  remote & target_extra_thread_info, use cache w/ qThreadExtraInfo and
    qP too
  Improve alignment of "info threads" output, align "Target Id" column

 gdb/remote.c                        | 51 ++++++++++++++-----------------
 gdb/testsuite/gdb.threads/names.exp |  2 +-
 gdb/thread.c                        | 61 ++++++++++++++++++++++++-------------
 3 files changed, 63 insertions(+), 51 deletions(-)

-- 
2.14.3

Re: [PATCH 2/2] Improve alignment of "info threads" output, align "Target Id" column

[Pedro Alves]

On 06/14/2018 05:28 PM, Pedro Alves wrote:
> It's long annoyed me that "info threads"'s columns are misaligned.

Did "info threads" a few more times today, and remembered this patch.

> This results in calling several target_pid_to_str /
> target_extra_thread_info / target_thread_name twice for each thread,

Err, that "several" was not supposed to be there.  It's really only
twice per thread.

> @@ -1159,6 +1178,7 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads,
>      else
>        {
>  	int n_threads = 0;
> +	size_t thread_id_len = 17;

I meanwhile renamed this variable to "target_id_col_width" to make it
a little more clear, since the column is called "Target Id",
not "Thread Id".

Here's what I pushed.

From 75acb4867dc8bdd701983af6899d823c9e2e53a4 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 29 Jun 2018 20:45:35 +0100
Subject: [PATCH 2/2] Improve alignment of "info threads" output, align "Target
 Id" column

It's long annoyed me that "info threads"'s columns are misaligned.

Particularly the "Target Id" column's content is usually longer than
the specified column width, so the table ends up with the "Frame"
column misaligned.  For example, currently we get this:

 (gdb) info threads
   Id   Target Id         Frame
   1    Thread 0x7ffff7fb5740 (LWP 9056) "threads" 0x00007ffff7bc28ad in __pthread_join (threadid=140737345763072, thread_return=0x7fffffffd3e8) at pthread_join.c:90
   2    Thread 0x7ffff7803700 (LWP 9060) "function0" thread_function0 (arg=0x0) at threads.c:90
 * 3    Thread 0x7ffff7002700 (LWP 9061) "threads" thread_function1 (arg=0x1) at threads.c:106

The fact that the "Frame" heading is in a weird spot is particularly
annoying.

This commit turns the above into into this:

 (gdb) info threads
   Id   Target Id                                    Frame
   1    Thread 0x7ffff7fb5740 (LWP 7548) "threads"   0x00007ffff7bc28ad in __pthread_join (threadid=140737345763072, thread_return=0x7fffffffd3e8) at pthread_join.c:90
   2    Thread 0x7ffff7803700 (LWP 7555) "function0" thread_function0 (arg=0x0) at threads.c:91
 * 3    Thread 0x7ffff7002700 (LWP 7557) "threads"   thread_function1 (arg=0x1) at threads.c:104

It does that by computing the max width of the "Target Id" column and
using that as column width when creating the table.

This results in calling target_pid_to_str / target_extra_thread_info /
target_thread_name twice for each thread, but I think that it doesn't
matter in practice performance-wise, because the remote target caches
the info, and with native targets it shouldn't be noticeable.  It
could matter if we have many threads (say, thousands), but then "info
threads" is practically useless in such a scenario anyway -- better
thread filtering and aggregation would be necessary.

(Note: I have an old branch somewhere where I attempted at making
gdb's "info threads"-like tables follow a model/view design, so that a
general framework took care of issues like these, but it's incomplete
and a much bigger change.  This patch doesn't prevent going in that
direction in the future, of course.)

gdb/ChangeLog:
2018-06-29  Pedro Alves  <palves@redhat.com>

	* thread.c (thread_target_id_str): New, factored out from ...
	(print_thread_info_1): ... here.  Use it to compute the max
	"Target Id" column width.

gdb/testsuite/ChangeLog:
2018-06-29  Pedro Alves  <palves@redhat.com>

	* gdb.threads/names.exp: Adjust expected "info threads" output.
---
 gdb/ChangeLog                       |  6 ++++
 gdb/testsuite/ChangeLog             |  4 +++
 gdb/testsuite/gdb.threads/names.exp |  2 +-
 gdb/thread.c                        | 65 ++++++++++++++++++++++++-------------
 4 files changed, 54 insertions(+), 23 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 2d75bd2ac1..0cbccf5f8a 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2018-06-29  Pedro Alves  <palves@redhat.com>
+
+	* thread.c (thread_target_id_str): New, factored out from ...
+	(print_thread_info_1): ... here.  Use it to compute the max
+	"Target Id" column width.
+
 2018-06-29  Pedro Alves  <palves@redhat.com>
 
 	* remote.c (remote_target::extra_thread_info): Delete
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 8a11071993..93c849c040 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2018-06-29  Pedro Alves  <palves@redhat.com>
+
+	* gdb.threads/names.exp: Adjust expected "info threads" output.
+
 2018-06-29  Pedro Alves  <palves@redhat.com>
 
 	* gdb.opt/inline-break.exp (line number, address): Add "info
diff --git a/gdb/testsuite/gdb.threads/names.exp b/gdb/testsuite/gdb.threads/names.exp
index 44e21e65e9..fbc10f5506 100644
--- a/gdb/testsuite/gdb.threads/names.exp
+++ b/gdb/testsuite/gdb.threads/names.exp
@@ -31,7 +31,7 @@ if ![runto "all_threads_ready"] {
 }
 
 gdb_test "info threads" \
-    [multi_line "\\* 1   .*\"main\" all_threads_ready.*" \
+    [multi_line "\\* 1   .*\"main\"\[ \]\+all_threads_ready.*" \
 		"  2   .*\"carrot\".*"  \
 		"  3   .*\"potato\".*"  \
 		"  4   .*\"celery\".*" ] \
diff --git a/gdb/thread.c b/gdb/thread.c
index 77b497e281..fd92e78e06 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -1119,6 +1119,26 @@ should_print_thread (const char *requested_threads, int default_inf_num,
   return 1;
 }
 
+/* Return the string to display in "info threads"'s "Target Id"
+   column, for TP.  */
+
+static std::string
+thread_target_id_str (thread_info *tp)
+{
+  const char *target_id = target_pid_to_str (tp->ptid);
+  const char *extra_info = target_extra_thread_info (tp);
+  const char *name = tp->name != nullptr ? tp->name : target_thread_name (tp);
+
+  if (extra_info != nullptr && name != nullptr)
+    return string_printf ("%s \"%s\" (%s)", target_id, name, extra_info);
+  else if (extra_info != nullptr)
+    return string_printf ("%s (%s)", target_id, extra_info);
+  else if (name != nullptr)
+    return string_printf ("%s \"%s\"", target_id, name);
+  else
+    return target_id;
+}
+
 /* Like print_thread_info, but in addition, GLOBAL_IDS indicates
    whether REQUESTED_THREADS is a list of global or per-inferior
    thread ids.  */
@@ -1129,7 +1149,6 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads,
 		     int show_global_ids)
 {
   struct thread_info *tp;
-  const char *extra_info, *name, *target_id;
   struct inferior *inf;
   int default_inf_num = current_inferior ()->num;
 
@@ -1155,6 +1174,9 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads,
     else
       {
 	int n_threads = 0;
+	/* The width of the "Target Id" column.  Grown below to
+	   accommodate the largest entry.  */
+	size_t target_id_col_width = 17;
 
 	ALL_THREADS (tp)
 	  {
@@ -1162,6 +1184,13 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads,
 				      global_ids, pid, tp))
 	      continue;
 
+	    if (!uiout->is_mi_like_p ())
+	      {
+		target_id_col_width
+		  = std::max (target_id_col_width,
+			      thread_target_id_str (tp).size ());
+	      }
+
 	    ++n_threads;
 	  }
 
@@ -1182,7 +1211,8 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads,
 	uiout->table_header (4, ui_left, "id-in-tg", "Id");
 	if (show_global_ids)
 	  uiout->table_header (4, ui_left, "id", "GId");
-	uiout->table_header (17, ui_left, "target-id", "Target Id");
+	uiout->table_header (target_id_col_width, ui_left,
+			     "target-id", "Target Id");
 	uiout->table_header (1, ui_left, "frame", "Frame");
 	uiout->table_body ();
       }
@@ -1224,33 +1254,24 @@ print_thread_info_1 (struct ui_out *uiout, const char *requested_threads,
 	   shared by several fields.  For MI, we do the right thing
 	   instead.  */
 
-	target_id = target_pid_to_str (tp->ptid);
-	extra_info = target_extra_thread_info (tp);
-	name = tp->name ? tp->name : target_thread_name (tp);
-
 	if (uiout->is_mi_like_p ())
 	  {
-	    uiout->field_string ("target-id", target_id);
-	    if (extra_info)
+	    uiout->field_string ("target-id", target_pid_to_str (tp->ptid));
+
+	    const char *extra_info = target_extra_thread_info (tp);
+	    if (extra_info != nullptr)
 	      uiout->field_string ("details", extra_info);
-	    if (name)
+
+	    const char *name = (tp->name != nullptr
+				? tp->name
+				: target_thread_name (tp));
+	    if (name != NULL)
 	      uiout->field_string ("name", name);
 	  }
 	else
 	  {
-	    std::string contents;
-
-	    if (extra_info && name)
-	      contents = string_printf ("%s \"%s\" (%s)", target_id,
-					name, extra_info);
-	    else if (extra_info)
-	      contents = string_printf ("%s (%s)", target_id, extra_info);
-	    else if (name)
-	      contents = string_printf ("%s \"%s\"", target_id, name);
-	    else
-	      contents = target_id;
-
-	    uiout->field_string ("target-id", contents.c_str ());
+	    uiout->field_string ("target-id",
+				 thread_target_id_str (tp).c_str ());
 	  }
 
 	if (tp->state == THREAD_RUNNING)
-- 
2.14.4