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

[Vladimir Prus <vladimir@codesourcery.com>]

[-- 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. */