[RFA] Implement -thread-info.

[RFA] Implement -thread-info.

[Vladimir Prus]

Presently, the MI -thread-info and -thread-list-all-threads
commands are not implemented, so a frontend wishing to know
the state of all threads upon stop is required to manually iterate
over threads, or use CLI. This patch implements -thread-info,
that prints essentially the same information as 'info thread' in CLI.
The new command can either print information for all threads, or
for a specific one provided as argument, making -thread-list-all-threads
not necessary. The sample output is:

^done,
   threads=[
    {id="2",target-id="Thread 0xb7e1eb90 (LWP 21429)",frame={addr="0xffffe410",func="__kernel_vsyscall",args=[]}},
    {id="1",target-id="Thread 0xb7e1f6b0 (LWP 21426)",frame={...}}],
   current-thread-id="1"

I'll provide docs patch if the code patch is approved. OK?

- Volodya

	* gdb.h (mi_info_threads): Declare.
	* thread.c (mi_info_threads): New.

	* mi/mi-cmds.c (mi_cmds): Specify a handler
	for -thread-info.
	* mi/mi-cmds.h (mi_cmd_thread_info): Declare.
	* mi/mi-main.c (mi_cmd_thread_info): New.
	(mi_cmd_list_features): Include 'thread-info'.
---
 gdb/gdb.h        |    2 +
 gdb/mi/mi-cmds.c |    3 +-
 gdb/mi/mi-cmds.h |    1 +
 gdb/mi/mi-main.c |   19 +++++++++++++++++
 gdb/thread.c     |   60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/gdb/gdb.h b/gdb/gdb.h
index fcd3e3b..13523fb 100644
--- a/gdb/gdb.h
+++ b/gdb/gdb.h
@@ -55,4 +55,6 @@ enum gdb_rc gdb_thread_select (struct ui_out *uiout, char *tidstr,
 enum gdb_rc gdb_list_thread_ids (struct ui_out *uiout,
 				 char **error_message);
 
+extern void mi_info_threads (struct ui_out *uiout, int thread);
+
 #endif
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index c651694..89c6376 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -130,8 +130,7 @@ struct mi_cmd mi_cmds[] =
   { "target-list-current-targets", { NULL, 0 }, NULL, NULL },
   { "target-list-parameters", { NULL, 0 }, NULL, NULL },
   { "target-select", { NULL, 0 }, mi_cmd_target_select},
-  { "thread-info", { NULL, 0 }, NULL, NULL },
-  { "thread-list-all-threads", { NULL, 0 }, NULL, NULL },
+  { "thread-info", { NULL, 0 }, NULL, mi_cmd_thread_info },
   { "thread-list-ids", { NULL, 0 }, 0, mi_cmd_thread_list_ids},
   { "thread-select", { NULL, 0 }, 0, mi_cmd_thread_select},
   { "trace-actions", { NULL, 0 }, NULL, NULL },
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index 2d0393b..6a033e5 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -105,6 +105,7 @@ extern mi_cmd_argv_ftype mi_cmd_target_file_get;
 extern mi_cmd_argv_ftype mi_cmd_target_file_put;
 extern mi_cmd_argv_ftype mi_cmd_target_file_delete;
 extern mi_cmd_args_ftype mi_cmd_target_select;
+extern mi_cmd_argv_ftype mi_cmd_thread_info;
 extern mi_cmd_argv_ftype mi_cmd_thread_list_ids;
 extern mi_cmd_argv_ftype mi_cmd_thread_select;
 extern mi_cmd_argv_ftype mi_cmd_var_assign;
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 41e12d2..5d89847 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -277,6 +277,24 @@ mi_cmd_thread_list_ids (char *command, char **argv, int argc)
 }
 
 enum mi_cmd_result
+mi_cmd_thread_info (char *command, char **argv, int argc)
+{
+  int thread = -1;
+  
+  if (argc != 0 && argc != 1)
+    {
+      mi_error_message = xstrprintf ("Invalid MI command");
+      return MI_CMD_ERROR;
+    }
+
+  if (argc == 1)
+    thread = atoi (argv[0]);
+
+  mi_info_threads (uiout, thread);
+  return MI_CMD_DONE;
+}
+
+enum mi_cmd_result
 mi_cmd_data_list_register_names (char *command, char **argv, int argc)
 {
   int regnum, numregs;
@@ -1055,6 +1073,7 @@ mi_cmd_list_features (char *command, char **argv, int argc)
 
       ui_out_field_string (uiout, NULL, "frozen-varobjs");
       ui_out_field_string (uiout, NULL, "pending-breakpoints");
+      ui_out_field_string (uiout, NULL, "thread-info");
       
       do_cleanups (cleanup);
 
diff --git a/gdb/thread.c b/gdb/thread.c
index 3a39703..d11ef00 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -471,6 +471,66 @@ info_threads_command (char *arg, int from_tty)
     }
 }
 
+/* Prints the list of threads and their details on UIOUT.
+   This is a version of 'info_thread_command' suitable for
+   use from MI.  
+   If REQESTED_THREAD is not -1, it's the GDB id of the thread
+   that should be printed.  Otherwise, all threads are
+   printed.  */
+void
+mi_info_threads (struct ui_out *uiout, int requested_thread)
+{
+  struct thread_info *tp;
+  ptid_t current_ptid;
+  int current_thread = -1;
+  struct cleanup *cleanup_chain;
+  char *extra_info;
+  struct frame_id saved_frame_id;
+
+  saved_frame_id = get_frame_id (get_selected_frame (NULL));
+  cleanup_chain = make_cleanup_restore_current_thread (inferior_ptid, 
+						       saved_frame_id);
+
+  prune_threads ();
+  target_find_new_threads ();
+  current_ptid = inferior_ptid;
+  cleanup_chain = make_cleanup_ui_out_list_begin_end (uiout, "threads");
+
+  for (tp = thread_list; tp; tp = tp->next)
+    {
+      struct cleanup *cleanup2;
+
+      if (requested_thread != -1 && tp->num != requested_thread)
+	continue;
+
+      cleanup2 = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
+
+      if (ptid_equal (tp->ptid, current_ptid))
+	current_thread = tp->num;
+
+      ui_out_field_int (uiout, "id", tp->num);
+      ui_out_field_string (uiout, "target-id", target_tid_to_str (tp->ptid));
+
+      extra_info = target_extra_thread_info (tp);
+      if (extra_info)
+	ui_out_field_string (uiout, "details", extra_info);
+
+      /* That switch put us at the top of the stack (leaf frame).  */
+      switch_to_thread (tp->ptid);
+      print_stack_frame (get_selected_frame (NULL), 0, LOCATION);
+
+      do_cleanups (cleanup2);
+    }
+  do_cleanups (cleanup_chain);
+  
+  if (requested_thread == -1)
+    {
+      gdb_assert (current_thread != -1);
+      ui_out_field_int (uiout, "current-thread-id", current_thread);
+    }
+}
+
+
 /* Switch from one thread to another. */
 
 void
-- 
1.5.3.5

Re: [RFA] Implement -thread-info.

[Nick Roberts]

 > Presently, the MI -thread-info and -thread-list-all-threads
 > commands are not implemented, so a frontend wishing to know
 > the state of all threads upon stop is required to manually iterate
 > over threads, or use CLI. This patch implements -thread-info,
 > that prints essentially the same information as 'info thread' in CLI.
 > The new command can either print information for all threads, or
 > for a specific one provided as argument, making -thread-list-all-threads
 > not necessary.

mi_info_threads is nearly the same function as thread_command.  Can this be
used in a dual way just as -break-list uses breakpoint_1?

Also Denis Pilat has already proposed a patch for -thread-info:
http://sourceware.org/ml/gdb-patches/2007-03/msg00167.html

How does your patch compare?  (assuming the problem of return type is solved
as has been done for thread_select).

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFA] Implement -thread-info.

[Vladimir Prus]

On Monday 18 February 2008 09:25:29 Nick Roberts wrote:
>  > Presently, the MI -thread-info and -thread-list-all-threads
>  > commands are not implemented, so a frontend wishing to know
>  > the state of all threads upon stop is required to manually iterate
>  > over threads, or use CLI. This patch implements -thread-info,
>  > that prints essentially the same information as 'info thread' in CLI.
>  > The new command can either print information for all threads, or
>  > for a specific one provided as argument, making -thread-list-all-threads
>  > not necessary.
> 
> mi_info_threads is nearly the same function as thread_command.  

You probably meant info_threads_command?

> Can this be 
> used in a dual way just as -break-list uses breakpoint_1?

I don't know -- I actually have no idea what magic makes breakpoint_1 to
work both for MI and CLI. Do you happen to know?

> Also Denis Pilat has already proposed a patch for -thread-info:
> http://sourceware.org/ml/gdb-patches/2007-03/msg00167.html

I did not notice that.

> 
> How does your patch compare?  (assuming the problem of return type is solved
> as has been done for thread_select).

It appears that my patch:

1. Does not bother with making non-throwing function, as MI top-level can
handle exceptions.

2. Allows to print information about all threads.

I don't know what problem of return type you refer to -- can you clarify?

- Volodya

 

Re: [RFA] Implement -thread-info.

[Nick Roberts]

 > > mi_info_threads is nearly the same function as thread_command.  
 > 
 > You probably meant info_threads_command?

Yes.

 > > Can this be 
 > > used in a dual way just as -break-list uses breakpoint_1?
 > 
 > I don't know -- I actually have no idea what magic makes breakpoint_1 to
 > work both for MI and CLI. Do you happen to know?

It just seems to be a kind of overloading where the output format of
functions like ui_out_field_string depends on the interpreter used (value
of uiout).  In cases where only one interpreter outputs anything
ui_out_is_mi_like_p is used.

 > > Also Denis Pilat has already proposed a patch for -thread-info:
 > > http://sourceware.org/ml/gdb-patches/2007-03/msg00167.html
 > 
 > I did not notice that.
 > 
 > > 
 > > How does your patch compare?  (assuming the problem of return type is solved
 > > as has been done for thread_select).
 > 
 > It appears that my patch:
 > 
 > 1. Does not bother with making non-throwing function, as MI top-level can
 > handle exceptions.
 > 
 > 2. Allows to print information about all threads.

I see.  Denis also proposed something for -thread-list-all-threads
(http://sourceware.org/ml/gdb-patches/2007-03/msg00144.html)

Do you not want to print the frame level?  Also do you want to
catch errors in print_stack_frame?  Rather than:

+      print_stack_frame (get_selected_frame (NULL), 0, LOCATION);

Would it be better to use:

+      print_frame_info (get_selected_frame (NULL), 1, LOC_AND_ADDRESS, 0);

?

Note that with MI (following the thread with Denis) that the address gets
printed anyway when print_stack_frame is used:

  args.print_what = ui_out_is_mi_like_p (uiout) ? LOC_AND_ADDRESS : print_what;

 > I don't know what problem of return type you refer to -- can you clarify?

That, in Denis' patch, gdb_thread_info returns type enum gdb_rc but
catch_exceptions_with_msg returns type int.  (just like gdb_breakpoint and
break_command_really did for a while).

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFA] Implement -thread-info.

[Vladimir Prus]

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

On Tuesday 19 February 2008 00:44:33 Nick Roberts wrote:
>  > > mi_info_threads is nearly the same function as thread_command.  
>  > 
>  > You probably meant info_threads_command?
> 
> Yes.
> 
>  > > Can this be 
>  > > used in a dual way just as -break-list uses breakpoint_1?
>  > 
>  > I don't know -- I actually have no idea what magic makes breakpoint_1 to
>  > work both for MI and CLI. Do you happen to know?
> 
> It just seems to be a kind of overloading where the output format of
> functions like ui_out_field_string depends on the interpreter used (value
> of uiout).  

Okay, I've tried to do that. Now, I'm using ui_out_text to ensure empty
spaces between fields. breakpoint.c uses ui_out_table_begin_end, but
I don't think the formatting that one produces is good here.

> In cases where only one interpreter outputs anything 
> ui_out_is_mi_like_p is used.
> 
>  > > Also Denis Pilat has already proposed a patch for -thread-info:
>  > > http://sourceware.org/ml/gdb-patches/2007-03/msg00167.html
>  > 
>  > I did not notice that.
>  > 
>  > > 
>  > > How does your patch compare?  (assuming the problem of return type is solved
>  > > as has been done for thread_select).
>  > 
>  > It appears that my patch:
>  > 
>  > 1. Does not bother with making non-throwing function, as MI top-level can
>  > handle exceptions.
>  > 
>  > 2. Allows to print information about all threads.
> 
> I see.  Denis also proposed something for -thread-list-all-threads
> (http://sourceware.org/ml/gdb-patches/2007-03/msg00144.html)

I think having one command is a bit better.

> Do you not want to print the frame level?  

Probably not; this command is useful right after stop, when the level is always 0.

> Also do you want to 
> catch errors in print_stack_frame?

Yes, why not? A typical error is when we have a char* parameter pointing to
something not very accessible, in which case I'd prefer print_stack_frame
to catch this error, as opposed to -thread-info outright dying. 


> Rather than: 
> 
> +      print_stack_frame (get_selected_frame (NULL), 0, LOCATION);
> 
> Would it be better to use:
> 
> +      print_frame_info (get_selected_frame (NULL), 1, LOC_AND_ADDRESS, 0);
> 
> ?
> 
> Note that with MI (following the thread with Denis) that the address gets
> printed anyway when print_stack_frame is used:
> 
>   args.print_what = ui_out_is_mi_like_p (uiout) ? LOC_AND_ADDRESS : print_what;

Then, using LOC_AND_ADDRESS in print_stack_frame invocation will only
be more clear, without affecting behaviour? But I guess it's better, indeed.

> 
>  > I don't know what problem of return type you refer to -- can you clarify?
> 
> That, in Denis' patch, gdb_thread_info returns type enum gdb_rc but
> catch_exceptions_with_msg returns type int.  (just like gdb_breakpoint and
> break_command_really did for a while).

This problem does not exist in my patch, as catch_exceptions_with_msg is
just not used.

I attach the revised version of the patch, including docs. Eli, is the
doc part OK? I intend to commit the code part in a week unless there are
objections.

- Volodya

	[gdb]
	Implement -thread-info.
        * gdb.h (mi_info_threads): Declare.

        * thread.c (mi_info_threads): New, extracted
        from info_threads_command and adjusted to
        work for CLI and MI.
        (info_threads_command): Use mi_info_threads.

        * mi/mi-cmds.c (mi_cmds): Specify a handler
        for -thread-info.
        * mi/mi-cmds.h (mi_cmd_thread_info): Declare.
        * mi/mi-main.c (mi_cmd_thread_info): New.
        (mi_cmd_list_features): Include 'thread-info'.

        [gdb/doc]
        * gdb.texinfo (Thread Commands): Document
        -thread-info.  Remove -thread-list-all-threads.

[-- Attachment #2: 0001-Implement-thread-info.patch --]
[-- Type: text/x-diff, Size: 9271 bytes --]

From b194235c305682ab6e4176df70dc52533e8b2e07 Mon Sep 17 00:00:00 2001
From: Vladimir Prus <vladimir@codesourcery.com>
Date: Sun, 17 Feb 2008 18:16:02 +0300
Subject: [RFA] Implement -thread-info.
To: gdb-patches@sources.redhat.com
X-KMail-Transport: CodeSourcery
X-KMail-Identity: 901867920

	* gdb.h (mi_info_threads): Declare.

	* thread.c (mi_info_threads): New, extracted
	from info_threads_command and adjusted to
	work for CLI and MI.
	(info_threads_command): Use mi_info_threads.

	* mi/mi-cmds.c (mi_cmds): Specify a handler
	for -thread-info.
	* mi/mi-cmds.h (mi_cmd_thread_info): Declare.
	* mi/mi-main.c (mi_cmd_thread_info): New.
	(mi_cmd_list_features): Include 'thread-info'.

	doc/
	* gdb.texinfo (Thread Commands): Document
	-thread-info.  Remove -thread-list-all-threads.
---
 gdb/doc/gdb.texinfo |   35 ++++++++++++-------------
 gdb/gdb.h           |    2 +
 gdb/mi/mi-cmds.c    |    3 +-
 gdb/mi/mi-cmds.h    |    1 +
 gdb/mi/mi-main.c    |   19 +++++++++++++
 gdb/thread.c        |   71 ++++++++++++++++++++++++++++++++++++++++-----------
 6 files changed, 96 insertions(+), 35 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 8754350..66480d8 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -18854,34 +18854,33 @@ The corresponding @value{GDBN} command is @samp{pwd}.
 @subsubheading Synopsis
 
 @smallexample
- -thread-info
+ -thread-info [ @var{thread-id} ]
 @end smallexample
 
+Reports the information about either a specific thread, if 
+the @var{thread-id} parameter is present, or about all
+threads.  When printing information about all threads,
+also reports the current thread.
+
 @subsubheading @value{GDBN} Command
 
-No equivalent.
+The @samp{info thread} command prints the same information
+about all threads.
 
 @subsubheading Example
-N.A.
-
-
-@subheading The @code{-thread-list-all-threads} Command
-@findex -thread-list-all-threads
-
-@subsubheading Synopsis
 
 @smallexample
- -thread-list-all-threads
+-thread-info
+^done,threads=[
+@{id="2",target-id="Thread 0xb7e14b90 (LWP 21257)",
+   frame=@{addr="0xffffe410",func="__kernel_vsyscall",args=[]@},
+@{id="1",target-id="Thread 0xb7e156b0 (LWP 21254)",
+   frame=@{addr="0x0804891f",func="foo",args=[@{name="i",value="10"@}],
+           file="/tmp/a.c",fullname="/tmp/a.c",line="158"@}@}],
+current-thread-id="1"
+(gdb)
 @end smallexample
 
-@subsubheading @value{GDBN} Command
-
-The equivalent @value{GDBN} command is @samp{info threads}.
-
-@subsubheading Example
-N.A.
-
-
 @subheading The @code{-thread-list-ids} Command
 @findex -thread-list-ids
 
diff --git a/gdb/gdb.h b/gdb/gdb.h
index fcd3e3b..13523fb 100644
--- a/gdb/gdb.h
+++ b/gdb/gdb.h
@@ -55,4 +55,6 @@ enum gdb_rc gdb_thread_select (struct ui_out *uiout, char *tidstr,
 enum gdb_rc gdb_list_thread_ids (struct ui_out *uiout,
 				 char **error_message);
 
+extern void mi_info_threads (struct ui_out *uiout, int thread);
+
 #endif
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index c651694..89c6376 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -130,8 +130,7 @@ struct mi_cmd mi_cmds[] =
   { "target-list-current-targets", { NULL, 0 }, NULL, NULL },
   { "target-list-parameters", { NULL, 0 }, NULL, NULL },
   { "target-select", { NULL, 0 }, mi_cmd_target_select},
-  { "thread-info", { NULL, 0 }, NULL, NULL },
-  { "thread-list-all-threads", { NULL, 0 }, NULL, NULL },
+  { "thread-info", { NULL, 0 }, NULL, mi_cmd_thread_info },
   { "thread-list-ids", { NULL, 0 }, 0, mi_cmd_thread_list_ids},
   { "thread-select", { NULL, 0 }, 0, mi_cmd_thread_select},
   { "trace-actions", { NULL, 0 }, NULL, NULL },
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index 2d0393b..6a033e5 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -105,6 +105,7 @@ extern mi_cmd_argv_ftype mi_cmd_target_file_get;
 extern mi_cmd_argv_ftype mi_cmd_target_file_put;
 extern mi_cmd_argv_ftype mi_cmd_target_file_delete;
 extern mi_cmd_args_ftype mi_cmd_target_select;
+extern mi_cmd_argv_ftype mi_cmd_thread_info;
 extern mi_cmd_argv_ftype mi_cmd_thread_list_ids;
 extern mi_cmd_argv_ftype mi_cmd_thread_select;
 extern mi_cmd_argv_ftype mi_cmd_var_assign;
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 41e12d2..5d89847 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -277,6 +277,24 @@ mi_cmd_thread_list_ids (char *command, char **argv, int argc)
 }
 
 enum mi_cmd_result
+mi_cmd_thread_info (char *command, char **argv, int argc)
+{
+  int thread = -1;
+  
+  if (argc != 0 && argc != 1)
+    {
+      mi_error_message = xstrprintf ("Invalid MI command");
+      return MI_CMD_ERROR;
+    }
+
+  if (argc == 1)
+    thread = atoi (argv[0]);
+
+  mi_info_threads (uiout, thread);
+  return MI_CMD_DONE;
+}
+
+enum mi_cmd_result
 mi_cmd_data_list_register_names (char *command, char **argv, int argc)
 {
   int regnum, numregs;
@@ -1055,6 +1073,7 @@ mi_cmd_list_features (char *command, char **argv, int argc)
 
       ui_out_field_string (uiout, NULL, "frozen-varobjs");
       ui_out_field_string (uiout, NULL, "pending-breakpoints");
+      ui_out_field_string (uiout, NULL, "thread-info");
       
       do_cleanups (cleanup);
 
diff --git a/gdb/thread.c b/gdb/thread.c
index 3a39703..1f4b099 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -416,15 +416,14 @@ prune_threads (void)
     }
 }
 
-/* Print information about currently known threads 
-
- * Note: this has the drawback that it _really_ switches
- *       threads, which frees the frame cache.  A no-side
- *       effects info-threads command would be nicer.
- */
-
-static void
-info_threads_command (char *arg, int from_tty)
+/* Prints the list of threads and their details on UIOUT.
+   This is a version of 'info_thread_command' suitable for
+   use from MI.  
+   If REQESTED_THREAD is not -1, it's the GDB id of the thread
+   that should be printed.  Otherwise, all threads are
+   printed.  */
+void
+mi_info_threads (struct ui_out *uiout, int requested_thread)
 {
   struct thread_info *tp;
   ptid_t current_ptid;
@@ -432,45 +431,87 @@ info_threads_command (char *arg, int from_tty)
   struct cleanup *old_chain;
   struct frame_id saved_frame_id;
   char *extra_info;
+  int current_thread = -1;
 
   /* Backup current thread and selected frame.  */
   saved_frame_id = get_frame_id (get_selected_frame (NULL));
   old_chain = make_cleanup_restore_current_thread (inferior_ptid, saved_frame_id);
 
+  old_chain = make_cleanup_ui_out_list_begin_end (uiout, "threads");
+
   prune_threads ();
   target_find_new_threads ();
   current_ptid = inferior_ptid;
   for (tp = thread_list; tp; tp = tp->next)
     {
+      struct cleanup *chain2;
+
+      if (requested_thread != -1 && tp->num != requested_thread)
+	continue;
+
+      chain2 = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
+
       if (ptid_equal (tp->ptid, current_ptid))
-	printf_filtered ("* ");
+	{
+	  current_thread = tp->num;
+	  ui_out_text (uiout, "* ");
+	}
       else
-	printf_filtered ("  ");
+	ui_out_text (uiout, "  ");
 
-      printf_filtered ("%d %s", tp->num, target_tid_to_str (tp->ptid));
+      ui_out_field_int (uiout, "id", tp->num);
+      ui_out_text (uiout, " ");
+      ui_out_field_string (uiout, "target-id", target_tid_to_str (tp->ptid));
 
       extra_info = target_extra_thread_info (tp);
       if (extra_info)
-	printf_filtered (" (%s)", extra_info);
-      puts_filtered ("  ");
+	ui_out_field_fmt (uiout, "details", " (%s)", extra_info);
+      ui_out_text (uiout, "  ");
       /* That switch put us at the top of the stack (leaf frame).  */
       switch_to_thread (tp->ptid);
       print_stack_frame (get_selected_frame (NULL), 0, LOCATION);
+
+      do_cleanups (chain2);
     }
 
   /* Restores the current thread and the frame selected before
      the "info threads" command.  */
   do_cleanups (old_chain);
 
+  if (requested_thread == -1)
+    {
+      gdb_assert (current_thread != -1);
+      if (ui_out_is_mi_like_p (uiout))
+	ui_out_field_int (uiout, "current-thread-id", current_thread);
+    }
+
   /*  If case we were not able to find the original frame, print the
       new selected frame.  */
   if (frame_find_by_id (saved_frame_id) == NULL)
     {
       warning (_("Couldn't restore frame in current thread, at frame 0"));
-      print_stack_frame (get_selected_frame (NULL), 0, LOCATION);
+      /* For MI, we should probably have a notification about
+	 current frame change.  But this error is not very likely, so
+	 don't bother for now.  */
+      if (!ui_out_is_mi_like_p (uiout))
+	print_stack_frame (get_selected_frame (NULL), 0, LOCATION);
     }
 }
 
+
+/* Print information about currently known threads 
+
+ * Note: this has the drawback that it _really_ switches
+ *       threads, which frees the frame cache.  A no-side
+ *       effects info-threads command would be nicer.
+ */
+
+static void
+info_threads_command (char *arg, int from_tty)
+{
+  mi_info_threads (uiout, -1);
+}
+
 /* Switch from one thread to another. */
 
 void
-- 
1.5.3.5

Re: [RFA] Implement -thread-info.

[Eli Zaretskii]

> From: Vladimir Prus <vladimir@codesourcery.com>
> Date: Wed, 20 Feb 2008 14:27:35 +0300
> Cc: gdb-patches@sources.redhat.com,
>  Eli Zaretskii <eliz@gnu.org>
> 
> I attach the revised version of the patch, including docs. Eli, is the
> doc part OK?

Yes, thanks.

Re: [RFA] Implement -thread-info.

[Nick Roberts]

 > > Also do you want to 
 > > catch errors in print_stack_frame?
 > 
 > Yes, why not? A typical error is when we have a char* parameter pointing to
 > something not very accessible, in which case I'd prefer print_stack_frame
 > to catch this error, as opposed to -thread-info outright dying. 

If there is an error then presumably you want MI to catch (and report) it.
I don't know if this is a problem, in practice, because I don't know how to
get print_stack_frame to fail.

 > >  > I don't know what problem of return type you refer to -- can you
 > >  > clarify?
 > > 
 > > That, in Denis' patch, gdb_thread_info returns type enum gdb_rc but
 > > catch_exceptions_with_msg returns type int.  (just like gdb_breakpoint and
 > > break_command_really did for a while).
 > 
 > This problem does not exist in my patch, as catch_exceptions_with_msg is
 > just not used.

You misunderstand.  The problem you were asking about was in Denis' patch.
I never said it was in yours.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFA] Implement -thread-info.

[Vladimir Prus]

On Wednesday 20 February 2008 22:37:25 Nick Roberts wrote:
>  > > Also do you want to 
>  > > catch errors in print_stack_frame?
>  > 
>  > Yes, why not? A typical error is when we have a char* parameter pointing to
>  > something not very accessible, in which case I'd prefer print_stack_frame
>  > to catch this error, as opposed to -thread-info outright dying. 
> 
> If there is an error then presumably you want MI to catch (and report) it.
> I don't know if this is a problem, in practice, because I don't know how to
> get print_stack_frame to fail.

Well, the only error I know is failure to access memory when printing
arguments, which I probably don't want to convert into ^error of the entire
command. Should we run into really fatal errors that should terminate
-thread-info, we can always change.

> 
>  > >  > I don't know what problem of return type you refer to -- can you
>  > >  > clarify?
>  > > 
>  > > That, in Denis' patch, gdb_thread_info returns type enum gdb_rc but
>  > > catch_exceptions_with_msg returns type int.  (just like gdb_breakpoint and
>  > > break_command_really did for a while).
>  > 
>  > This problem does not exist in my patch, as catch_exceptions_with_msg is
>  > just not used.
> 
> You misunderstand.  The problem you were asking about was in Denis' patch.
> I never said it was in yours.

OK.

- Volodya

Re: [RFA] Implement -thread-info.

[Nick Roberts]

 > --- a/gdb/mi/mi-cmds.c
 > +++ b/gdb/mi/mi-cmds.c
 > @@ -130,8 +130,7 @@ struct mi_cmd mi_cmds[] =
 >    { "target-list-current-targets", { NULL, 0 }, NULL, NULL },
 >    { "target-list-parameters", { NULL, 0 }, NULL, NULL },
 >    { "target-select", { NULL, 0 }, mi_cmd_target_select},
 > -  { "thread-info", { NULL, 0 }, NULL, NULL },
 > -  { "thread-list-all-threads", { NULL, 0 }, NULL, NULL },
 > +  { "thread-info", { NULL, 0 }, NULL, mi_cmd_thread_info },
 >    { "thread-list-ids", { NULL, 0 }, 0, mi_cmd_thread_list_ids},
 >    { "thread-select", { NULL, 0 }, 0, mi_cmd_thread_select},
 >    { "trace-actions", { NULL, 0 }, NULL, NULL },

I was thinking:

*** mi-cmds.c.~1.30.~	2008-01-04 10:24:35.000000000 +1300
--- mi-cmds.c	2008-02-26 13:47:53.000000000 +1300
*************** struct mi_cmd mi_cmds[] =
*** 133,138 ****
--- 133,139 ----
    { "thread-info", { NULL, 0 }, NULL, NULL },
    { "thread-list-all-threads", { NULL, 0 }, NULL, NULL },
    { "thread-list-ids", { NULL, 0 }, 0, mi_cmd_thread_list_ids},
+   { "thread-info", { "info threads", 0 }, NULL, NULL },
    { "thread-select", { NULL, 0 }, 0, mi_cmd_thread_select},
    { "trace-actions", { NULL, 0 }, NULL, NULL },
    { "trace-delete", { NULL, 0 }, NULL, NULL },

like for "info break".

I realise that -thread-info can give info about one thread but presumably
the change could allow "info threads" to do that too (just as is already
done for "info break").  Then you just need

+   { "thread-info", { "info threads", 1 }, NULL, NULL },

WDYT?

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFA] Implement -thread-info.

[Daniel Jacobowitz]

On Tue, Feb 26, 2008 at 01:53:16PM +1300, Nick Roberts wrote:
> +   { "thread-info", { "info threads", 1 }, NULL, NULL },

I would suggest against using args_p == 1 for any new commands.  The
quoting rules between CLI and MI are different; we ignore that for
some commands, but it's a bug and it bites frontend developers.

If the arguments are just going to be integers then it doesn't make
much difference but it will make things simpler if we want to add
either quoted arguments or dashed options later.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Implement -thread-info.

[Vladimir Prus]

Nick Roberts wrote:

>  > --- a/gdb/mi/mi-cmds.c
>  > +++ b/gdb/mi/mi-cmds.c
>  > @@ -130,8 +130,7 @@ struct mi_cmd mi_cmds[] =
>  >    { "target-list-current-targets", { NULL, 0 }, NULL, NULL },
>  >    { "target-list-parameters", { NULL, 0 }, NULL, NULL },
>  >    { "target-select", { NULL, 0 }, mi_cmd_target_select},
>  > -  { "thread-info", { NULL, 0 }, NULL, NULL },
>  > -  { "thread-list-all-threads", { NULL, 0 }, NULL, NULL },
>  > +  { "thread-info", { NULL, 0 }, NULL, mi_cmd_thread_info },
>  >    { "thread-list-ids", { NULL, 0 }, 0, mi_cmd_thread_list_ids},
>  >    { "thread-select", { NULL, 0 }, 0, mi_cmd_thread_select},
>  >    { "trace-actions", { NULL, 0 }, NULL, NULL },
> 
> I was thinking:
> 
> *** mi-cmds.c.~1.30.~ 2008-01-04 10:24:35.000000000 +1300
> --- mi-cmds.c 2008-02-26 13:47:53.000000000 +1300
> *************** struct mi_cmd mi_cmds[] =
> *** 133,138 ****
> --- 133,139 ----
>     { "thread-info", { NULL, 0 }, NULL, NULL },
>     { "thread-list-all-threads", { NULL, 0 }, NULL, NULL },
>     { "thread-list-ids", { NULL, 0 }, 0, mi_cmd_thread_list_ids},
> +   { "thread-info", { "info threads", 0 }, NULL, NULL },
>     { "thread-select", { NULL, 0 }, 0, mi_cmd_thread_select},
>     { "trace-actions", { NULL, 0 }, NULL, NULL },
>     { "trace-delete", { NULL, 0 }, NULL, NULL },
> 
> like for "info break".
> 
> I realise that -thread-info can give info about one thread but presumably
> the change could allow "info threads" to do that too (just as is already
> done for "info break").  Then you just need
> 
> +   { "thread-info", { "info threads", 1 }, NULL, NULL },
> 
> WDYT?

I think I agree with Dan's concerns about using a backward-compatibility
mechanisms.

- Volodya

Re: [RFA] Implement -thread-info.

[Vladimir Prus]

Vladimir Prus wrote:

> I attach the revised version of the patch, including docs. Eli, is the
> doc part OK? I intend to commit the code part in a week unless there are
> objections.

I have realized that while the patch is mostly about MI, it also
touches generic code -- thread.c. Is that part of patch (attached again
for convenience) OK?

- Volodya

        * gdb.h (mi_info_threads): Declare.

        * thread.c (mi_info_threads): New, extracted
        from info_threads_command and adjusted to
        work for CLI and MI.
        (info_threads_command): Use mi_info_threads.

        * mi/mi-cmds.c (mi_cmds): Specify a handler
        for -thread-info.
        * mi/mi-cmds.h (mi_cmd_thread_info): Declare.
        * mi/mi-main.c (mi_cmd_thread_info): New.
        (mi_cmd_list_features): Include 'thread-info'.

        doc/
        * gdb.texinfo (Thread Commands): Document
        -thread-info.  Remove -thread-list-all-threads.
---
 gdb/doc/gdb.texinfo |   35 ++++++++++++-------------
 gdb/gdb.h           |    2 +
 gdb/mi/mi-cmds.c    |    3 +-
 gdb/mi/mi-cmds.h    |    1 +
 gdb/mi/mi-main.c    |   19 +++++++++++++
 gdb/thread.c        |   71 ++++++++++++++++++++++++++++++++++++++++-----------
 6 files changed, 96 insertions(+), 35 deletions(-)

diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index 722db28..f25363a 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -18851,34 +18851,33 @@ The corresponding @value{GDBN} command is @samp{pwd}.
 @subsubheading Synopsis
 
 @smallexample
- -thread-info
+ -thread-info [ @var{thread-id} ]
 @end smallexample
 
+Reports the information about either a specific thread, if 
+the @var{thread-id} parameter is present, or about all
+threads.  When printing information about all threads,
+also reports the current thread.
+
 @subsubheading @value{GDBN} Command
 
-No equivalent.
+The @samp{info thread} command prints the same information
+about all threads.
 
 @subsubheading Example
-N.A.
-
-
-@subheading The @code{-thread-list-all-threads} Command
-@findex -thread-list-all-threads
-
-@subsubheading Synopsis
 
 @smallexample
- -thread-list-all-threads
+-thread-info
+^done,threads=[
+@{id="2",target-id="Thread 0xb7e14b90 (LWP 21257)",
+   frame=@{addr="0xffffe410",func="__kernel_vsyscall",args=[]@},
+@{id="1",target-id="Thread 0xb7e156b0 (LWP 21254)",
+   frame=@{addr="0x0804891f",func="foo",args=[@{name="i",value="10"@}],
+           file="/tmp/a.c",fullname="/tmp/a.c",line="158"@}@}],
+current-thread-id="1"
+(gdb)
 @end smallexample
 
-@subsubheading @value{GDBN} Command
-
-The equivalent @value{GDBN} command is @samp{info threads}.
-
-@subsubheading Example
-N.A.
-
-
 @subheading The @code{-thread-list-ids} Command
 @findex -thread-list-ids
 
diff --git a/gdb/gdb.h b/gdb/gdb.h
index fcd3e3b..13523fb 100644
--- a/gdb/gdb.h
+++ b/gdb/gdb.h
@@ -55,4 +55,6 @@ enum gdb_rc gdb_thread_select (struct ui_out *uiout, char *tidstr,
 enum gdb_rc gdb_list_thread_ids (struct ui_out *uiout,
                                 char **error_message);
 
+extern void mi_info_threads (struct ui_out *uiout, int thread);
+
 #endif
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index c651694..89c6376 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -130,8 +130,7 @@ struct mi_cmd mi_cmds[] =
   { "target-list-current-targets", { NULL, 0 }, NULL, NULL },
   { "target-list-parameters", { NULL, 0 }, NULL, NULL },
   { "target-select", { NULL, 0 }, mi_cmd_target_select},
-  { "thread-info", { NULL, 0 }, NULL, NULL },
-  { "thread-list-all-threads", { NULL, 0 }, NULL, NULL },
+  { "thread-info", { NULL, 0 }, NULL, mi_cmd_thread_info },
   { "thread-list-ids", { NULL, 0 }, 0, mi_cmd_thread_list_ids},
   { "thread-select", { NULL, 0 }, 0, mi_cmd_thread_select},
   { "trace-actions", { NULL, 0 }, NULL, NULL },
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index 2d0393b..6a033e5 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -105,6 +105,7 @@ extern mi_cmd_argv_ftype mi_cmd_target_file_get;
 extern mi_cmd_argv_ftype mi_cmd_target_file_put;
 extern mi_cmd_argv_ftype mi_cmd_target_file_delete;
 extern mi_cmd_args_ftype mi_cmd_target_select;
+extern mi_cmd_argv_ftype mi_cmd_thread_info;
 extern mi_cmd_argv_ftype mi_cmd_thread_list_ids;
 extern mi_cmd_argv_ftype mi_cmd_thread_select;
 extern mi_cmd_argv_ftype mi_cmd_var_assign;
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 41e12d2..5d89847 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -277,6 +277,24 @@ mi_cmd_thread_list_ids (char *command, char **argv, int argc)
 }
 
 enum mi_cmd_result
+mi_cmd_thread_info (char *command, char **argv, int argc)
+{
+  int thread = -1;
+  
+  if (argc != 0 && argc != 1)
+    {
+      mi_error_message = xstrprintf ("Invalid MI command");
+      return MI_CMD_ERROR;
+    }
+
+  if (argc == 1)
+    thread = atoi (argv[0]);
+
+  mi_info_threads (uiout, thread);
+  return MI_CMD_DONE;
+}
+
+enum mi_cmd_result
 mi_cmd_data_list_register_names (char *command, char **argv, int argc)
 {
   int regnum, numregs;
@@ -1055,6 +1073,7 @@ mi_cmd_list_features (char *command, char **argv, int argc)
 
       ui_out_field_string (uiout, NULL, "frozen-varobjs");
       ui_out_field_string (uiout, NULL, "pending-breakpoints");
+      ui_out_field_string (uiout, NULL, "thread-info");
       
       do_cleanups (cleanup);
 
diff --git a/gdb/thread.c b/gdb/thread.c
index 3a39703..1f4b099 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -416,15 +416,14 @@ prune_threads (void)
     }
 }
 
-/* Print information about currently known threads 
-
- * Note: this has the drawback that it _really_ switches
- *       threads, which frees the frame cache.  A no-side
- *       effects info-threads command would be nicer.
- */
-
-static void
-info_threads_command (char *arg, int from_tty)
+/* Prints the list of threads and their details on UIOUT.
+   This is a version of 'info_thread_command' suitable for
+   use from MI.  
+   If REQESTED_THREAD is not -1, it's the GDB id of the thread
+   that should be printed.  Otherwise, all threads are
+   printed.  */
+void
+mi_info_threads (struct ui_out *uiout, int requested_thread)
 {
   struct thread_info *tp;
   ptid_t current_ptid;
@@ -432,45 +431,87 @@ info_threads_command (char *arg, int from_tty)
   struct cleanup *old_chain;
   struct frame_id saved_frame_id;
   char *extra_info;
+  int current_thread = -1;
 
   /* Backup current thread and selected frame.  */
   saved_frame_id = get_frame_id (get_selected_frame (NULL));
   old_chain = make_cleanup_restore_current_thread (inferior_ptid, saved_frame_id);
 
+  old_chain = make_cleanup_ui_out_list_begin_end (uiout, "threads");
+
   prune_threads ();
   target_find_new_threads ();
   current_ptid = inferior_ptid;
   for (tp = thread_list; tp; tp = tp->next)
     {
+      struct cleanup *chain2;
+
+      if (requested_thread != -1 && tp->num != requested_thread)
+       continue;
+
+      chain2 = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
+
       if (ptid_equal (tp->ptid, current_ptid))
-       printf_filtered ("* ");
+       {
+         current_thread = tp->num;
+         ui_out_text (uiout, "* ");
+       }
       else
-       printf_filtered ("  ");
+       ui_out_text (uiout, "  ");
 
-      printf_filtered ("%d %s", tp->num, target_tid_to_str (tp->ptid));
+      ui_out_field_int (uiout, "id", tp->num);
+      ui_out_text (uiout, " ");
+      ui_out_field_string (uiout, "target-id", target_tid_to_str (tp->ptid));
 
       extra_info = target_extra_thread_info (tp);
       if (extra_info)
-       printf_filtered (" (%s)", extra_info);
-      puts_filtered ("  ");
+       ui_out_field_fmt (uiout, "details", " (%s)", extra_info);
+      ui_out_text (uiout, "  ");
       /* That switch put us at the top of the stack (leaf frame).  */
       switch_to_thread (tp->ptid);
       print_stack_frame (get_selected_frame (NULL), 0, LOCATION);
+
+      do_cleanups (chain2);
     }
 
   /* Restores the current thread and the frame selected before
      the "info threads" command.  */
   do_cleanups (old_chain);
 
+  if (requested_thread == -1)
+    {
+      gdb_assert (current_thread != -1);
+      if (ui_out_is_mi_like_p (uiout))
+       ui_out_field_int (uiout, "current-thread-id", current_thread);
+    }
+
   /*  If case we were not able to find the original frame, print the
       new selected frame.  */
   if (frame_find_by_id (saved_frame_id) == NULL)
     {
       warning (_("Couldn't restore frame in current thread, at frame 0"));
-      print_stack_frame (get_selected_frame (NULL), 0, LOCATION);
+      /* For MI, we should probably have a notification about
+        current frame change.  But this error is not very likely, so
+        don't bother for now.  */
+      if (!ui_out_is_mi_like_p (uiout))
+       print_stack_frame (get_selected_frame (NULL), 0, LOCATION);
     }
 }
 
+
+/* Print information about currently known threads 
+
+ * Note: this has the drawback that it _really_ switches
+ *       threads, which frees the frame cache.  A no-side
+ *       effects info-threads command would be nicer.
+ */
+
+static void
+info_threads_command (char *arg, int from_tty)
+{
+  mi_info_threads (uiout, -1);
+}
+
 /* Switch from one thread to another. */
 
 void
-- 
1.5.3.5

Re: [RFA] Implement -thread-info.

[Nick Roberts]

 > -static void
 > -info_threads_command (char *arg, int from_tty)
 > +/* Prints the list of threads and their details on UIOUT.

 > +   This is a version of 'info_thread_command' suitable for
 > +   use from MI.  

Isn't this comment out of date now that info_threads_command calls
mi_info_threads?

 > +   If REQESTED_THREAD is not -1, it's the GDB id of the thread
 > +   that should be printed.  Otherwise, all threads are
 > +   printed.  */
 > +void
 > +mi_info_threads (struct ui_out *uiout, int requested_thread)
 >  {

As mi_info_threads is now not mi specific, should it be called something like
info_threads_1?

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFA] Implement -thread-info.

[Vladimir Prus]

On Saturday 08 March 2008 23:03:38 Nick Roberts wrote:
> 
>  > -static void
>  > -info_threads_command (char *arg, int from_tty)
>  > +/* Prints the list of threads and their details on UIOUT.
> 
>  > +   This is a version of 'info_thread_command' suitable for
>  > +   use from MI.  
> 
> Isn't this comment out of date now that info_threads_command calls
> mi_info_threads?

It is out of date, indeed.

> 
>  > +   If REQESTED_THREAD is not -1, it's the GDB id of the thread
>  > +   that should be printed.  Otherwise, all threads are
>  > +   printed.  */
>  > +void
>  > +mi_info_threads (struct ui_out *uiout, int requested_thread)
>  >  {
> 
> As mi_info_threads is now not mi specific, should it be called something like
> info_threads_1?

The "_1" is a bit misleading, as the function can print information about all
threads just fine. How about "print_thread_info"?

- Volodya

Re: [RFA] Implement -thread-info.

[Nick Roberts]

 > > As mi_info_threads is now not mi specific, should it be called something
 > > like info_threads_1?
 > 
 > The "_1" is a bit misleading, as the function can print information about all
 > threads just fine. How about "print_thread_info"?

Sure.  You don't really need to ask now.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFA] Implement -thread-info.

[Vladimir Prus]

Vladimir Prus wrote:

> Vladimir Prus wrote:
> 
>> I attach the revised version of the patch, including docs. Eli, is the
>> doc part OK? I intend to commit the code part in a week unless there are
>> objections.
> 
> I have realized that while the patch is mostly about MI, it also
> touches generic code -- thread.c. Is that part of patch (attached again
> for convenience) OK?

Ping?

- Volodya

Re: [RFA] Implement -thread-info.

[Daniel Jacobowitz]

On Sat, Mar 08, 2008 at 06:54:45PM +0300, Vladimir Prus wrote:
> Vladimir Prus wrote:
> 
> > I attach the revised version of the patch, including docs. Eli, is the
> > doc part OK? I intend to commit the code part in a week unless there are
> > objections.
> 
> I have realized that while the patch is mostly about MI, it also
> touches generic code -- thread.c. Is that part of patch (attached again
> for convenience) OK?
> 
> - Volodya
> 
>         * gdb.h (mi_info_threads): Declare.

I think gdbthread.h is the appropriate header for this; gdb.h is for
things using GDB_RC_OK and so forth.

>  @smallexample
> - -thread-info
> + -thread-info [ @var{thread-id} ]
>  @end smallexample
>  
> +Reports the information about either a specific thread, if 

Just "reports information", no "the".

> @@ -1055,6 +1073,7 @@ mi_cmd_list_features (char *command, char **argv, int argc)
>  
>        ui_out_field_string (uiout, NULL, "frozen-varobjs");
>        ui_out_field_string (uiout, NULL, "pending-breakpoints");
> +      ui_out_field_string (uiout, NULL, "thread-info");
>        
>        do_cleanups (cleanup);
>  

Shouldn't this be documented too?

> +void
> +mi_info_threads (struct ui_out *uiout, int requested_thread)

Nick's right about the name of this function.  print_thread_info would
be fine.  info_threads_1 would be fine too - the _1 is a weird
convention that just means "the real body of info_threads", not
"info_threads for one thread".  print_thread_info sounds better to me.

>    /* Backup current thread and selected frame.  */
>    saved_frame_id = get_frame_id (get_selected_frame (NULL));
>    old_chain = make_cleanup_restore_current_thread (inferior_ptid, saved_frame_id);
>  
> +  old_chain = make_cleanup_ui_out_list_begin_end (uiout, "threads");
> +

By overwriting old_chain, you've stopped do_cleanups from restoring
the previously selected thread.

>        extra_info = target_extra_thread_info (tp);
>        if (extra_info)
> -       printf_filtered (" (%s)", extra_info);
> -      puts_filtered ("  ");
> +       ui_out_field_fmt (uiout, "details", " (%s)", extra_info);
> +      ui_out_text (uiout, "  ");

Do you really want to give the front end " (some extra text)"?  It'll
want to display this in a different column anyway.  It should probably
be

  ui_out_text (uiout, " (");
  ui_out_field_string (uiout, extra_info);
  ui_out_text (")  ");

>        /* That switch put us at the top of the stack (leaf frame).  */
>        switch_to_thread (tp->ptid);
>        print_stack_frame (get_selected_frame (NULL), 0, LOCATION);

I think Nick was right about printing the frame level.  It's not
necessarily going to be zero in the future.  For instance, GDB might
automatically select the nearest user frame if you stop when a C++
exception is thrown, like we already do for Ada; and then we could
teach GDB not to lose the selected frame whenever it switches threads.
That wants doing anyway.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Implement -thread-info.

[Daniel Jacobowitz]

On Fri, Mar 14, 2008 at 11:07:32AM -0400, Daniel Jacobowitz wrote:
> > I have realized that while the patch is mostly about MI, it also
> > touches generic code -- thread.c. Is that part of patch (attached again
> > for convenience) OK?

Oh, one other thing.  I went back to look at Denis's patches.  He made
-thread-info without an argument describe the current thread only.
Doesn't that seem useful, especially before we get notifications
completely hammered out?  I do not see any other MI command that
reports the current thread.

So that would mean we needed -thread-list-all-threads back again.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Implement -thread-info.

[Vladimir Prus]

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

On Friday 14 March 2008 18:11:15 Daniel Jacobowitz wrote:
> On Fri, Mar 14, 2008 at 11:07:32AM -0400, Daniel Jacobowitz wrote:

> >        /* That switch put us at the top of the stack (leaf frame).  */
> >        switch_to_thread (tp->ptid);
> >        print_stack_frame (get_selected_frame (NULL), 0, LOCATION);
>
> I think Nick was right about printing the frame level.  It's not
> necessarily going to be zero in the future.  For instance, GDB might
> automatically select the nearest user frame if you stop when a C++
> exception is thrown, like we already do for Ada; and then we could
> teach GDB not to lose the selected frame whenever it switches threads.
> That wants doing anyway.

Ok, I've enabled printing of stack level.

> > > I have realized that while the patch is mostly about MI, it also
> > > touches generic code -- thread.c. Is that part of patch (attached again
> > > for convenience) OK?
> 
> Oh, one other thing.  I went back to look at Denis's patches.  He made
> -thread-info without an argument describe the current thread only.
> Doesn't that seem useful, especially before we get notifications
> completely hammered out?  I do not see any other MI command that
> reports the current thread.
> 
> So that would mean we needed -thread-list-all-threads back again.

Hmm, I'd rather not. Currently, the current thread is reported by 
*stopped, and is not supposed to randomly change -- because the
target is fully stopped while frontend talks with gdb. Therefore,
using -thread-info as a roundabout way to get the current thread
does not really give you anything.

For non-stop mode, I have long and evil plans that I'll post soon,
but they don't require this either.

I attach the patch revised per your comments. How does it look?

- Volodya

[-- Attachment #2: 0001-Implement-thread-info.patch --]
[-- Type: text/x-diff, Size: 10430 bytes --]

From b8e58feeae705971a9f927a7f0df3220df6f5523 Mon Sep 17 00:00:00 2001
From: Vladimir Prus <vladimir@codesourcery.com>
Date: Sun, 17 Feb 2008 18:16:02 +0300
Subject: [RFA] Implement -thread-info.
To: gdb-patches@sources.redhat.com
X-KMail-Transport: CodeSourcery
X-KMail-Identity: 901867920

	* gdbthread.h (print_thread_info): Declare.

	* thread.c (print_thread_info): New, extracted
	from info_threads_command and adjusted to
	work for CLI and MI.
	(info_threads_command): Use print_thread_info.
        * Makefile.in: Update dependencies.

	* mi/mi-cmds.c (mi_cmds): Specify a handler
	for -thread-info.
	* mi/mi-cmds.h (mi_cmd_thread_info): Declare.
	* mi/mi-main.c (mi_cmd_thread_info): New.
	(mi_cmd_list_features): Include 'thread-info'.

	doc/
	* gdb.texinfo (Thread Commands): Document
	-thread-info.  Remove -thread-list-all-threads.
---
 gdb/Makefile.in     |    2 +-
 gdb/doc/gdb.texinfo |   37 ++++++++++++------------
 gdb/gdbthread.h     |    4 ++
 gdb/mi/mi-cmds.c    |    3 +-
 gdb/mi/mi-cmds.h    |    1 +
 gdb/mi/mi-main.c    |   19 ++++++++++++
 gdb/thread.c        |   77 ++++++++++++++++++++++++++++++++++++++++----------
 7 files changed, 106 insertions(+), 37 deletions(-)

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 2ba51b1..75f4991 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -793,7 +793,7 @@ gdb_stabs_h = gdb-stabs.h
 gdb_stat_h = gdb_stat.h
 gdb_string_h = gdb_string.h
 gdb_thread_db_h = gdb_thread_db.h
-gdbthread_h = gdbthread.h $(breakpoint_h) $(frame_h)
+gdbthread_h = gdbthread.h $(breakpoint_h) $(frame_h) $(ui_out_h)
 gdbtypes_h = gdbtypes.h $(hashtab_h)
 gdb_vfork_h = gdb_vfork.h
 gdb_wait_h = gdb_wait.h
diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
index dbc9efc..98cf7a5 100644
--- a/gdb/doc/gdb.texinfo
+++ b/gdb/doc/gdb.texinfo
@@ -18924,34 +18924,33 @@ The corresponding @value{GDBN} command is @samp{pwd}.
 @subsubheading Synopsis
 
 @smallexample
- -thread-info
+ -thread-info [ @var{thread-id} ]
 @end smallexample
 
+Reports information about either a specific thread, if 
+the @var{thread-id} parameter is present, or about all
+threads.  When printing information about all threads,
+also reports the current thread.
+
 @subsubheading @value{GDBN} Command
 
-No equivalent.
+The @samp{info thread} command prints the same information
+about all threads.
 
 @subsubheading Example
-N.A.
-
-
-@subheading The @code{-thread-list-all-threads} Command
-@findex -thread-list-all-threads
-
-@subsubheading Synopsis
 
 @smallexample
- -thread-list-all-threads
+-thread-info
+^done,threads=[
+@{id="2",target-id="Thread 0xb7e14b90 (LWP 21257)",
+   frame=@{addr="0xffffe410",func="__kernel_vsyscall",args=[]@},
+@{id="1",target-id="Thread 0xb7e156b0 (LWP 21254)",
+   frame=@{addr="0x0804891f",func="foo",args=[@{name="i",value="10"@}],
+           file="/tmp/a.c",fullname="/tmp/a.c",line="158"@}@}],
+current-thread-id="1"
+(gdb)
 @end smallexample
 
-@subsubheading @value{GDBN} Command
-
-The equivalent @value{GDBN} command is @samp{info threads}.
-
-@subsubheading Example
-N.A.
-
-
 @subheading The @code{-thread-list-ids} Command
 @findex -thread-list-ids
 
@@ -21845,6 +21844,8 @@ The current list of features is:
 @item
 @samp{pending-breakpoints}---indicates presence of the @code{-f}
 option to the @code{-break-insert} command.
+@item
+@samp{thread-info}---indicates presence of the @code{-thread-info} command.
 
 @end itemize
 
diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h
index a1e4efe..5ca329e 100644
--- a/gdb/gdbthread.h
+++ b/gdb/gdbthread.h
@@ -32,6 +32,8 @@ struct symtab;
 /* For struct frame_id.  */
 #include "frame.h"
 
+#include "ui-out.h"
+
 struct thread_info
 {
   struct thread_info *next;
@@ -152,4 +154,6 @@ 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);
+
 #endif /* GDBTHREAD_H */
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index c651694..89c6376 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -130,8 +130,7 @@ struct mi_cmd mi_cmds[] =
   { "target-list-current-targets", { NULL, 0 }, NULL, NULL },
   { "target-list-parameters", { NULL, 0 }, NULL, NULL },
   { "target-select", { NULL, 0 }, mi_cmd_target_select},
-  { "thread-info", { NULL, 0 }, NULL, NULL },
-  { "thread-list-all-threads", { NULL, 0 }, NULL, NULL },
+  { "thread-info", { NULL, 0 }, NULL, mi_cmd_thread_info },
   { "thread-list-ids", { NULL, 0 }, 0, mi_cmd_thread_list_ids},
   { "thread-select", { NULL, 0 }, 0, mi_cmd_thread_select},
   { "trace-actions", { NULL, 0 }, NULL, NULL },
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index 2d0393b..6a033e5 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -105,6 +105,7 @@ extern mi_cmd_argv_ftype mi_cmd_target_file_get;
 extern mi_cmd_argv_ftype mi_cmd_target_file_put;
 extern mi_cmd_argv_ftype mi_cmd_target_file_delete;
 extern mi_cmd_args_ftype mi_cmd_target_select;
+extern mi_cmd_argv_ftype mi_cmd_thread_info;
 extern mi_cmd_argv_ftype mi_cmd_thread_list_ids;
 extern mi_cmd_argv_ftype mi_cmd_thread_select;
 extern mi_cmd_argv_ftype mi_cmd_var_assign;
diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
index 41e12d2..cea8503 100644
--- a/gdb/mi/mi-main.c
+++ b/gdb/mi/mi-main.c
@@ -277,6 +277,24 @@ mi_cmd_thread_list_ids (char *command, char **argv, int argc)
 }
 
 enum mi_cmd_result
+mi_cmd_thread_info (char *command, char **argv, int argc)
+{
+  int thread = -1;
+  
+  if (argc != 0 && argc != 1)
+    {
+      mi_error_message = xstrprintf ("Invalid MI command");
+      return MI_CMD_ERROR;
+    }
+
+  if (argc == 1)
+    thread = atoi (argv[0]);
+
+  print_thread_info (uiout, thread);
+  return MI_CMD_DONE;
+}
+
+enum mi_cmd_result
 mi_cmd_data_list_register_names (char *command, char **argv, int argc)
 {
   int regnum, numregs;
@@ -1055,6 +1073,7 @@ mi_cmd_list_features (char *command, char **argv, int argc)
 
       ui_out_field_string (uiout, NULL, "frozen-varobjs");
       ui_out_field_string (uiout, NULL, "pending-breakpoints");
+      ui_out_field_string (uiout, NULL, "thread-info");
       
       do_cleanups (cleanup);
 
diff --git a/gdb/thread.c b/gdb/thread.c
index 4aae4b6..e1ff672 100644
--- a/gdb/thread.c
+++ b/gdb/thread.c
@@ -416,15 +416,14 @@ prune_threads (void)
     }
 }
 
-/* Print information about currently known threads 
-
- * Note: this has the drawback that it _really_ switches
- *       threads, which frees the frame cache.  A no-side
- *       effects info-threads command would be nicer.
- */
-
-static void
-info_threads_command (char *arg, int from_tty)
+/* Prints the list of threads and their details on UIOUT.
+   This is a version of 'info_thread_command' suitable for
+   use from MI.  
+   If REQESTED_THREAD is not -1, it's the GDB id of the thread
+   that should be printed.  Otherwise, all threads are
+   printed.  */
+void
+print_thread_info (struct ui_out *uiout, int requested_thread)
 {
   struct thread_info *tp;
   ptid_t current_ptid;
@@ -432,45 +431,91 @@ info_threads_command (char *arg, int from_tty)
   struct cleanup *old_chain;
   struct frame_id saved_frame_id;
   char *extra_info;
+  int current_thread = -1;
 
   /* Backup current thread and selected frame.  */
   saved_frame_id = get_frame_id (get_selected_frame (NULL));
   old_chain = make_cleanup_restore_current_thread (inferior_ptid, saved_frame_id);
 
+  make_cleanup_ui_out_list_begin_end (uiout, "threads");
+
   prune_threads ();
   target_find_new_threads ();
   current_ptid = inferior_ptid;
   for (tp = thread_list; tp; tp = tp->next)
     {
+      struct cleanup *chain2;
+
+      if (requested_thread != -1 && tp->num != requested_thread)
+	continue;
+
+      chain2 = make_cleanup_ui_out_tuple_begin_end (uiout, NULL);
+
       if (ptid_equal (tp->ptid, current_ptid))
-	printf_filtered ("* ");
+	{
+	  current_thread = tp->num;
+	  ui_out_text (uiout, "* ");
+	}
       else
-	printf_filtered ("  ");
+	ui_out_text (uiout, "  ");
 
-      printf_filtered ("%d %s", tp->num, target_tid_to_str (tp->ptid));
+      ui_out_field_int (uiout, "id", tp->num);
+      ui_out_text (uiout, " ");
+      ui_out_field_string (uiout, "target-id", target_tid_to_str (tp->ptid));
 
       extra_info = target_extra_thread_info (tp);
       if (extra_info)
-	printf_filtered (" (%s)", extra_info);
-      puts_filtered ("  ");
+	{
+	  ui_out_text (uiout, " (");
+	  ui_out_field_string (uiout, "details", extra_info);
+	  ui_out_text (uiout, ")");
+	}
+      ui_out_text (uiout, "  ");
       /* That switch put us at the top of the stack (leaf frame).  */
       switch_to_thread (tp->ptid);
-      print_stack_frame (get_selected_frame (NULL), 0, LOCATION);
+      print_stack_frame (get_selected_frame (NULL), 1, LOCATION);
+
+      do_cleanups (chain2);
     }
 
   /* Restores the current thread and the frame selected before
      the "info threads" command.  */
   do_cleanups (old_chain);
 
+  if (requested_thread == -1)
+    {
+      gdb_assert (current_thread != -1);
+      if (ui_out_is_mi_like_p (uiout))
+	ui_out_field_int (uiout, "current-thread-id", current_thread);
+    }
+
   /*  If case we were not able to find the original frame, print the
       new selected frame.  */
   if (frame_find_by_id (saved_frame_id) == NULL)
     {
       warning (_("Couldn't restore frame in current thread, at frame 0"));
-      print_stack_frame (get_selected_frame (NULL), 0, LOCATION);
+      /* For MI, we should probably have a notification about
+	 current frame change.  But this error is not very likely, so
+	 don't bother for now.  */
+      if (!ui_out_is_mi_like_p (uiout))
+	print_stack_frame (get_selected_frame (NULL), 0, LOCATION);
     }
 }
 
+
+/* Print information about currently known threads 
+
+ * Note: this has the drawback that it _really_ switches
+ *       threads, which frees the frame cache.  A no-side
+ *       effects info-threads command would be nicer.
+ */
+
+static void
+info_threads_command (char *arg, int from_tty)
+{
+  print_thread_info (uiout, -1);
+}
+
 /* Switch from one thread to another. */
 
 void
-- 
1.5.3.5

Re: [RFA] Implement -thread-info.

[Daniel Jacobowitz]

On Fri, Mar 14, 2008 at 08:03:47PM +0300, Vladimir Prus wrote:
> > Oh, one other thing.  I went back to look at Denis's patches.  He made
> > -thread-info without an argument describe the current thread only.
> > Doesn't that seem useful, especially before we get notifications
> > completely hammered out?  I do not see any other MI command that
> > reports the current thread.
> > 
> > So that would mean we needed -thread-list-all-threads back again.
> 
> Hmm, I'd rather not. Currently, the current thread is reported by 
> *stopped, and is not supposed to randomly change -- because the
> target is fully stopped while frontend talks with gdb. Therefore,
> using -thread-info as a roundabout way to get the current thread
> does not really give you anything.
> 
> For non-stop mode, I have long and evil plans that I'll post soon,
> but they don't require this either.

I find it strange that -thread-info gives information about multiple
threads, myself.  But this isn't worth arguing about; we'll do it
your way.

> +@{id="1",target-id="Thread 0xb7e156b0 (LWP 21254)",
> +   frame=@{addr="0x0804891f",func="foo",args=[@{name="i",value="10"@}],
> +           file="/tmp/a.c",fullname="/tmp/a.c",line="158"@}@}],

If we're adding the level then please update the example.

>        /* That switch put us at the top of the stack (leaf frame).  */
>        switch_to_thread (tp->ptid);
> -      print_stack_frame (get_selected_frame (NULL), 0, LOCATION);
> +      print_stack_frame (get_selected_frame (NULL), 1, LOCATION);

mi_like_p?  This will print out the #0 in the CLI too.

Otherwise OK.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Implement -thread-info.

[Michael Snyder]

On Fri, 2008-03-14 at 13:16 +0300, Vladimir Prus wrote:
> Vladimir Prus wrote:
> 
> > Vladimir Prus wrote:
> > 
> >> I attach the revised version of the patch, including docs. Eli, is the
> >> doc part OK? I intend to commit the code part in a week unless there are
> >> objections.
> > 
> > I have realized that while the patch is mostly about MI, it also
> > touches generic code -- thread.c. Is that part of patch (attached again
> > for convenience) OK?
> 
> Ping?

I'm looking back at your 02/20/08 patch -- is that the correct one?

I noticed that your new version of info_threads_command always
passes -1 to mi_info_threads, but then I checked and saw that
the existing version of info_threads_command also ignores its
argument.  I think that's too bad, but at least it doesn't make
the status quo worse.

That was my only concern so far...

Re: [RFA] Implement -thread-info.

[Nick Roberts]

Daniel Jacobowitz writes:
 > On Tue, Feb 26, 2008 at 01:53:16PM +1300, Nick Roberts wrote:
 > > +   { "thread-info", { "info threads", 1 }, NULL, NULL },
 > 
 > I would suggest against using args_p == 1 for any new commands.  The
 > quoting rules between CLI and MI are different; we ignore that for
 > some commands, but it's a bug and it bites frontend developers.
 > 
 > If the arguments are just going to be integers then it doesn't make
 > much difference but it will make things simpler if we want to add
 > either quoted arguments or dashed options later.

With non-stop mode there will presumably be many new MI commands.  Providing
the same functionality in CLI duplicates a lot of effort.  Perhaps the parsing
of MI and CLI arguments should be unified.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFA] Implement -thread-info.

[Vladimir Prus]

On Sunday 23 March 2008 07:55:55 Nick Roberts wrote:
> Daniel Jacobowitz writes:
>  > On Tue, Feb 26, 2008 at 01:53:16PM +1300, Nick Roberts wrote:
>  > > +   { "thread-info", { "info threads", 1 }, NULL, NULL },
>  >
>  > I would suggest against using args_p == 1 for any new commands. 
>  > The quoting rules between CLI and MI are different; we ignore that
>  > for some commands, but it's a bug and it bites frontend
>  > developers.
>  >
>  > If the arguments are just going to be integers then it doesn't
>  > make much difference but it will make things simpler if we want to
>  > add either quoted arguments or dashed options later.
>
> With non-stop mode there will presumably be many new MI commands.

Just one, I think -- to enable non-stop mode. Many commands will take
additional options, though.

> Providing the same functionality in CLI duplicates a lot of effort. 
> Perhaps the parsing of MI and CLI arguments should be unified.

Are you suggesting that we continue to implement MI commands by
basically executing the corresponding CLI command? 

- Volodya

Re: [RFA] Implement -thread-info.

[Daniel Jacobowitz]

On Sun, Mar 23, 2008 at 04:55:55PM +1200, Nick Roberts wrote:
> With non-stop mode there will presumably be many new MI commands.  Providing
> the same functionality in CLI duplicates a lot of effort.  Perhaps the parsing
> of MI and CLI arguments should be unified.

I don't think there will be many new commands and the user model is
probably different - the logical way to expose things to a front end
and to an end user may not be the same.

In any case, an argv version of the CLI command interface is something
I've wanted for a long time, but it's a lot of work.

-- 
Daniel Jacobowitz
CodeSourcery