[RFC] Implement -list-thread-groups.

[RFC] Implement -list-thread-groups.

[Vladimir Prus]

This patch implements new -list-thread-groups command. See the MI doc
patch I've posted earlier for general explanations. With this patch,
the command essentially prints gdb inferiors table. The --available
option is not implemented, and will be posted separately.

I'll commit in a few days if there are no objections.

- Volodya

From 3a312c9177927097a50030700d6f3939c7d4a5a2 Mon Sep 17 00:00:00 2001

	* thread.c (print_thread_info): New parameter pid, to print
	threads of specific process.
	* gdbthread.h (print_thread_info): New parameter pid.
	* mi/mi-cmds.c (mi_cmds): Register -list-thread-groups.
	* mi/mi-cmds.h (mi_cmd_list_thread_groups): New.
	* mi/mi-main.c (mi_cmd_thread_info): Adjust.
	(print_one_process, mi_cmd_list_thread_groups): New.
---
 gdb/gdbthread.h  |    3 ++-
 gdb/mi/mi-cmds.c |    1 +
 gdb/mi/mi-cmds.h |    1 +
 gdb/mi/mi-main.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 gdb/thread.c     |   11 +++++++----
 5 files changed, 61 insertions(+), 6 deletions(-)

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 55c848d..cac20f7 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -295,7 +295,8 @@ extern struct cmd_list_element *thread_cmd_list;
    `set print thread-events'.  */
 extern int print_thread_events;
 
-extern void print_thread_info (struct ui_out *uiout, int thread);
+extern void print_thread_info (struct ui_out *uiout, int thread,
+			       int pid);
 
 extern struct cleanup *make_cleanup_restore_current_thread (void);
 
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index ca0f428..d38de35 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -90,6 +90,7 @@ struct mi_cmd mi_cmds[] =
   { "interpreter-exec", { NULL, 0 }, mi_cmd_interpreter_exec},
   { "list-features", { NULL, 0 }, mi_cmd_list_features},
   { "list-target-features", { NULL, 0 }, mi_cmd_list_target_features},
+  { "list-thread-groups", { NULL, 0 }, mi_cmd_list_thread_groups },  
   { "overlay-auto", { NULL, 0 }, NULL },
   { "overlay-list-mapping-state", { NULL, 0 }, NULL },
   { "overlay-list-overlays", { NULL, 0 }, NULL },
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index 16887ae..a9bb1e0 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -67,6 +67,7 @@ extern mi_cmd_argv_ftype mi_cmd_inferior_tty_show;
 extern mi_cmd_argv_ftype mi_cmd_interpreter_exec;
 extern mi_cmd_argv_ftype mi_cmd_list_features;
 extern mi_cmd_argv_ftype mi_cmd_list_target_features;
+extern mi_cmd_argv_ftype mi_cmd_list_thread_groups;
 extern mi_cmd_argv_ftype mi_cmd_stack_info_depth;
 extern mi_cmd_argv_ftype mi_cmd_stack_info_frame;
 extern mi_cmd_argv_ftype mi_cmd_stack_list_args;
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index a9fbcad..544fec6 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -46,6 +46,7 @@
 #include "mi-main.h"
 #include "language.h"
 #include "valprint.h"
+#include "processes.h"
 
 #include <ctype.h>
 #include <sys/time.h>
@@ -245,7 +246,55 @@ mi_cmd_thread_info (char *command, char **argv, int argc)
   if (argc == 1)
     thread = atoi (argv[0]);
 
-  print_thread_info (uiout, thread);
+  print_thread_info (uiout, thread, -1);
+}
+
+static int
+print_one_process (struct process_info *process, void *arg)
+{
+  struct cleanup *back_to = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
+
+  ui_out_field_fmt (uiout, "id", "p%d", process->pid);
+  ui_out_field_string (uiout, "type", "process");
+  ui_out_field_int (uiout, "pid", process->pid);
+  
+  do_cleanups (back_to);
+  return 0;
+}
+
+void
+mi_cmd_list_thread_groups (char *command, char **argv, int argc)
+{
+  struct cleanup *back_to;
+  int available = 0;
+  char *id = NULL;
+
+  if (argc > 0 && strcmp (argv[0], "--available") == 0)
+    {
+      ++argv;
+      --argc;
+      available = 1;
+    }
+
+  if (argc > 0)
+    id = argv[0];
+
+  back_to = make_cleanup (&null_cleanup, NULL);
+
+  if (id)
+    {
+      int pid = atoi (id);
+      if (!in_process_list (pid))
+	error ("Invalid thread group id '%s'", id);
+      print_thread_info (uiout, -1, pid);    
+    }
+  else
+    {
+      make_cleanup_ui_out_list_begin_end (uiout, "groups");
+      iterate_over_processes (print_one_process, NULL);
+    }
+  
+  do_cleanups (back_to);
 }
 
 void
diff --git a/gdb/thread.c b/gdb/thread.c
index b1e318d..316319a 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -638,7 +638,7 @@ set_stop_requested (ptid_t ptid, int stop)
    that should be printed.  Otherwise, all threads are
    printed.  */
 void
-print_thread_info (struct ui_out *uiout, int requested_thread)
+print_thread_info (struct ui_out *uiout, int requested_thread, int pid)
 {
   struct thread_info *tp;
   ptid_t current_ptid;
@@ -658,9 +658,12 @@ print_thread_info (struct ui_out *uiout, int requested_thread)
     {
       struct cleanup *chain2;
 
+      if (pid != -1 && PIDGET (tp->ptid) != pid)
+         continue;
+      
       if (requested_thread != -1 && tp->num != requested_thread)
 	continue;
-
+	
       if (ptid_equal (tp->ptid, current_ptid))
 	current_thread = tp->num;
 
@@ -715,7 +718,7 @@ print_thread_info (struct ui_out *uiout, int requested_thread)
      the "info threads" command.  */
   do_cleanups (old_chain);
 
-  if (requested_thread == -1)
+  if (pid == -1 && requested_thread == -1 )
     {
       gdb_assert (current_thread != -1
 		  || !thread_list);
@@ -740,7 +743,7 @@ The current thread <Thread ID %d> has terminated.  See `help thread'.\n",
 static void
 info_threads_command (char *arg, int from_tty)
 {
-  print_thread_info (uiout, -1);
+  print_thread_info (uiout, -1, -1);
 }
 
 /* Switch from one thread to another. */
-- 
1.5.3.5

Re: [RFC] Implement -list-thread-groups.

[Joel Brobecker]

> I'll commit in a few days if there are no objections.

If others are fine with this too, then so am I. But I don't think
this is the normal procedure: For non-MI changes, my understanding
is that you're supposed to wait for approval unless the changes
are obvious (particularly for patches labeled RFC). I don't blame
you for doing this, given the poor track record we're having in
terms of speed of review - I guess we need more help.

Anyway, onto the patch...

> 	* thread.c (print_thread_info): New parameter pid, to print
> 	threads of specific process.
> 	* gdbthread.h (print_thread_info): New parameter pid.
> 	* mi/mi-cmds.c (mi_cmds): Register -list-thread-groups.
> 	* mi/mi-cmds.h (mi_cmd_list_thread_groups): New.
> 	* mi/mi-main.c (mi_cmd_thread_info): Adjust.
> 	(print_one_process, mi_cmd_list_thread_groups): New.

Overall, looks good to me. Just a couple of minor nits and a question.

>  void
> -print_thread_info (struct ui_out *uiout, int requested_thread)
> +print_thread_info (struct ui_out *uiout, int requested_thread, int pid)

Can you add a comment for the new parameter in the function documentation,
please?

> -
> +	

You're adding some spaces in what would otherwise be an empty line.
Can you get rid of that change?

> +  if (pid == -1 && requested_thread == -1 )
>      {
>        gdb_assert (current_thread != -1
>  		  || !thread_list);

This has little to do with your change, but I'm curious. What does
this code do? It seems to be adding a current-thread-id field in
the output, but why only doing it when requested_thread (and pid)
is -1?

-- 
Joel

Re: [RFC] Implement -list-thread-groups.

[Michael Snyder]

Joel Brobecker wrote:
>> I'll commit in a few days if there are no objections.
> 
> If others are fine with this too, then so am I. But I don't think
> this is the normal procedure: For non-MI changes, my understanding
> is that you're supposed to wait for approval unless the changes
> are obvious (particularly for patches labeled RFC). 

Yes, the changes to thread.c should be reviewed and approved.

Re: [RFC] Implement -list-thread-groups.

[Vladimir Prus]

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

On Friday 14 November 2008 04:52:17 Joel Brobecker wrote:
> > I'll commit in a few days if there are no objections.
>
> If others are fine with this too, then so am I. But I don't think
> this is the normal procedure: For non-MI changes, my understanding
> is that you're supposed to wait for approval unless the changes
> are obvious (particularly for patches labeled RFC). I don't blame
> you for doing this, given the poor track record we're having in
> terms of speed of review - I guess we need more help.

Sorry if I'm trying to circumvent the rules. On the other hand, I believe I 
was told that MI boundaries are logical, not physical. In that sense, a change 
that clearly matters only for MI, and is actually posted for comments, is kind
of a boundary case.

Another example are observes. Although generic in nature, they are quickly
becoming de-facto interface of GDB core with MI.

> Anyway, onto the patch...
>
> > 	* thread.c (print_thread_info): New parameter pid, to print
> > 	threads of specific process.
> > 	* gdbthread.h (print_thread_info): New parameter pid.
> > 	* mi/mi-cmds.c (mi_cmds): Register -list-thread-groups.
> > 	* mi/mi-cmds.h (mi_cmd_list_thread_groups): New.
> > 	* mi/mi-main.c (mi_cmd_thread_info): Adjust.
> > 	(print_one_process, mi_cmd_list_thread_groups): New.
>
> Overall, looks good to me. Just a couple of minor nits and a question.
>
> >  void
> > -print_thread_info (struct ui_out *uiout, int requested_thread)
> > +print_thread_info (struct ui_out *uiout, int requested_thread, int pid)
>
> Can you add a comment for the new parameter in the function documentation,
> please?

Done.

>
> > -
> > +
>
> You're adding some spaces in what would otherwise be an empty line.
> Can you get rid of that change?

Done. Indicentally, I know how to configure Emacs to remove trailing 
whitespace on save. But pretty much every source file in gdb already has such 
lines. If somebody tell me how to make Emacs not add lines without trailing
whitespace, while *not* changing existing lines, it would be gtreat.

> > +  if (pid == -1 && requested_thread == -1 )
> >      {
> >        gdb_assert (current_thread != -1
> >
> >  		  || !thread_list);
>
> This has little to do with your change, but I'm curious. What does
> this code do? It seems to be adding a current-thread-id field in
> the output, but why only doing it when requested_thread (and pid)
> is -1?

Yes, exactly. If you ask to print details of a specific thread, then you don't
care what current thread is.

I attach the revised patch.

- Volodya





[-- Attachment #2: list_thread_groups.patch --]
[-- Type: text/x-patch, Size: 5515 bytes --]

commit a28a9fa5263190c36b5899f17b5746fd8cc292a2
Author: vladimir <vladimir@e7755896-6108-0410-9592-8049d3e74e28>
Date:   Thu Jul 31 13:23:28 2008 +0000

    Implement -list-thread-groups.
    
    	* thread.c (print_thread_info): New parameter pid, to print
    	threads of specific process.
    	* gdbthread.h (print_thread_info): New parameter pid.
    	* mi/mi-cmds.c (mi_cmds): Register -list-thread-groups.
    	* mi/mi-cmds.h (mi_cmd_list_thread_groups): New.
    	* mi/mi-main.c (mi_cmd_thread_info): Adjust.
    	(print_one_process, mi_cmd_list_thread_groups): New.

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 55c848d..cac20f7 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -295,7 +295,8 @@ extern struct cmd_list_element *thread_cmd_list;
    `set print thread-events'.  */
 extern int print_thread_events;
 
-extern void print_thread_info (struct ui_out *uiout, int thread);
+extern void print_thread_info (struct ui_out *uiout, int thread,
+			       int pid);
 
 extern struct cleanup *make_cleanup_restore_current_thread (void);
 
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index ca0f428..d38de35 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -90,6 +90,7 @@ struct mi_cmd mi_cmds[] =
   { "interpreter-exec", { NULL, 0 }, mi_cmd_interpreter_exec},
   { "list-features", { NULL, 0 }, mi_cmd_list_features},
   { "list-target-features", { NULL, 0 }, mi_cmd_list_target_features},
+  { "list-thread-groups", { NULL, 0 }, mi_cmd_list_thread_groups },  
   { "overlay-auto", { NULL, 0 }, NULL },
   { "overlay-list-mapping-state", { NULL, 0 }, NULL },
   { "overlay-list-overlays", { NULL, 0 }, NULL },
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index 16887ae..a9bb1e0 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -67,6 +67,7 @@ extern mi_cmd_argv_ftype mi_cmd_inferior_tty_show;
 extern mi_cmd_argv_ftype mi_cmd_interpreter_exec;
 extern mi_cmd_argv_ftype mi_cmd_list_features;
 extern mi_cmd_argv_ftype mi_cmd_list_target_features;
+extern mi_cmd_argv_ftype mi_cmd_list_thread_groups;
 extern mi_cmd_argv_ftype mi_cmd_stack_info_depth;
 extern mi_cmd_argv_ftype mi_cmd_stack_info_frame;
 extern mi_cmd_argv_ftype mi_cmd_stack_list_args;
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index a9fbcad..544fec6 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -46,6 +46,7 @@
 #include "mi-main.h"
 #include "language.h"
 #include "valprint.h"
+#include "processes.h"
 
 #include <ctype.h>
 #include <sys/time.h>
@@ -245,7 +246,55 @@ mi_cmd_thread_info (char *command, char **argv, int argc)
   if (argc == 1)
     thread = atoi (argv[0]);
 
-  print_thread_info (uiout, thread);
+  print_thread_info (uiout, thread, -1);
+}
+
+static int
+print_one_process (struct process_info *process, void *arg)
+{
+  struct cleanup *back_to = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
+
+  ui_out_field_fmt (uiout, "id", "p%d", process->pid);
+  ui_out_field_string (uiout, "type", "process");
+  ui_out_field_int (uiout, "pid", process->pid);
+  
+  do_cleanups (back_to);
+  return 0;
+}
+
+void
+mi_cmd_list_thread_groups (char *command, char **argv, int argc)
+{
+  struct cleanup *back_to;
+  int available = 0;
+  char *id = NULL;
+
+  if (argc > 0 && strcmp (argv[0], "--available") == 0)
+    {
+      ++argv;
+      --argc;
+      available = 1;
+    }
+
+  if (argc > 0)
+    id = argv[0];
+
+  back_to = make_cleanup (&null_cleanup, NULL);
+
+  if (id)
+    {
+      int pid = atoi (id);
+      if (!in_process_list (pid))
+	error ("Invalid thread group id '%s'", id);
+      print_thread_info (uiout, -1, pid);    
+    }
+  else
+    {
+      make_cleanup_ui_out_list_begin_end (uiout, "groups");
+      iterate_over_processes (print_one_process, NULL);
+    }
+  
+  do_cleanups (back_to);
 }
 
 void
diff --git a/gdb/thread.c b/gdb/thread.c
index b1e318d..a32dff4 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -636,9 +636,10 @@ set_stop_requested (ptid_t ptid, int stop)
    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.  */
+   printed.  
+   If PID is not -1, only prints threads from the process PID.  */
 void
-print_thread_info (struct ui_out *uiout, int requested_thread)
+print_thread_info (struct ui_out *uiout, int requested_thread, int pid)
 {
   struct thread_info *tp;
   ptid_t current_ptid;
@@ -646,6 +647,8 @@ print_thread_info (struct ui_out *uiout, int requested_thread)
   char *extra_info;
   int current_thread = -1;
 
+  gdb_assert (requested_thead == -1 || pid == -1);
+
   prune_threads ();
   target_find_new_threads ();
   current_ptid = inferior_ptid;
@@ -658,6 +661,9 @@ print_thread_info (struct ui_out *uiout, int requested_thread)
     {
       struct cleanup *chain2;
 
+      if (pid != -1 && PIDGET (tp->ptid) != pid)
+         continue;
+      
       if (requested_thread != -1 && tp->num != requested_thread)
 	continue;
 
@@ -715,7 +721,7 @@ print_thread_info (struct ui_out *uiout, int requested_thread)
      the "info threads" command.  */
   do_cleanups (old_chain);
 
-  if (requested_thread == -1)
+  if (pid == -1 && requested_thread == -1 )
     {
       gdb_assert (current_thread != -1
 		  || !thread_list);
@@ -740,7 +746,7 @@ The current thread <Thread ID %d> has terminated.  See `help thread'.\n",
 static void
 info_threads_command (char *arg, int from_tty)
 {
-  print_thread_info (uiout, -1);
+  print_thread_info (uiout, -1, -1);
 }
 
 /* Switch from one thread to another. */

Re: [RFC] Implement -list-thread-groups.

[Michael Snyder]

> diff --git a/gdb/thread.c b/gdb/thread.c
> index b1e318d..a32dff4 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -636,9 +636,10 @@ set_stop_requested (ptid_t ptid, int stop)
>     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.  */
> +   printed.  
> +   If PID is not -1, only prints threads from the process PID.  */

For completeness and consistency, how about adding
"Otherwise, threads from all attached PIDs are printed."

>  void
> -print_thread_info (struct ui_out *uiout, int requested_thread)
> +print_thread_info (struct ui_out *uiout, int requested_thread, int pid)
>  {
>    struct thread_info *tp;
>    ptid_t current_ptid;
> @@ -646,6 +647,8 @@ print_thread_info (struct ui_out *uiout, int requested_thread)
>    char *extra_info;
>    int current_thread = -1;
>  
> +  gdb_assert (requested_thead == -1 || pid == -1);

I'm puzzled by this assert.
You don't think we'll ever want to specify both the pid and the thread?


Otherwise, these changes to thread.c seem fine.

Re: [RFC] Implement -list-thread-groups.

[Vladimir Prus]

On Friday 14 November 2008 20:39:47 you wrote:
> 
> > diff --git a/gdb/thread.c b/gdb/thread.c
> > index b1e318d..a32dff4 100644
> > --- a/gdb/thread.c
> > +++ b/gdb/thread.c
> > @@ -636,9 +636,10 @@ set_stop_requested (ptid_t ptid, int stop)
> >     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.  */
> > +   printed.  
> > +   If PID is not -1, only prints threads from the process PID.  */
> 
> For completeness and consistency, how about adding
> "Otherwise, threads from all attached PIDs are printed."

Ok.

> >  void
> > -print_thread_info (struct ui_out *uiout, int requested_thread)
> > +print_thread_info (struct ui_out *uiout, int requested_thread, int pid)
> >  {
> >    struct thread_info *tp;
> >    ptid_t current_ptid;
> > @@ -646,6 +647,8 @@ print_thread_info (struct ui_out *uiout, int requested_thread)
> >    char *extra_info;
> >    int current_thread = -1;
> >  
> > +  gdb_assert (requested_thead == -1 || pid == -1);
> 
> I'm puzzled by this assert.
> You don't think we'll ever want to specify both the pid and the thread?

I think that makes no sense. If a thread is specified, then there's no possible
use of 'pid'. Threads are globally numbered.

- Volodya

Re: [RFC] Implement -list-thread-groups.

[Michael Snyder]

Vladimir Prus wrote:
> On Friday 14 November 2008 20:39:47 you wrote:
>>> diff --git a/gdb/thread.c b/gdb/thread.c
>>> index b1e318d..a32dff4 100644
>>> --- a/gdb/thread.c
>>> +++ b/gdb/thread.c
>>> @@ -636,9 +636,10 @@ set_stop_requested (ptid_t ptid, int stop)
>>>     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.  */
>>> +   printed.  
>>> +   If PID is not -1, only prints threads from the process PID.  */
>> For completeness and consistency, how about adding
>> "Otherwise, threads from all attached PIDs are printed."
> 
> Ok.
> 
>>>  void
>>> -print_thread_info (struct ui_out *uiout, int requested_thread)
>>> +print_thread_info (struct ui_out *uiout, int requested_thread, int pid)
>>>  {
>>>    struct thread_info *tp;
>>>    ptid_t current_ptid;
>>> @@ -646,6 +647,8 @@ print_thread_info (struct ui_out *uiout, int requested_thread)
>>>    char *extra_info;
>>>    int current_thread = -1;
>>>  
>>> +  gdb_assert (requested_thead == -1 || pid == -1);
>> I'm puzzled by this assert.
>> You don't think we'll ever want to specify both the pid and the thread?
> 
> I think that makes no sense. If a thread is specified, then there's no possible
> use of 'pid'. Threads are globally numbered.

Even if it makes no sense in the sense that
it's not required, that doesn't necessarily make it
an error.  Suppose somebody specifies both the pid and
the thread?  What's the harm?  If they're inconsistent
(this pid does not contain this thread), THEN we'll
return an error.

Re: [RFC] Implement -list-thread-groups.

[Vladimir Prus]

On Friday 14 November 2008 21:54:46 Michael Snyder wrote:

> >> I'm puzzled by this assert.
> >> You don't think we'll ever want to specify both the pid and the thread?
> >
> > I think that makes no sense. If a thread is specified, then there's no
> > possible use of 'pid'. Threads are globally numbered.
>
> Even if it makes no sense in the sense that
> it's not required, that doesn't necessarily make it
> an error.  Suppose somebody specifies both the pid and
> the thread?  What's the harm?  If they're inconsistent
> (this pid does not contain this thread), THEN we'll
> return an error.

I think it's better to make functions have as tight preconditions as possible. 
In this case, passing both thread and pid does not serve any possible purpose,
so it's likely that caller is doing this by mistake. It's best to assert 
immediately, rather than spending time and code space verifying if those
parameters are consistent. Checking if a thread belongs to a process is not
the part of this this function purpose.

- Volodya

Re: [RFC] Implement -list-thread-groups.

[Michael Snyder]

Vladimir Prus wrote:
> On Friday 14 November 2008 21:54:46 Michael Snyder wrote:
> 
>>>> I'm puzzled by this assert.
>>>> You don't think we'll ever want to specify both the pid and the thread?
>>> I think that makes no sense. If a thread is specified, then there's no
>>> possible use of 'pid'. Threads are globally numbered.
>> Even if it makes no sense in the sense that
>> it's not required, that doesn't necessarily make it
>> an error.  Suppose somebody specifies both the pid and
>> the thread?  What's the harm?  If they're inconsistent
>> (this pid does not contain this thread), THEN we'll
>> return an error.
> 
> I think it's better to make functions have as tight preconditions as possible. 
> In this case, passing both thread and pid does not serve any possible purpose,
> so it's likely that caller is doing this by mistake. It's best to assert 
> immediately, rather than spending time and code space verifying if those
> parameters are consistent.

I respect your opinion, but MI is not the only caller of this function.

 > Checking if a thread belongs to a process is not
> the part of this this function purpose.

It's input validation.  What you're doing is also input
validation, it's just imposing a more stringent requirement.

I feel that an assert is excessively stringent in this context.
An assert implies an internal gdb error.  These potentially
conflicting inputs could come about as a result of (foreseeable)
user input, rather than internal error.  Admittedly not any
user input that could be given now, but the CLI (or other
potential clients) could change.

I feel that if it's possible for these inputs to violate
the assert without actually reflecting an internally
inconsistant state, then the assert is too strong.

Re: [RFC] Implement -list-thread-groups.

[Vladimir Prus]

On Friday 14 November 2008 22:41:58 Michael Snyder wrote:
> Vladimir Prus wrote:
> > On Friday 14 November 2008 21:54:46 Michael Snyder wrote:
> > 
> >>>> I'm puzzled by this assert.
> >>>> You don't think we'll ever want to specify both the pid and the thread?
> >>> I think that makes no sense. If a thread is specified, then there's no
> >>> possible use of 'pid'. Threads are globally numbered.
> >> Even if it makes no sense in the sense that
> >> it's not required, that doesn't necessarily make it
> >> an error.  Suppose somebody specifies both the pid and
> >> the thread?  What's the harm?  If they're inconsistent
> >> (this pid does not contain this thread), THEN we'll
> >> return an error.
> > 
> > I think it's better to make functions have as tight preconditions as possible. 
> > In this case, passing both thread and pid does not serve any possible purpose,
> > so it's likely that caller is doing this by mistake. It's best to assert 
> > immediately, rather than spending time and code space verifying if those
> > parameters are consistent.
> 
> I respect your opinion, but MI is not the only caller of this function.
> 
>  > Checking if a thread belongs to a process is not
> > the part of this this function purpose.
> 
> It's input validation.  What you're doing is also input
> validation, it's just imposing a more stringent requirement.
> 
> I feel that an assert is excessively stringent in this context.
> An assert implies an internal gdb error.  These potentially
> conflicting inputs could come about as a result of (foreseeable)
> user input, rather than internal error.  Admittedly not any
> user input that could be given now, but the CLI (or other
> potential clients) could change.
> 
> I feel that if it's possible for these inputs to violate
> the assert without actually reflecting an internally
> inconsistant state, then the assert is too strong.

This is not the question of what *external* inputs, or user-defined
inputs can be meaningful. It's the question of what the function
promises. In my original patch, the function, in its comment, did not
say anything about behaviour in the case where both thread and pid
are not -1. Therefore, any caller of this function that can possible
pass thread!=-1 and pid!=-1 gets undefined behaviour. There are 3 ways
from here:

1. Document that thread!=-1 && pid!=-1 is invalid parameter set of this function.
Add gdb_assert.

2. Document, exactly, the behaviour in thread!=-1 && pid !=-1 case.

3. Leave everything as is -- e.g. with undefined behaviour.

(3) is not good, for obvious reasons. If you don't like (1), then can you specify
what behaviour you want from this function in the thread!=-1 && pid !=-1 case,
so that I can document and implement it?

- Volodya

Re: [RFC] Implement -list-thread-groups.

[Michael Snyder]

Vladimir Prus wrote:
> On Friday 14 November 2008 22:41:58 Michael Snyder wrote:
>> Vladimir Prus wrote:
>>> On Friday 14 November 2008 21:54:46 Michael Snyder wrote:
>>>
>>>>>> I'm puzzled by this assert.
>>>>>> You don't think we'll ever want to specify both the pid and the thread?
>>>>> I think that makes no sense. If a thread is specified, then there's no
>>>>> possible use of 'pid'. Threads are globally numbered.
>>>> Even if it makes no sense in the sense that
>>>> it's not required, that doesn't necessarily make it
>>>> an error.  Suppose somebody specifies both the pid and
>>>> the thread?  What's the harm?  If they're inconsistent
>>>> (this pid does not contain this thread), THEN we'll
>>>> return an error.
>>> I think it's better to make functions have as tight preconditions as possible. 
>>> In this case, passing both thread and pid does not serve any possible purpose,
>>> so it's likely that caller is doing this by mistake. It's best to assert 
>>> immediately, rather than spending time and code space verifying if those
>>> parameters are consistent.
>> I respect your opinion, but MI is not the only caller of this function.
>>
>>  > Checking if a thread belongs to a process is not
>>> the part of this this function purpose.
>> It's input validation.  What you're doing is also input
>> validation, it's just imposing a more stringent requirement.
>>
>> I feel that an assert is excessively stringent in this context.
>> An assert implies an internal gdb error.  These potentially
>> conflicting inputs could come about as a result of (foreseeable)
>> user input, rather than internal error.  Admittedly not any
>> user input that could be given now, but the CLI (or other
>> potential clients) could change.
>>
>> I feel that if it's possible for these inputs to violate
>> the assert without actually reflecting an internally
>> inconsistant state, then the assert is too strong.
> 
> This is not the question of what *external* inputs, or user-defined
> inputs can be meaningful. It's the question of what the function
> promises. In my original patch, the function, in its comment, did not
> say anything about behaviour in the case where both thread and pid
> are not -1. Therefore, any caller of this function that can possible
> pass thread!=-1 and pid!=-1 gets undefined behaviour. There are 3 ways
> from here:
> 
> 1. Document that thread!=-1 && pid!=-1 is invalid parameter set of this function.
> Add gdb_assert.
> 
> 2. Document, exactly, the behaviour in thread!=-1 && pid !=-1 case.
> 
> 3. Leave everything as is -- e.g. with undefined behaviour.
> 
> (3) is not good, for obvious reasons. If you don't like (1), then can you specify
> what behaviour you want from this function in the thread!=-1 && pid !=-1 case,
> so that I can document and implement it?

Sounds good, and well summarized.

What about this for #2:
   1) Look up the thread based on TID as you already do.
   2) Compare the thread's PID with the supplied PID.
   3) If they match, print the result.  If not, return error / not found.

Sound reasonable?

Re: [RFC] Implement -list-thread-groups.

[Vladimir Prus]

On Sunday 16 November 2008 00:12:21 Michael Snyder wrote:
> Vladimir Prus wrote:
> > On Friday 14 November 2008 22:41:58 Michael Snyder wrote:
> >> Vladimir Prus wrote:
> >>> On Friday 14 November 2008 21:54:46 Michael Snyder wrote:
> >>>
> >>>>>> I'm puzzled by this assert.
> >>>>>> You don't think we'll ever want to specify both the pid and the thread?
> >>>>> I think that makes no sense. If a thread is specified, then there's no
> >>>>> possible use of 'pid'. Threads are globally numbered.
> >>>> Even if it makes no sense in the sense that
> >>>> it's not required, that doesn't necessarily make it
> >>>> an error.  Suppose somebody specifies both the pid and
> >>>> the thread?  What's the harm?  If they're inconsistent
> >>>> (this pid does not contain this thread), THEN we'll
> >>>> return an error.
> >>> I think it's better to make functions have as tight preconditions as possible. 
> >>> In this case, passing both thread and pid does not serve any possible purpose,
> >>> so it's likely that caller is doing this by mistake. It's best to assert 
> >>> immediately, rather than spending time and code space verifying if those
> >>> parameters are consistent.
> >> I respect your opinion, but MI is not the only caller of this function.
> >>
> >>  > Checking if a thread belongs to a process is not
> >>> the part of this this function purpose.
> >> It's input validation.  What you're doing is also input
> >> validation, it's just imposing a more stringent requirement.
> >>
> >> I feel that an assert is excessively stringent in this context.
> >> An assert implies an internal gdb error.  These potentially
> >> conflicting inputs could come about as a result of (foreseeable)
> >> user input, rather than internal error.  Admittedly not any
> >> user input that could be given now, but the CLI (or other
> >> potential clients) could change.
> >>
> >> I feel that if it's possible for these inputs to violate
> >> the assert without actually reflecting an internally
> >> inconsistant state, then the assert is too strong.
> > 
> > This is not the question of what *external* inputs, or user-defined
> > inputs can be meaningful. It's the question of what the function
> > promises. In my original patch, the function, in its comment, did not
> > say anything about behaviour in the case where both thread and pid
> > are not -1. Therefore, any caller of this function that can possible
> > pass thread!=-1 and pid!=-1 gets undefined behaviour. There are 3 ways
> > from here:
> > 
> > 1. Document that thread!=-1 && pid!=-1 is invalid parameter set of this function.
> > Add gdb_assert.
> > 
> > 2. Document, exactly, the behaviour in thread!=-1 && pid !=-1 case.
> > 
> > 3. Leave everything as is -- e.g. with undefined behaviour.
> > 
> > (3) is not good, for obvious reasons. If you don't like (1), then can you specify
> > what behaviour you want from this function in the thread!=-1 && pid !=-1 case,
> > so that I can document and implement it?
> 
> Sounds good, and well summarized.
> 
> What about this for #2:
>    1) Look up the thread based on TID as you already do.
>    2) Compare the thread's PID with the supplied PID.
>    3) If they match, print the result.  If not, return error / not found.
> 
> Sound reasonable?

The function does not have a return value, so 'error' is the
only way to do reporting. Is that what you suggest?

- Volodya

Re: [RFC] Implement -list-thread-groups.

[Vladimir Prus]

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

On Sunday 16 November 2008 01:14:01 Michael Snyder wrote:
> Excuse the top post.  Yes, calling error then.

Here's the final patch. OK?

- Volodya

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

commit 58e34ce9109b3dfc10ddba4fe21476caa373f764
Author: vladimir <vladimir@e7755896-6108-0410-9592-8049d3e74e28>
Date:   Thu Jul 31 13:23:28 2008 +0000

    Implement -list-thread-groups.
    
    	* thread.c (print_thread_info): New parameter pid, to print
    	threads of specific process.
    	* gdbthread.h (print_thread_info): New parameter pid.
    	* mi/mi-cmds.c (mi_cmds): Register -list-thread-groups.
    	* mi/mi-cmds.h (mi_cmd_list_thread_groups): New.
    	* mi/mi-main.c (mi_cmd_thread_info): Adjust.
    	(print_one_process, mi_cmd_list_thread_groups): New.

diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index 55c848d..cac20f7 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -295,7 +295,8 @@ extern struct cmd_list_element *thread_cmd_list;
    `set print thread-events'.  */
 extern int print_thread_events;
 
-extern void print_thread_info (struct ui_out *uiout, int thread);
+extern void print_thread_info (struct ui_out *uiout, int thread,
+			       int pid);
 
 extern struct cleanup *make_cleanup_restore_current_thread (void);
 
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index ca0f428..d38de35 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -90,6 +90,7 @@ struct mi_cmd mi_cmds[] =
   { "interpreter-exec", { NULL, 0 }, mi_cmd_interpreter_exec},
   { "list-features", { NULL, 0 }, mi_cmd_list_features},
   { "list-target-features", { NULL, 0 }, mi_cmd_list_target_features},
+  { "list-thread-groups", { NULL, 0 }, mi_cmd_list_thread_groups },  
   { "overlay-auto", { NULL, 0 }, NULL },
   { "overlay-list-mapping-state", { NULL, 0 }, NULL },
   { "overlay-list-overlays", { NULL, 0 }, NULL },
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index 16887ae..a9bb1e0 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -67,6 +67,7 @@ extern mi_cmd_argv_ftype mi_cmd_inferior_tty_show;
 extern mi_cmd_argv_ftype mi_cmd_interpreter_exec;
 extern mi_cmd_argv_ftype mi_cmd_list_features;
 extern mi_cmd_argv_ftype mi_cmd_list_target_features;
+extern mi_cmd_argv_ftype mi_cmd_list_thread_groups;
 extern mi_cmd_argv_ftype mi_cmd_stack_info_depth;
 extern mi_cmd_argv_ftype mi_cmd_stack_info_frame;
 extern mi_cmd_argv_ftype mi_cmd_stack_list_args;
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index a9fbcad..43ec0b4 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -46,6 +46,7 @@
 #include "mi-main.h"
 #include "language.h"
 #include "valprint.h"
+#include "inferior.h"
 
 #include <ctype.h>
 #include <sys/time.h>
@@ -245,7 +246,55 @@ mi_cmd_thread_info (char *command, char **argv, int argc)
   if (argc == 1)
     thread = atoi (argv[0]);
 
-  print_thread_info (uiout, thread);
+  print_thread_info (uiout, thread, -1);
+}
+
+static int
+print_one_inferior (struct inferior *inferior, void *arg)
+{
+  struct cleanup *back_to = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
+
+  ui_out_field_fmt (uiout, "id", "%d", inferior->pid);
+  ui_out_field_string (uiout, "type", "process");
+  ui_out_field_int (uiout, "pid", inferior->pid);
+  
+  do_cleanups (back_to);
+  return 0;
+}
+
+void
+mi_cmd_list_thread_groups (char *command, char **argv, int argc)
+{
+  struct cleanup *back_to;
+  int available = 0;
+  char *id = NULL;
+
+  if (argc > 0 && strcmp (argv[0], "--available") == 0)
+    {
+      ++argv;
+      --argc;
+      available = 1;
+    }
+
+  if (argc > 0)
+    id = argv[0];
+
+  back_to = make_cleanup (&null_cleanup, NULL);
+
+  if (id)
+    {
+      int pid = atoi (id);
+      if (!in_inferior_list (pid))
+	error ("Invalid thread group id '%s'", id);
+      print_thread_info (uiout, -1, pid);    
+    }
+  else
+    {
+      make_cleanup_ui_out_list_begin_end (uiout, "groups");
+      iterate_over_inferiors (print_one_inferior, NULL);
+    }
+  
+  do_cleanups (back_to);
 }
 
 void
diff --git a/gdb/thread.c b/gdb/thread.c
index b1e318d..1f50e6a 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -636,9 +636,14 @@ set_stop_requested (ptid_t ptid, int stop)
    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.  */
+   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)
+print_thread_info (struct ui_out *uiout, int requested_thread, int pid)
 {
   struct thread_info *tp;
   ptid_t current_ptid;
@@ -661,6 +666,13 @@ print_thread_info (struct ui_out *uiout, int requested_thread)
       if (requested_thread != -1 && tp->num != requested_thread)
 	continue;
 
+      if (pid != -1 && PIDGET (tp->ptid) != pid)
+	{
+	  if (requested_thread != -1)
+	    error (_("Requested thread not found in requested process"));
+	  continue;
+	}
+
       if (ptid_equal (tp->ptid, current_ptid))
 	current_thread = tp->num;
 
@@ -715,7 +727,7 @@ print_thread_info (struct ui_out *uiout, int requested_thread)
      the "info threads" command.  */
   do_cleanups (old_chain);
 
-  if (requested_thread == -1)
+  if (pid == -1 && requested_thread == -1 )
     {
       gdb_assert (current_thread != -1
 		  || !thread_list);
@@ -740,7 +752,7 @@ The current thread <Thread ID %d> has terminated.  See `help thread'.\n",
 static void
 info_threads_command (char *arg, int from_tty)
 {
-  print_thread_info (uiout, -1);
+  print_thread_info (uiout, -1, -1);
 }
 
 /* Switch from one thread to another. */

Re: [RFC] Implement -list-thread-groups.

[Michael Snyder]

Vladimir Prus wrote:
> On Sunday 16 November 2008 01:14:01 Michael Snyder wrote:
>> Excuse the top post.  Yes, calling error then.
> 
> Here's the final patch. OK?

I'm OK with the thread.c/gdbthread.h parts.
Thanks Volodya


> ------------------------------------------------------------------------
> 
> commit 58e34ce9109b3dfc10ddba4fe21476caa373f764
> Author: vladimir <vladimir@e7755896-6108-0410-9592-8049d3e74e28>
> Date:   Thu Jul 31 13:23:28 2008 +0000
> 
>     Implement -list-thread-groups.
>     
>     	* thread.c (print_thread_info): New parameter pid, to print
>     	threads of specific process.
>     	* gdbthread.h (print_thread_info): New parameter pid.
>     	* mi/mi-cmds.c (mi_cmds): Register -list-thread-groups.
>     	* mi/mi-cmds.h (mi_cmd_list_thread_groups): New.
>     	* mi/mi-main.c (mi_cmd_thread_info): Adjust.
>     	(print_one_process, mi_cmd_list_thread_groups): New.
> 
> diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
> index 55c848d..cac20f7 100644
> --- a/gdb/gdbthread.h
> +++ b/gdb/gdbthread.h
> @@ -295,7 +295,8 @@ extern struct cmd_list_element *thread_cmd_list;
>     `set print thread-events'.  */
>  extern int print_thread_events;
>  
> -extern void print_thread_info (struct ui_out *uiout, int thread);
> +extern void print_thread_info (struct ui_out *uiout, int thread,
> +			       int pid);
>  
>  extern struct cleanup *make_cleanup_restore_current_thread (void);
>  
> diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
> index ca0f428..d38de35 100644
> --- a/gdb/mi/mi-cmds.c
> +++ b/gdb/mi/mi-cmds.c
> @@ -90,6 +90,7 @@ struct mi_cmd mi_cmds[] =
>    { "interpreter-exec", { NULL, 0 }, mi_cmd_interpreter_exec},
>    { "list-features", { NULL, 0 }, mi_cmd_list_features},
>    { "list-target-features", { NULL, 0 }, mi_cmd_list_target_features},
> +  { "list-thread-groups", { NULL, 0 }, mi_cmd_list_thread_groups },  
>    { "overlay-auto", { NULL, 0 }, NULL },
>    { "overlay-list-mapping-state", { NULL, 0 }, NULL },
>    { "overlay-list-overlays", { NULL, 0 }, NULL },
> diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
> index 16887ae..a9bb1e0 100644
> --- a/gdb/mi/mi-cmds.h
> +++ b/gdb/mi/mi-cmds.h
> @@ -67,6 +67,7 @@ extern mi_cmd_argv_ftype mi_cmd_inferior_tty_show;
>  extern mi_cmd_argv_ftype mi_cmd_interpreter_exec;
>  extern mi_cmd_argv_ftype mi_cmd_list_features;
>  extern mi_cmd_argv_ftype mi_cmd_list_target_features;
> +extern mi_cmd_argv_ftype mi_cmd_list_thread_groups;
>  extern mi_cmd_argv_ftype mi_cmd_stack_info_depth;
>  extern mi_cmd_argv_ftype mi_cmd_stack_info_frame;
>  extern mi_cmd_argv_ftype mi_cmd_stack_list_args;
> diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
> index a9fbcad..43ec0b4 100644
> --- a/gdb/mi/mi-main.c
> +++ b/gdb/mi/mi-main.c
> @@ -46,6 +46,7 @@
>  #include "mi-main.h"
>  #include "language.h"
>  #include "valprint.h"
> +#include "inferior.h"
>  
>  #include <ctype.h>
>  #include <sys/time.h>
> @@ -245,7 +246,55 @@ mi_cmd_thread_info (char *command, char **argv, int argc)
>    if (argc == 1)
>      thread = atoi (argv[0]);
>  
> -  print_thread_info (uiout, thread);
> +  print_thread_info (uiout, thread, -1);
> +}
> +
> +static int
> +print_one_inferior (struct inferior *inferior, void *arg)
> +{
> +  struct cleanup *back_to = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
> +
> +  ui_out_field_fmt (uiout, "id", "%d", inferior->pid);
> +  ui_out_field_string (uiout, "type", "process");
> +  ui_out_field_int (uiout, "pid", inferior->pid);
> +  
> +  do_cleanups (back_to);
> +  return 0;
> +}
> +
> +void
> +mi_cmd_list_thread_groups (char *command, char **argv, int argc)
> +{
> +  struct cleanup *back_to;
> +  int available = 0;
> +  char *id = NULL;
> +
> +  if (argc > 0 && strcmp (argv[0], "--available") == 0)
> +    {
> +      ++argv;
> +      --argc;
> +      available = 1;
> +    }
> +
> +  if (argc > 0)
> +    id = argv[0];
> +
> +  back_to = make_cleanup (&null_cleanup, NULL);
> +
> +  if (id)
> +    {
> +      int pid = atoi (id);
> +      if (!in_inferior_list (pid))
> +	error ("Invalid thread group id '%s'", id);
> +      print_thread_info (uiout, -1, pid);    
> +    }
> +  else
> +    {
> +      make_cleanup_ui_out_list_begin_end (uiout, "groups");
> +      iterate_over_inferiors (print_one_inferior, NULL);
> +    }
> +  
> +  do_cleanups (back_to);
>  }
>  
>  void
> diff --git a/gdb/thread.c b/gdb/thread.c
> index b1e318d..1f50e6a 100644
> --- a/gdb/thread.c
> +++ b/gdb/thread.c
> @@ -636,9 +636,14 @@ set_stop_requested (ptid_t ptid, int stop)
>     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.  */
> +   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)
> +print_thread_info (struct ui_out *uiout, int requested_thread, int pid)
>  {
>    struct thread_info *tp;
>    ptid_t current_ptid;
> @@ -661,6 +666,13 @@ print_thread_info (struct ui_out *uiout, int requested_thread)
>        if (requested_thread != -1 && tp->num != requested_thread)
>  	continue;
>  
> +      if (pid != -1 && PIDGET (tp->ptid) != pid)
> +	{
> +	  if (requested_thread != -1)
> +	    error (_("Requested thread not found in requested process"));
> +	  continue;
> +	}
> +
>        if (ptid_equal (tp->ptid, current_ptid))
>  	current_thread = tp->num;
>  
> @@ -715,7 +727,7 @@ print_thread_info (struct ui_out *uiout, int requested_thread)
>       the "info threads" command.  */
>    do_cleanups (old_chain);
>  
> -  if (requested_thread == -1)
> +  if (pid == -1 && requested_thread == -1 )
>      {
>        gdb_assert (current_thread != -1
>  		  || !thread_list);
> @@ -740,7 +752,7 @@ The current thread <Thread ID %d> has terminated.  See `help thread'.\n",
>  static void
>  info_threads_command (char *arg, int from_tty)
>  {
> -  print_thread_info (uiout, -1);
> +  print_thread_info (uiout, -1, -1);
>  }
>  
>  /* Switch from one thread to another. */

Re: [RFC] Implement -list-thread-groups.

[Vladimir Prus]

On Monday 17 November 2008 05:17:30 Michael Snyder wrote:
> Vladimir Prus wrote:
> > On Sunday 16 November 2008 01:14:01 Michael Snyder wrote:
> >> Excuse the top post.  Yes, calling error then.
> >
> > Here's the final patch. OK?
>
> I'm OK with the thread.c/gdbthread.h parts.

Checked in.

- Volodya

Re: [RFC] Implement -list-thread-groups.

[Pedro Alves]

On Friday 14 November 2008 17:28:43, Vladimir Prus wrote:
> Done. Indicentally, I know how to configure Emacs to remove trailing 
> whitespace on save. But pretty much every source file in gdb already has such 
> lines. If somebody tell me how to make Emacs not add lines without trailing
> whitespace, while *not* changing existing lines, it would be gtreat.

I just do 'quilt refresh --strip-trailing-whitespace' which gets rid
of most of the extra whitespace *I'm touching*, but leaves the rest
untouched.  Won't work if you don't use quilt.  :-)

In addition, in emacs, I tend to switch into whitespace-mode when I'm
doing final editing of my patches, which lets you easily see all the
redundant and wrong indentation in full color.

-- 
Pedro Alves

Re: [RFC] Implement -list-thread-groups.

[Joel Brobecker]

> Sorry if I'm trying to circumvent the rules. On the other hand, I
> believe I was told that MI boundaries are logical, not physical. In
> that sense, a change that clearly matters only for MI, and is actually
> posted for comments, is kind of a boundary case.
> 
> Another example are observes. Although generic in nature, they are quickly
> becoming de-facto interface of GDB core with MI.

I agree there is no absolute rules. But I have to disagree with the
observers. Maybe many of them are being used by MI, but several others
are not. At AdaCore, we have some local VxWorks ports that we developped
which rely on oberver notifications as well. There are still a couple
of areas that need polishing, otherwise we'd have submitted this port
for inclusion as well.

> Done. Indicentally, I know how to configure Emacs to remove trailing
> whitespace on save. But pretty much every source file in gdb already
> has such lines. If somebody tell me how to make Emacs not add lines
> without trailing whitespace, while *not* changing existing lines, it
> would be gtreat.

Yeah - I try to be careful about not making it worse, but until we
have automated style checks at checkin time, I think it's a lost
battle.

> +   If PID is not -1, only prints threads from the process PID.  */

                             ^^^^^^ print

Since Michael commented on the following assertion:

> +  gdb_assert (requested_thead == -1 || pid == -1);
                           ^^^^^ Ooops, typo???

I think it's important to explain it in the function description.
Can you add a line or two explaining at the end of the function
description explaining that it doesn't make sense to provide both
the pid and the thread at the same time, and thus at least one
of the two must be set to 1.

The rest looks good to me. I don't remember if I pre-approved the patch
the previous time I reviewed your patch, so I'll just do it again now.

-- 
Joel

Re: [RFC] Implement -list-thread-groups.

[Joel Brobecker]

> Since Michael commented on the following assertion:
> 
> > +  gdb_assert (requested_thead == -1 || pid == -1);
>                            ^^^^^ Ooops, typo???
> 
> I think it's important to explain it in the function description.
> Can you add a line or two explaining at the end of the function
> description explaining that it doesn't make sense to provide both
> the pid and the thread at the same time, and thus at least one
> of the two must be set to 1.

BTW: I should have said that I'm neutral to what the function should
do in case the two parameters are not -1. I'll let you guys decide
what to do in that case, and I'm fine with doing that as a followup
patch (the consequences of this decision are not immediately visible,
I believe).

-- 
Joel