[RFA] Add optional argument to "info threads" command

[RFA] Add optional argument to "info threads" command

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 339 bytes --]

I've long been frustrated because I could not ask for
info about just one or more threads.

This patch makes use of command parsing code in breakpoint.c
to give "info threads" the same optional arguments as "info
breakpoints", ie. a list of one or more threads of interest,
or a range of threads (eg. "5-9").

Documentation updated.

OK?


[-- Attachment #2: infothread2.txt --]
[-- Type: text/plain, Size: 2681 bytes --]

2008-11-22  Michael Snyder  <msnyder@promb-2s-dhcp151.eng.vmware.com>

	* thread.c (info_threads_command): Parse arguments for optional
	list (or range) of threads to which the command shall apply.
	(_initialize_thread): Explain extension in help string.

2008-11-22  Michael Snyder  <msnyder@promb-2s-dhcp151.eng.vmware.com>

	* gdb.texinfo (Debugging Programs with Multiple Threads):
	Document new optional argument to "info threads" command.

Index: thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.98
diff -u -p -r1.98 thread.c
--- thread.c	17 Nov 2008 12:13:49 -0000	1.98
+++ thread.c	23 Nov 2008 02:11:22 -0000
@@ -752,7 +752,34 @@ The current thread <Thread ID %d> has te
 static void
 info_threads_command (char *arg, int from_tty)
 {
-  print_thread_info (uiout, -1, -1);
+  char *p = arg;
+  char *p1;
+  int num, match;
+  struct thread_info *tp;
+
+  if (p == NULL)
+    print_thread_info (uiout, -1, -1);
+  else
+    while (*p)
+      {
+	match = 0;
+	p1 = p;
+	num = get_number_or_range (&p1);
+	if (num == 0)
+	  warning (_("bad thread number at or near '%s'"), p);
+	else
+	  {
+	    for (tp = thread_list; tp; tp = tp->next)
+	      if (num == tp->num)
+		{
+		  match = 1;
+		  print_thread_info (uiout, num, -1);
+		}
+	    if (match == 0)
+	      printf_unfiltered (_("No thread number %d.\n"), num);
+	  }
+	p = p1;
+      }
 }
 
 /* Switch from one thread to another. */
@@ -1116,7 +1143,7 @@ _initialize_thread (void)
   struct cmd_list_element *c;
 
   c = add_info ("threads", info_threads_command,
-		_("IDs of currently known threads."));
+		_("IDs of currently known threads, or optionally specified threads."));
   set_cmd_no_selected_thread_ok (c);
 
   c = add_prefix_cmd ("thread", class_run, thread_command, _("\
Index: doc/gdb.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
retrieving revision 1.536
diff -u -p -r1.536 gdb.texinfo
--- doc/gdb.texinfo	22 Nov 2008 12:14:33 -0000	1.536
+++ doc/gdb.texinfo	23 Nov 2008 02:11:23 -0000
@@ -2491,9 +2491,11 @@ number---always a single integer---with 
 
 @table @code
 @kindex info threads
-@item info threads
-Display a summary of all threads currently in your
-program.  @value{GDBN} displays for each thread (in this order):
+@item info threads @r{[}@var{threads}@r{]} @r{[}@var{range}@dots{}@r{]}
+Display a summary of all threads currently in your program.  
+Optional argument means display information only
+about the specified threads or range of threads.
+@value{GDBN} displays for each thread (in this order):
 
 @enumerate
 @item

Re: [RFA] Add optional argument to "info threads" command

[teawater]

Hey, I think this function is useful.

Thanks,
Hui

On Sun, Nov 23, 2008 at 10:12, Michael Snyder <msnyder@vmware.com> wrote:
>
> I've long been frustrated because I could not ask for
> info about just one or more threads.
>
> This patch makes use of command parsing code in breakpoint.c
> to give "info threads" the same optional arguments as "info
> breakpoints", ie. a list of one or more threads of interest,
> or a range of threads (eg. "5-9").
>
> Documentation updated.
>
> OK?
>
>
> 2008-11-22  Michael Snyder  <msnyder@promb-2s-dhcp151.eng.vmware.com>
>
>        * thread.c (info_threads_command): Parse arguments for optional
>        list (or range) of threads to which the command shall apply.
>        (_initialize_thread): Explain extension in help string.
>
> 2008-11-22  Michael Snyder  <msnyder@promb-2s-dhcp151.eng.vmware.com>
>
>        * gdb.texinfo (Debugging Programs with Multiple Threads):
>        Document new optional argument to "info threads" command.
>
> Index: thread.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/thread.c,v
> retrieving revision 1.98
> diff -u -p -r1.98 thread.c
> --- thread.c    17 Nov 2008 12:13:49 -0000      1.98
> +++ thread.c    23 Nov 2008 02:11:22 -0000
> @@ -752,7 +752,34 @@ The current thread <Thread ID %d> has te
>  static void
>  info_threads_command (char *arg, int from_tty)
>  {
> -  print_thread_info (uiout, -1, -1);
> +  char *p = arg;
> +  char *p1;
> +  int num, match;
> +  struct thread_info *tp;
> +
> +  if (p == NULL)
> +    print_thread_info (uiout, -1, -1);
> +  else
> +    while (*p)
> +      {
> +       match = 0;
> +       p1 = p;
> +       num = get_number_or_range (&p1);
> +       if (num == 0)
> +         warning (_("bad thread number at or near '%s'"), p);
> +       else
> +         {
> +           for (tp = thread_list; tp; tp = tp->next)
> +             if (num == tp->num)
> +               {
> +                 match = 1;
> +                 print_thread_info (uiout, num, -1);
> +               }
> +           if (match == 0)
> +             printf_unfiltered (_("No thread number %d.\n"), num);
> +         }
> +       p = p1;
> +      }
>  }
>
>  /* Switch from one thread to another. */
> @@ -1116,7 +1143,7 @@ _initialize_thread (void)
>   struct cmd_list_element *c;
>
>   c = add_info ("threads", info_threads_command,
> -               _("IDs of currently known threads."));
> +               _("IDs of currently known threads, or optionally specified threads."));
>   set_cmd_no_selected_thread_ok (c);
>
>   c = add_prefix_cmd ("thread", class_run, thread_command, _("\
> Index: doc/gdb.texinfo
> ===================================================================
> RCS file: /cvs/src/src/gdb/doc/gdb.texinfo,v
> retrieving revision 1.536
> diff -u -p -r1.536 gdb.texinfo
> --- doc/gdb.texinfo     22 Nov 2008 12:14:33 -0000      1.536
> +++ doc/gdb.texinfo     23 Nov 2008 02:11:23 -0000
> @@ -2491,9 +2491,11 @@ number---always a single integer---with
>
>  @table @code
>  @kindex info threads
> -@item info threads
> -Display a summary of all threads currently in your
> -program.  @value{GDBN} displays for each thread (in this order):
> +@item info threads @r{[}@var{threads}@r{]} @r{[}@var{range}@dots{}@r{]}
> +Display a summary of all threads currently in your program.
> +Optional argument means display information only
> +about the specified threads or range of threads.
> +@value{GDBN} displays for each thread (in this order):
>
>  @enumerate
>  @item
>

Re: [RFA] Add optional argument to "info threads" command

[Pedro Alves]

On Sunday 23 November 2008 02:12:52, Michael Snyder wrote:
> I've long been frustrated because I could not ask for
> info about just one or more threads.
> 
> This patch makes use of command parsing code in breakpoint.c
> to give "info threads" the same optional arguments as "info
> breakpoints", ie. a list of one or more threads of interest,
> or a range of threads (eg. "5-9").
> 
> 
> OK?

I'd like to point out that if you're debugging a multi-threaded
application with lots of threads (the case where you'd want to use
the range of threads feature), your implementation will, for
each id in the specified range:

 1. call prune_threads.

    This is a bunch of T (thread alive) packets in the remote target,
    one for each thread in the thread list -- not for each in
    range, really *for all* threads.

 2. call target_find_new_threads.

    A bunch of qfThreadInfo packets.  One for *each remote thread*, not
    just the ones specified in the range.

 3. do a make_cleanup_restore_current_thread and restore
    the selected thread and frame once for each thread in the
    range.  This triggers about the double amount of registers
    and memory reads than necessary.

 4. also, there's a chance you'll hit the same warning in
    restore_selected_frame once for each id in the range.

If you consider remote targets, and thinking that this new
feature would be most useful if you have many threads,
points 1-3 above can be heavy against not so fast links.  I
suggest abstracting out the bits in print_thread_info that do the
actual printing from the bits that prune dead threads, find
new threads, and restore the original selected frame.

-- 
Pedro Alves

Re: [RFA] Add optional argument to "info threads" command

[Michael Snyder]

Pedro Alves wrote:
> On Sunday 23 November 2008 02:12:52, Michael Snyder wrote:
>> I've long been frustrated because I could not ask for
>> info about just one or more threads.
>>
>> This patch makes use of command parsing code in breakpoint.c
>> to give "info threads" the same optional arguments as "info
>> breakpoints", ie. a list of one or more threads of interest,
>> or a range of threads (eg. "5-9").
>>
>>
>> OK?
> 
> I'd like to point out that if you're debugging a multi-threaded
> application with lots of threads (the case where you'd want to use
> the range of threads feature), your implementation will, for
> each id in the specified range:
> 
>  1. call prune_threads.
> 
>     This is a bunch of T (thread alive) packets in the remote target,
>     one for each thread in the thread list -- not for each in
>     range, really *for all* threads.
> 
>  2. call target_find_new_threads.
> 
>     A bunch of qfThreadInfo packets.  One for *each remote thread*, not
>     just the ones specified in the range.
> 
>  3. do a make_cleanup_restore_current_thread and restore
>     the selected thread and frame once for each thread in the
>     range.  This triggers about the double amount of registers
>     and memory reads than necessary.
> 
>  4. also, there's a chance you'll hit the same warning in
>     restore_selected_frame once for each id in the range.
> 
> If you consider remote targets, and thinking that this new
> feature would be most useful if you have many threads,
> points 1-3 above can be heavy against not so fast links.  I
> suggest abstracting out the bits in print_thread_info that do the
> actual printing from the bits that prune dead threads, find
> new threads, and restore the original selected frame.

Well, all your observations are correct -- but the same thing
also happens if you merely switch the thread of focus using the
"thread" command.

I do think that prune-threads and target_find_new_threads
are being called too aggressively from somewhere, but it
isn't directly from info_threads_command.

I think this is a separate, pre-existing problem that this patch
helps to reveal.

Re: [RFA] Add optional argument to "info threads" command

[Pedro Alves]

On Monday 24 November 2008 19:05:12, Michael Snyder wrote:
> Well, all your observations are correct -- but the same thing
> also happens if you merely switch the thread of focus using the
> "thread" command.
> 
> I do think that prune-threads and target_find_new_threads
> are being called too aggressively from somewhere, but it
> isn't directly from info_threads_command.

It's inside print_thread_info, and IIUC, you're calling it
on each of the ids in range?

> I think this is a separate, pre-existing problem that this patch
> helps to reveal.

I'm thinking about the case where you have 200 threads, and you
do 'info threads 40-45'.

That'll do 15 prune_threads, target_find_new_threads and
frame restores.  That'll be around '14 * (200 + 200 + 1)'
roundtrips extra at least instead of the current (200 + 200).

The worst case, is of course, if you do 'info threads 1-200'.
Then you'll have '199 * (200 + 200 + 1)' extra target
roundtrips compared to plain 'info threads'.  That's around
80000?  Unless I'm missing something.

Isn't this the same reasoning behind having thread_apply_command
and thread_apply_all implementations, instead of having one call
into the other?

(Upps, just spotted a cleanups bug in thread_apply_command).

-- 
Pedro Alves

Re: [RFA] Add optional argument to "info threads" command

[Pedro Alves]

On Monday 24 November 2008 19:24:14, Pedro Alves wrote:
> (Upps, just spotted a cleanups bug in thread_apply_command).

No bug, false alarm.

It looked like the make_cleanup_restore_current_thread call
could be being done more than once, but it can't happen.

-- 
Pedro Alves

Re: [RFA] Add optional argument to "info threads" command

[Michael Snyder]

Pedro Alves wrote:
> On Monday 24 November 2008 19:05:12, Michael Snyder wrote:
>> Well, all your observations are correct -- but the same thing
>> also happens if you merely switch the thread of focus using the
>> "thread" command.
>>
>> I do think that prune-threads and target_find_new_threads
>> are being called too aggressively from somewhere, but it
>> isn't directly from info_threads_command.
> 
> It's inside print_thread_info, and IIUC, you're calling it
> on each of the ids in range?
> 
>> I think this is a separate, pre-existing problem that this patch
>> helps to reveal.
> 
> I'm thinking about the case where you have 200 threads, and you
> do 'info threads 40-45'.
> 
> That'll do 15 prune_threads, target_find_new_threads and
> frame restores.  That'll be around '14 * (200 + 200 + 1)'
> roundtrips extra at least instead of the current (200 + 200).
> 
> The worst case, is of course, if you do 'info threads 1-200'.
> Then you'll have '199 * (200 + 200 + 1)' extra target
> roundtrips compared to plain 'info threads'.  That's around
> 80000?  Unless I'm missing something.
> 
> Isn't this the same reasoning behind having thread_apply_command
> and thread_apply_all implementations, instead of having one call
> into the other?

OK -- are you suggesting to abstract print_thread_info out
into two separate functions?

Re: [RFA] Add optional argument to "info threads" command

[Pedro Alves]

[-- Attachment #1: Type: text/plain, Size: 857 bytes --]

On Monday 24 November 2008 21:53:25, Michael Snyder wrote:
> > Isn't this the same reasoning behind having thread_apply_command
> > and thread_apply_all implementations, instead of having one call
> > into the other?
> 
> OK -- are you suggesting to abstract print_thread_info out
> into two separate functions?

That, or make print_thread_info itself take a range, something
like the attached.  If MI wants to be able to specify a range,
we could make print_thread_info itself that a char* and do the
range parsing there.

There's still always both a prune_threads and target_find_new_threads
call, but with that I can live.

We *could* be smarter about that too.  Only prune threads in
the passed range iff we're specifying a range (all otherwise); and,
only find new threads if any of the range ends is higher
than the highest id known.

-- 
Pedro Alves

[-- Attachment #2: thread_info_range.diff --]
[-- Type: text/x-diff, Size: 6369 bytes --]

---
 gdb/thread.c |  135 ++++++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 87 insertions(+), 48 deletions(-)

Index: src/gdb/thread.c
===================================================================
--- src.orig/gdb/thread.c	2008-11-24 22:08:50.000000000 +0000
+++ src/gdb/thread.c	2008-11-24 23:05:22.000000000 +0000
@@ -631,19 +631,22 @@ set_stop_requested (ptid_t ptid, int sto
     observer_notify_thread_stop_requested (ptid);
 }
 
-/* Prints the list of threads and their details on UIOUT.
-   This is a version of 'info_thread_command' suitable for
-   use from MI.  
-   If REQUESTED_THREAD is not -1, it's the GDB id of the thread
-   that should be printed.  Otherwise, all threads are
-   printed.  
-   If PID is not -1, only print threads from the process PID.
-   Otherwise, threads from all attached PIDs are printed.   
-   If both REQUESTED_THREAD and PID are not -1, then the thread
-   is printed if it belongs to the specified process.  Otherwise,
-   an error is raised.  */
-void
-print_thread_info (struct ui_out *uiout, int requested_thread, int pid)
+/* Prints the list of threads whose id falls in the range specified by
+   RANGE_START and RANGE_END (inclusive) and their details on UIOUT.
+
+   If RANGE_START is -1, all threads are printed.
+
+   If PID is not -1, only print threads from the process PID (target
+   id, not GDB inferior number).
+
+   Otherwise, threads from all attached PIDs are printed.  If both
+   RANGE_START and PID are not -1, and RANGE_START equal RANGE_END,
+   then the RANGE_START thread is printed if it belongs to the
+   specified process; otherwise, an error is raised.  */
+
+static void
+print_thread_info_1 (struct ui_out *uiout,
+		     int range_start, int range_end, int pid)
 {
   struct thread_info *tp;
   ptid_t current_ptid;
@@ -663,12 +666,9 @@ print_thread_info (struct ui_out *uiout,
     {
       struct cleanup *chain2;
 
-      if (requested_thread != -1 && tp->num != requested_thread)
-	continue;
-
       if (pid != -1 && PIDGET (tp->ptid) != pid)
 	{
-	  if (requested_thread != -1)
+	  if (range_start != -1 && range_start == range_end)
 	    error (_("Requested thread not found in requested process"));
 	  continue;
 	}
@@ -676,6 +676,10 @@ print_thread_info (struct ui_out *uiout,
       if (ptid_equal (tp->ptid, current_ptid))
 	current_thread = tp->num;
 
+      if (range_start != -1
+	  && (tp->num < range_start || range_end < tp->num))
+	continue;
+
       if (tp->state_ == THREAD_EXITED)
 	continue;
 
@@ -700,7 +704,12 @@ print_thread_info (struct ui_out *uiout,
       ui_out_text (uiout, "  ");
 
       if (tp->state_ == THREAD_RUNNING)
-	ui_out_text (uiout, "(running)\n");
+	{
+	  ui_out_text (uiout, "(running)\n");
+
+	  if (ui_out_is_mi_like_p (uiout))
+	    ui_out_field_string (uiout, "state", "running");
+	}
       else
 	{
 	  /* The switch below puts us at the top of the stack (leaf
@@ -710,14 +719,9 @@ print_thread_info (struct ui_out *uiout,
 			     /* For MI output, print frame level.  */
 			     ui_out_is_mi_like_p (uiout),
 			     LOCATION);
-	}
 
-      if (ui_out_is_mi_like_p (uiout))
-	{
-	  char *state = "stopped";
-	  if (tp->state_ == THREAD_RUNNING)
-	    state = "running";
-	  ui_out_field_string (uiout, "state", state);
+	  if (ui_out_is_mi_like_p (uiout))
+	    ui_out_field_string (uiout, "state", "stopped");
 	}
 
       do_cleanups (chain2);
@@ -727,7 +731,7 @@ print_thread_info (struct ui_out *uiout,
      the "info threads" command.  */
   do_cleanups (old_chain);
 
-  if (pid == -1 && requested_thread == -1 )
+  if (pid == -1 && range_start == -1)
     {
       gdb_assert (current_thread != -1
 		  || !thread_list);
@@ -741,6 +745,56 @@ The current thread <Thread ID %d> has te
     }
 }
 
+/* Prints the list of threads and their details on UIOUT.  This is a
+   version of 'info_thread_command' suitable for use from MI.
+
+   If REQUESTED_THREAD is not -1, it's the GDB id of the thread that
+   should be printed.  Otherwise, all threads are printed.
+
+   If PID is not -1, only print threads from the process PID.
+   Otherwise, threads from all attached PIDs are printed.  If both
+   REQUESTED_THREAD and PID are not -1, then the thread is printed if
+   it belongs to the specified process.  Otherwise, an error is
+   raised.  */
+
+void
+print_thread_info (struct ui_out *uiout, int requested_thread, int pid)
+{
+  print_thread_info_1 (uiout, requested_thread, requested_thread, pid);
+}
+
+/* Parse a thread id or thread id range specified in TIDLIST, and
+   store it into START and END.  */
+
+static char *
+parse_thread_id_or_range (char *tidlist, int *start, int *end)
+{
+  char *p;
+
+  *start = strtol (tidlist, &p, 10);
+  if (p == tidlist)
+    error (_("Error parsing %s"), tidlist);
+  tidlist = p;
+
+  while (*tidlist == ' ' || *tidlist == '\t')
+    tidlist++;
+
+  if (*tidlist == '-')	/* Got a range of IDs? */
+    {
+      tidlist++;		/* Skip the - */
+      *end = strtol (tidlist, &p, 10);
+      if (p == tidlist)
+	error (_("Error parsing %s"), tidlist);
+      tidlist = p;
+
+      while (*tidlist == ' ' || *tidlist == '\t')
+	tidlist++;
+    }
+  else
+    *end = *start;
+
+  return tidlist;
+}
 
 /* Print information about currently known threads 
 
@@ -752,7 +806,12 @@ The current thread <Thread ID %d> has te
 static void
 info_threads_command (char *arg, int from_tty)
 {
-  print_thread_info (uiout, -1, -1);
+  int start = -1, end = -1;
+
+  if (arg && *arg)
+    parse_thread_range (arg, &start, &end);
+
+  print_thread_info_1 (uiout, start, end, -1);
 }
 
 /* Switch from one thread to another. */
@@ -971,27 +1030,7 @@ thread_apply_command (char *tidlist, int
       struct thread_info *tp;
       int start, end;
 
-      start = strtol (tidlist, &p, 10);
-      if (p == tidlist)
-	error (_("Error parsing %s"), tidlist);
-      tidlist = p;
-
-      while (*tidlist == ' ' || *tidlist == '\t')
-	tidlist++;
-
-      if (*tidlist == '-')	/* Got a range of IDs? */
-	{
-	  tidlist++;		/* Skip the - */
-	  end = strtol (tidlist, &p, 10);
-	  if (p == tidlist)
-	    error (_("Error parsing %s"), tidlist);
-	  tidlist = p;
-
-	  while (*tidlist == ' ' || *tidlist == '\t')
-	    tidlist++;
-	}
-      else
-	end = start;
+      tidlist = parse_thread_range (tidlist, &start, &end);
 
       make_cleanup_restore_current_thread ();
 

Re: [RFA] Add optional argument to "info threads" command

[Eli Zaretskii]

> Date: Sat, 22 Nov 2008 18:12:52 -0800
> From: Michael Snyder <msnyder@vmware.com>
> 
> --- doc/gdb.texinfo	22 Nov 2008 12:14:33 -0000	1.536
> +++ doc/gdb.texinfo	23 Nov 2008 02:11:23 -0000
> @@ -2491,9 +2491,11 @@ number---always a single integer---with 
>  
>  @table @code
>  @kindex info threads
> -@item info threads
> -Display a summary of all threads currently in your
> -program.  @value{GDBN} displays for each thread (in this order):
> +@item info threads @r{[}@var{threads}@r{]} @r{[}@var{range}@dots{}@r{]}
> +Display a summary of all threads currently in your program.  
> +Optional argument means display information only
> +about the specified threads or range of threads.
> +@value{GDBN} displays for each thread (in this order):

This is okay, but I think we should say a word or two about the
supported syntax of "range".