[RFC] -thread-info new command

[RFC] -thread-info new command

[Denis PILAT]

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

Following discussion on thread 
http://sources.redhat.com/ml/gdb-patches/2007-03/msg00138.html
I propose an implementation of *-thread-info* mi command.

I will propose new testsuite enhancements and document for this command 
*later* since I prefer that command to be approved (in its format) 
before going in doc and testsuite work that represents much more work 
for me.

This command takes an optional thread id in parameter, if omitted the 
current thread is taken.


Attached is the patch to be discussed.
-- 
Denis





[-- Attachment #2: thread-info.patch --]
[-- Type: text/plain, Size: 5211 bytes --]

2007-03-19  Denis Pilat  <denis.pilat@st.com>

	* gdb.h (gdb_thread_info): New function.
	* thread.c (gdb_thread_info, do_captured_thread_info): New functions.
	* mi/mi-cmds.c (mi_cmds): Add entry for new MI command -thread-info.
	* mi/mi-cmds.h (mi_cmd_thread_info): New extern.
 	* mi/mi-main.c (mi_cmd_thread_info): New function.

Index: gdb.h
===================================================================
RCS file: /cvs/src/src/gdb/gdb.h,v
retrieving revision 1.6
diff -u -p -r1.6 gdb.h
--- gdb.h	9 Jan 2007 17:58:50 -0000	1.6
+++ gdb.h	19 Mar 2007 14:23:24 -0000
@@ -63,4 +63,8 @@ enum gdb_rc gdb_thread_select (struct ui
 enum gdb_rc gdb_list_thread_ids (struct ui_out *uiout,
 				 char **error_message);
 
+/* Print information for current thread or thread which num is in tidstr.  */
+enum gdb_rc gdb_thread_info (struct ui_out *uiout, char *tidstr,
+			     char **error_message);
+
 #endif
Index: thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.51
diff -u -p -r1.51 thread.c
--- thread.c	28 Feb 2007 17:35:01 -0000	1.51
+++ thread.c	19 Mar 2007 14:23:28 -0000
@@ -737,3 +742,59 @@ The new thread ID must be currently know
   if (!xdb_commands)
     add_com_alias ("t", "thread", class_run, 1);
 }
+
+
+static int
+do_captured_thread_info (struct ui_out *uiout, void *tidstr)
+{
+  int num;
+  struct thread_info *tp;
+  ptid_t current_ptid;
+  char *extra_info;
+
+  /* backup current thread.   */
+  current_ptid = inferior_ptid;
+
+  /* if no argument we consider user needs info for current thread.  */
+  if (tidstr)
+    num = value_as_long (parse_and_eval (tidstr));
+  else
+    num =  pid_to_thread_id (inferior_ptid);
+  
+  tp = find_thread_id (num);
+
+  if (!tp)
+    error (_("Thread ID %d not known."), num);
+
+  if (!thread_alive (tp))
+    error (_("Thread ID %d has terminated."), num);
+
+  if (tidstr)
+    switch_to_thread (tp->ptid);
+
+  ui_out_field_int (uiout, "thread-id", pid_to_thread_id (inferior_ptid));
+
+  /* For mi, we just print location.  */
+  if (ui_out_is_mi_like_p (uiout))
+    print_stack_frame (get_selected_frame (NULL), 1, LOC_AND_ADDRESS);
+  else
+    print_stack_frame (get_selected_frame (NULL), 1, SRC_AND_LOC);
+
+  extra_info = target_extra_thread_info (tp);
+  if (extra_info)
+    ui_out_field_string (uiout, "thread-extra-info",extra_info);  
+
+  /* Restores the current thread, this also restores the current frame.  */
+  if (tidstr)
+    switch_to_thread (current_ptid);
+
+  return GDB_RC_OK;
+}
+
+
+enum gdb_rc
+gdb_thread_info (struct ui_out *uiout, char *tidstr, char **error_message)
+{
+  return catch_exceptions_with_msg (uiout, do_captured_thread_info, tidstr,
+				    error_message, RETURN_MASK_ALL);
+}
Index: mi/mi-cmds.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmds.c,v
retrieving revision 1.24
diff -u -p -r1.24 mi-cmds.c
--- mi/mi-cmds.c	2 Feb 2007 23:01:27 -0000	1.24
+++ mi/mi-cmds.c	19 Mar 2007 14:23:31 -0000
@@ -128,7 +128,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-info", { NULL, 0 }, 0, mi_cmd_thread_info},
   { "thread-list-all-threads", { NULL, 0 }, NULL, NULL },
   { "thread-list-ids", { NULL, 0 }, 0, mi_cmd_thread_list_ids},
   { "thread-select", { NULL, 0 }, 0, mi_cmd_thread_select},
Index: mi/mi-cmds.h
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmds.h,v
retrieving revision 1.21
diff -u -p -r1.21 mi-cmds.h
--- mi/mi-cmds.h	2 Feb 2007 23:01:27 -0000	1.21
+++ mi/mi-cmds.h	19 Mar 2007 14:23:31 -0000
@@ -104,6 +104,7 @@ extern mi_cmd_args_ftype mi_cmd_target_d
 extern mi_cmd_args_ftype mi_cmd_target_select;
 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_thread_info;
 extern mi_cmd_argv_ftype mi_cmd_var_assign;
 extern mi_cmd_argv_ftype mi_cmd_var_create;
 extern mi_cmd_argv_ftype mi_cmd_var_delete;
Index: mi/mi-main.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-main.c,v
retrieving revision 1.94
diff -u -p -r1.94 mi-main.c
--- mi/mi-main.c	3 Feb 2007 05:41:15 -0000	1.94
+++ mi/mi-main.c	19 Mar 2007 14:23:35 -0000
@@ -283,6 +283,27 @@ mi_cmd_thread_list_ids (char *command, c
 }
 
 enum mi_cmd_result
+mi_cmd_thread_info (char *command, char **argv, int argc)
+{
+  enum gdb_rc rc = MI_CMD_DONE;
+
+  if (argc == 0)
+    rc = gdb_thread_info (uiout, NULL, &mi_error_message);
+  else if (argc == 1)
+    rc = gdb_thread_info (uiout, argv[0], &mi_error_message);
+  else
+    {
+      mi_error_message = xstrprintf ("mi_cmd_thread_info: USAGE: -thread-info [threadnum].");
+      return MI_CMD_ERROR;
+    }
+
+  if (rc == GDB_RC_FAIL)
+    return MI_CMD_ERROR;
+  else
+    return MI_CMD_DONE;
+}
+
+enum mi_cmd_result
 mi_cmd_data_list_register_names (char *command, char **argv, int argc)
 {
   int regnum, numregs;

Re: [RFC] -thread-info new command

[Nick Roberts]

 > +enum gdb_rc
 > +gdb_thread_info (struct ui_out *uiout, char *tidstr, char **error_message)
 > +{
 > +  return catch_exceptions_with_msg (uiout, do_captured_thread_info, tidstr,
 > +				    error_message, RETURN_MASK_ALL);
 > +}

This has the same problem as gdb_thread_select i.e gdb_thread_info returns type
enum gdb_rc but catch_exceptions_with_msg returns type int.  I did suggest a
patch for this:

http://sourceware.org/ml/gdb-patches/2006-10/msg00220.html
(Re: [PATCH] PR mi/2086 -break-insert missing error diagnostic)

but the thread stopped at that point.  I think that we should resolve this
issue before adding further similar functions.


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

Re: [RFC] -thread-info new command

[Nick Roberts]

 > This has the same problem as gdb_thread_select i.e gdb_thread_info returns type
 > enum gdb_rc but catch_exceptions_with_msg returns type int.  I did suggest a
 > patch for this:
 > 
 > http://sourceware.org/ml/gdb-patches/2006-10/msg00220.html
 > (Re: [PATCH] PR mi/2086 -break-insert missing error diagnostic)
 > 
 > but the thread stopped at that point.  I think that we should resolve this
 > issue before adding further similar functions.

It looks like I've misremembered, as the patch I've quoted is for 
mi_cmd_break_insert and not gdb_thread_select.  However, perhaps we should
still resolve the issue first.

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

Re: [RFC] -thread-info new command

[Daniel Jacobowitz]

On Tue, Mar 20, 2007 at 03:09:19PM +1200, Nick Roberts wrote:
> It looks like I've misremembered, as the patch I've quoted is for 
> mi_cmd_break_insert and not gdb_thread_select.  However, perhaps we should
> still resolve the issue first.

Yes, I agree.  Is that the latest patch posted, if you know offhand?

I am planning to spend a big chunk of tomorrow morning catching up on
neglected gdb patches; I'll make sure we resolve this.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] -thread-info new command

[Nick Roberts]

 > > It looks like I've misremembered, as the patch I've quoted is for 
 > > mi_cmd_break_insert and not gdb_thread_select.  However, perhaps we should
 > > still resolve the issue first.
 > 
 > Yes, I agree.  Is that the latest patch posted, if you know offhand?

I had thought it was, but no.  You replied with:

http://sourceware.org/ml/gdb-patches/2006-11/msg00185.html:

    It seems pretty clear to me that the patch which switched things to
    return the result of catch_exceptions_with_msg was wrong.  The
    functions are defined to return an enum gdb_rc.  Can't we make
    them do that again?  Simple, obviously correct.

but I could only come up with:

http://sourceware.org/ml/gdb-patches/2006-11/msg00208.html

which didn't seem that simple and is probably not correct

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

Re: [RFC] -thread-info new command

[Nick Roberts]

 > but I could only come up with:
 > 
 > http://sourceware.org/ml/gdb-patches/2006-11/msg00208.html
 > 
 > which didn't seem that simple and is probably not correct

I've gone through the archives to try to understand the changes.   The above
message says:

   Well we need to propagate the error message back to
   captured_mi_execute_command.

Actually there are currently two ways to catch an error in MI:

1) Using error () and catch_exception.

2) Using MI_CMD_ERROR and mi_error_message.

The first gets caught in mi_execute_command and the error message is stored
in result.message.  The second goes back to captured_mi_execute_command and
the error message is manually stored in mi_error_message.

I think that only one method should be used and this should be the first one.

It appears that catch_exceptions_with_msg was used to avoid using
error_last_message () but this was no longer needed when mi_execute_command was
changed to use catch_exception instead of catch_exceptions (the comment "Can
this use of catch_exceptions..." is an anachronism).  The error message should
be available in result.message for these functions (gdb_thread_select,
gdb_breakpoint,..)  anyway.

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

[RFC] gdb_breakpoint Re: [RFC] -thread-info new command

[Nick Roberts]

 > Actually there are currently two ways to catch an error in MI:
 > 
 > 1) Using error () and catch_exception.
 > 
 > 2) Using MI_CMD_ERROR and mi_error_message.
 > 
 > The first gets caught in mi_execute_command and the error message is stored
 > in result.message.  The second goes back to captured_mi_execute_command and
 > the error message is manually stored in mi_error_message.
 > 
 > I think that only one method should be used and this should be the first one.

Here's a patch which merges do_captured_breakpoint into gdb_breakpoint so that
errors get caught further up in mi_execute_command.  The same thing can be done
for gdb_breakpoint_query and gdb_list_thread_ids.  The function
gdb_thread_select is also used by CLI, but I don't know if that would
cause problems.

gdb_breakpoint has the extra complication that deprecated_set_gdb_event_hooks
have to be restored.  I've tried to deal with that in the changes to
mi-cmd-break.c.

This is a request for comment, not approval.  I guess cleanup_gdb_event_hooks
would have to go elsewhere/have another name.  There are probably other issues.
I'm trying to float ideas.

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


2007-03-22  Nick Roberts  <nickrob@snap.net.nz>

	* mi/mi-cmd-break.c (cleanup_gdb_event_hooks): New function.
	(mi_cmd_break_insert): Use it.

	* breakpoint.c (captured_breakpoint_args): Delete.
	(do_captured_breakpoint): Delete.  Merge functionality into...
	(gdb_breakpoint): ...here.  Don't catch errors.


*** mi-cmd-break.c	10 Jan 2007 11:56:57 +1300	1.15
--- mi-cmd-break.c	22 Mar 2007 16:19:17 +1200	
*************** enum bp_type
*** 58,63 ****
--- 58,69 ----
      REGEXP_BP
    };
  
+ void
+ cleanup_gdb_event_hooks (void *ptr)
+ {
+   deprecated_set_gdb_event_hooks (ptr);
+ }
+ 
  /* Insert a breakpoint. The type of breakpoint is specified by the
     first argument: -break-insert <location> --> insert a regular
     breakpoint.  -break-insert -t <location> --> insert a temporary
*************** mi_cmd_break_insert (char *command, char
*** 77,82 ****
--- 83,89 ----
    int ignore_count = 0;
    char *condition = NULL;
    enum gdb_rc rc;
+   struct cleanup *old_cleanups;
    struct gdb_events *old_hooks;
    enum opt
      {
*************** mi_cmd_break_insert (char *command, char
*** 135,153 ****
  
    /* Now we have what we need, let's insert the breakpoint! */
    old_hooks = deprecated_set_gdb_event_hooks (&breakpoint_hooks);
    switch (type)
      {
      case REG_BP:
        rc = gdb_breakpoint (address, condition,
  			   0 /*hardwareflag */ , temp_p,
! 			   thread, ignore_count,
! 			   &mi_error_message);
        break;
      case HW_BP:
        rc = gdb_breakpoint (address, condition,
  			   1 /*hardwareflag */ , temp_p,
! 			   thread, ignore_count,
! 			   &mi_error_message);
        break;
  #if 0
      case REGEXP_BP:
--- 142,159 ----
  
    /* Now we have what we need, let's insert the breakpoint! */
    old_hooks = deprecated_set_gdb_event_hooks (&breakpoint_hooks);
+   old_cleanups = make_cleanup (cleanup_gdb_event_hooks, old_hooks);
    switch (type)
      {
      case REG_BP:
        rc = gdb_breakpoint (address, condition,
  			   0 /*hardwareflag */ , temp_p,
! 			   thread, ignore_count);
        break;
      case HW_BP:
        rc = gdb_breakpoint (address, condition,
  			   1 /*hardwareflag */ , temp_p,
! 			   thread, ignore_count);
        break;
  #if 0
      case REGEXP_BP:
*************** mi_cmd_break_insert (char *command, char
*** 162,168 ****
        internal_error (__FILE__, __LINE__,
  		      _("mi_cmd_break_insert: Bad switch."));
      }
!   deprecated_set_gdb_event_hooks (old_hooks);
  
    if (rc == GDB_RC_FAIL)
      return MI_CMD_ERROR;
--- 168,174 ----
        internal_error (__FILE__, __LINE__,
  		      _("mi_cmd_break_insert: Bad switch."));
      }
!   do_cleanups (old_cleanups);
  
    if (rc == GDB_RC_FAIL)
      return MI_CMD_ERROR;


*** breakpoint.c	14 Mar 2007 00:13:43 +1300	1.242
--- breakpoint.c	22 Mar 2007 12:57:35 +1200	
*************** break_command_1 (char *arg, int flag, in
*** 5476,5498 ****
    return GDB_RC_OK;
  }
  
! /* Set a breakpoint of TYPE/DISPOSITION according to ARG (function,
!    linenum or *address) with COND and IGNORE_COUNT. */
  
! struct captured_breakpoint_args
!   {
!     char *address;
!     char *condition;
!     int hardwareflag;
!     int tempflag;
!     int thread;
!     int ignore_count;
!   };
! 
! static int
! do_captured_breakpoint (struct ui_out *uiout, void *data)
  {
-   struct captured_breakpoint_args *args = data;
    struct symtabs_and_lines sals;
    struct expression **cond;
    struct cleanup *old_chain;
--- 5476,5489 ----
    return GDB_RC_OK;
  }
  
! /* Set a breakpoint of TYPE/DISPOSITION according to function,
!    linenum or *address, with COND and IGNORE_COUNT. */
  
! enum gdb_rc
! gdb_breakpoint (char *address, char *condition,
! 		int hardwareflag, int tempflag,
! 		int thread, int ignore_count)
  {
    struct symtabs_and_lines sals;
    struct expression **cond;
    struct cleanup *old_chain;
*************** do_captured_breakpoint (struct ui_out *u
*** 5508,5514 ****
       place. */
    sals.sals = NULL;
    sals.nelts = 0;
!   address_end = args->address;
    addr_string = NULL;
    parse_breakpoint_sals (&address_end, &sals, &addr_string, 0);
  
--- 5499,5505 ----
       place. */
    sals.sals = NULL;
    sals.nelts = 0;
!   address_end = address;
    addr_string = NULL;
    parse_breakpoint_sals (&address_end, &sals, &addr_string, 0);
  
*************** do_captured_breakpoint (struct ui_out *u
*** 5554,5580 ****
      error (_("Garbage %s following breakpoint address"), address_end);
  
    /* Resolve all line numbers to PC's.  */
!   breakpoint_sals_to_pc (&sals, args->address);
  
    /* Verify that conditions can be parsed, before setting any
       breakpoints.  */
    for (i = 0; i < sals.nelts; i++)
      {
!       if (args->condition != NULL)
  	{
! 	  char *tok = args->condition;
  	  cond[i] = parse_exp_1 (&tok, block_for_pc (sals.sals[i].pc), 0);
  	  if (*tok != '\0')
  	    error (_("Garbage %s follows condition"), tok);
  	  make_cleanup (xfree, cond[i]);
! 	  cond_string[i] = xstrdup (args->condition);
  	}
      }
  
    create_breakpoints (sals, addr_string, cond, cond_string,
! 		      args->hardwareflag ? bp_hardware_breakpoint : bp_breakpoint,
! 		      args->tempflag ? disp_del : disp_donttouch,
! 		      args->thread, args->ignore_count, 0/*from-tty*/, 
  		      NULL/*pending_bp*/);
  
    /* That's it. Discard the cleanups for data inserted into the
--- 5545,5571 ----
      error (_("Garbage %s following breakpoint address"), address_end);
  
    /* Resolve all line numbers to PC's.  */
!   breakpoint_sals_to_pc (&sals, address);
  
    /* Verify that conditions can be parsed, before setting any
       breakpoints.  */
    for (i = 0; i < sals.nelts; i++)
      {
!       if (condition != NULL)
  	{
! 	  char *tok = condition;
  	  cond[i] = parse_exp_1 (&tok, block_for_pc (sals.sals[i].pc), 0);
  	  if (*tok != '\0')
  	    error (_("Garbage %s follows condition"), tok);
  	  make_cleanup (xfree, cond[i]);
! 	  cond_string[i] = xstrdup (condition);
  	}
      }
  
    create_breakpoints (sals, addr_string, cond, cond_string,
! 		      hardwareflag ? bp_hardware_breakpoint : bp_breakpoint,
! 		      tempflag ? disp_del : disp_donttouch,
! 		      thread, ignore_count, 0/*from-tty*/, 
  		      NULL/*pending_bp*/);
  
    /* That's it. Discard the cleanups for data inserted into the
*************** do_captured_breakpoint (struct ui_out *u
*** 5585,5608 ****
    return GDB_RC_OK;
  }
  
- enum gdb_rc
- gdb_breakpoint (char *address, char *condition,
- 		int hardwareflag, int tempflag,
- 		int thread, int ignore_count,
- 		char **error_message)
- {
-   struct captured_breakpoint_args args;
-   args.address = address;
-   args.condition = condition;
-   args.hardwareflag = hardwareflag;
-   args.tempflag = tempflag;
-   args.thread = thread;
-   args.ignore_count = ignore_count;
-   return catch_exceptions_with_msg (uiout, do_captured_breakpoint, &args,
- 				    error_message, RETURN_MASK_ALL);
- }
- 
- 
  /* Helper function for break_command_1 and disassemble_command.  */
  
  void
--- 5576,5581 ----

Re: [RFC] -thread-info new command

[Daniel Jacobowitz]

On Thu, Mar 22, 2007 at 09:09:15AM +1200, Nick Roberts wrote:
> I've gone through the archives to try to understand the changes.   The above
> message says:
> 
>    Well we need to propagate the error message back to
>    captured_mi_execute_command.
> 
> Actually there are currently two ways to catch an error in MI:
> 
> 1) Using error () and catch_exception.
> 
> 2) Using MI_CMD_ERROR and mi_error_message.
> 
> The first gets caught in mi_execute_command and the error message is stored
> in result.message.  The second goes back to captured_mi_execute_command and
> the error message is manually stored in mi_error_message.
> 
> I think that only one method should be used and this should be the first one.

Once upon a time there were supposed to be more things using "libgdb"
- these gdb_* wrapper functions.  It didn't come to pass.

For now can we do the minimal fix for this problem?  I apologize I was
never clear enough about what I meant when I said that, so here's a
patch.  Before:

-break-insert *
&"Argument required (expression to compute).\n"
^done

After:

-break-insert *
&"Argument required (expression to compute).\n"
^error,msg="Argument required (expression to compute)."

I'm running the testsuite on this now.

-- 
Daniel Jacobowitz
CodeSourcery

2007-03-27  Daniel Jacobowitz  <dan@codesourcery.com>

	* breakpoint.c (gdb_breakpoint_query): Really return an
	enum gdb_rc.
	(gdb_breakpoint): Likewise.
	* thread.c (do_captured_list_thread_ids): Likewise.
	(do_captured_thread_select): Likewise.
	* mi/mi-main.c (mi_cmd_thread_select): Expect an enum gdb_rc.
	(mi_cmd_thread_list_ids): Remove bogus initialization.

Index: breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.242
diff -u -p -r1.242 breakpoint.c
--- breakpoint.c	9 Mar 2007 16:20:42 -0000	1.242
+++ breakpoint.c	27 Mar 2007 19:51:50 -0000
@@ -3755,8 +3755,11 @@ gdb_breakpoint_query (struct ui_out *uio
   args.bnum = bnum;
   /* For the moment we don't trust print_one_breakpoint() to not throw
      an error. */
-  return catch_exceptions_with_msg (uiout, do_captured_breakpoint_query, &args,
-				    error_message, RETURN_MASK_ALL);
+  if (catch_exceptions_with_msg (uiout, do_captured_breakpoint_query, &args,
+				 error_message, RETURN_MASK_ALL) < 0)
+    return GDB_RC_FAIL;
+  else
+    return GDB_RC_OK;
 }
 
 /* Return non-zero if B is user settable (breakpoints, watchpoints,
@@ -5598,8 +5601,11 @@ gdb_breakpoint (char *address, char *con
   args.tempflag = tempflag;
   args.thread = thread;
   args.ignore_count = ignore_count;
-  return catch_exceptions_with_msg (uiout, do_captured_breakpoint, &args,
-				    error_message, RETURN_MASK_ALL);
+  if (catch_exceptions_with_msg (uiout, do_captured_breakpoint, &args,
+				 error_message, RETURN_MASK_ALL) < 0)
+    return GDB_RC_FAIL;
+  else
+    return GDB_RC_OK;
 }
 
 
Index: thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.51
diff -u -p -r1.51 thread.c
--- thread.c	28 Feb 2007 17:35:01 -0000	1.51
+++ thread.c	27 Mar 2007 19:51:50 -0000
@@ -286,8 +286,10 @@ do_captured_list_thread_ids (struct ui_o
 enum gdb_rc
 gdb_list_thread_ids (struct ui_out *uiout, char **error_message)
 {
-  return catch_exceptions_with_msg (uiout, do_captured_list_thread_ids, NULL,
-				    error_message, RETURN_MASK_ALL);
+  if (catch_exceptions_with_msg (uiout, do_captured_list_thread_ids, NULL,
+				 error_message, RETURN_MASK_ALL) < 0)
+    return GDB_RC_FAIL;
+  return GDB_RC_OK;
 }
 
 /* Load infrun state for the thread PID.  */
@@ -707,8 +709,10 @@ do_captured_thread_select (struct ui_out
 enum gdb_rc
 gdb_thread_select (struct ui_out *uiout, char *tidstr, char **error_message)
 {
-  return catch_exceptions_with_msg (uiout, do_captured_thread_select, tidstr,
-				    error_message, RETURN_MASK_ALL);
+  if (catch_exceptions_with_msg (uiout, do_captured_thread_select, tidstr,
+				 error_message, RETURN_MASK_ALL) < 0)
+    return GDB_RC_FAIL;
+  return GDB_RC_OK;
 }
 
 /* Commands with a prefix of `thread'.  */
Index: mi/mi-main.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-main.c,v
retrieving revision 1.94
diff -u -p -r1.94 mi-main.c
--- mi/mi-main.c	3 Feb 2007 05:41:15 -0000	1.94
+++ mi/mi-main.c	27 Mar 2007 19:51:50 -0000
@@ -253,11 +253,7 @@ mi_cmd_thread_select (char *command, cha
   else
     rc = gdb_thread_select (uiout, argv[0], &mi_error_message);
 
-  /* RC is enum gdb_rc if it is successful (>=0)
-     enum return_reason if not (<0).  */
-  if ((int) rc < 0 && (enum return_reason) rc == RETURN_ERROR)
-    return MI_CMD_ERROR;
-  else if ((int) rc >= 0 && rc == GDB_RC_FAIL)
+  if (rc == GDB_RC_FAIL)
     return MI_CMD_ERROR;
   else
     return MI_CMD_DONE;
@@ -266,7 +262,7 @@ mi_cmd_thread_select (char *command, cha
 enum mi_cmd_result
 mi_cmd_thread_list_ids (char *command, char **argv, int argc)
 {
-  enum gdb_rc rc = MI_CMD_DONE;
+  enum gdb_rc rc;
 
   if (argc != 0)
     {

Re: [RFC] -thread-info new command

[Nick Roberts]

 > > Actually there are currently two ways to catch an error in MI:
 > > 
 > > 1) Using error () and catch_exception.
 > > 
 > > 2) Using MI_CMD_ERROR and mi_error_message.
 > > 
 > > The first gets caught in mi_execute_command and the error message is stored
 > > in result.message.  The second goes back to captured_mi_execute_command and
 > > the error message is manually stored in mi_error_message.
 > > 
 > > I think that only one method should be used and this should be the first
 > > one.
 > 
 > Once upon a time there were supposed to be more things using "libgdb"
 > - these gdb_* wrapper functions.  It didn't come to pass.

I thought the gdb_* wrapper function were just designed to catch exceptions.
Does your statement defeat the logic of my suggestion?

 > For now can we do the minimal fix for this problem?  I apologize I was
 > never clear enough about what I meant when I said that, so here's a
 > patch.

Yes.  I can't believe now that I didn't consider this option.

 > ...
 > 2007-03-27  Daniel Jacobowitz  <dan@codesourcery.com>
 > 
 > 	* breakpoint.c (gdb_breakpoint_query): Really return an
 > 	enum gdb_rc.
 > 	(gdb_breakpoint): Likewise.
 > 	* thread.c (do_captured_list_thread_ids): Likewise.
 > 	(do_captured_thread_select): Likewise.
 > 	* mi/mi-main.c (mi_cmd_thread_select): Expect an enum gdb_rc.
 > 	(mi_cmd_thread_list_ids): Remove bogus initialization.

I think that the do_captured_* functions should have return type enum gdb_rc
not int.

More generally though, re my patch, does make_cleaunp work on
deprecated_set_gdb_event_hooks?  Do you think it's a good idea to distinguish
between user errors, e.g, "No stack." and front end errors, e.g,
"-var-delete: Usage: [-c] EXPRESSION."?

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

Re: [RFC] -thread-info new command

[Denis PILAT]

Nick Roberts wrote:
>  > > Actually there are currently two ways to catch an error in MI:
>  > > 
>  > > 1) Using error () and catch_exception.
>  > > 
>  > > 2) Using MI_CMD_ERROR and mi_error_message.
>  > > 
>  > > The first gets caught in mi_execute_command and the error message is stored
>  > > in result.message.  The second goes back to captured_mi_execute_command and
>  > > the error message is manually stored in mi_error_message.
>  > > 
>  > > I think that only one method should be used and this should be the first
>  > > one.
>  > 
>  > Once upon a time there were supposed to be more things using "libgdb"
>  > - these gdb_* wrapper functions.  It didn't come to pass.
>
> I thought the gdb_* wrapper function were just designed to catch exceptions.
> Does your statement defeat the logic of my suggestion?
>
>  > For now can we do the minimal fix for this problem?  I apologize I was
>  > never clear enough about what I meant when I said that, so here's a
>  > patch.
>
> Yes.  I can't believe now that I didn't consider this option.
>
>  > ...
>  > 2007-03-27  Daniel Jacobowitz  <dan@codesourcery.com>
>  > 
>  > 	* breakpoint.c (gdb_breakpoint_query): Really return an
>  > 	enum gdb_rc.
>  > 	(gdb_breakpoint): Likewise.
>  > 	* thread.c (do_captured_list_thread_ids): Likewise.
>  > 	(do_captured_thread_select): Likewise.
>  > 	* mi/mi-main.c (mi_cmd_thread_select): Expect an enum gdb_rc.
>  > 	(mi_cmd_thread_list_ids): Remove bogus initialization.
>
> I think that the do_captured_* functions should have return type enum gdb_rc
> not int.
>
> More generally though, re my patch, does make_cleaunp work on
> deprecated_set_gdb_event_hooks?  Do you think it's a good idea to distinguish
> between user errors, e.g, "No stack." and front end errors, e.g,
> "-var-delete: Usage: [-c] EXPRESSION."?
>
>   

Is there anything new about that ?
Should I re-propose a patch for -thread-info ?
-- 
Denis

Re: [RFC] -thread-info new command

[Daniel Jacobowitz]

On Wed, Apr 04, 2007 at 04:36:21PM +0200, Denis PILAT wrote:
> Is there anything new about that ?
> Should I re-propose a patch for -thread-info ?

Yes, please.  Although, I think that a manual patch is a more useful
way to start the discussion for a new MI command than a code patch.

You said in your original posting that Eclipse uses info threads but
only collects the thread ID and extra info from it; so I think the two
useful commands for Eclipse are "-thread-info" which returns the extra
info, and something which returns all the IDs and extra info.  Then
you can use one if you know only a few threads are visible, and the
other if you want to eagerly cache all the thread info.

The other two maybe useful models for an IDE are "all threads, with
stack but without extra info" and "all threads with both stack and
extra info".  That might depend on a lot of things.  If the IDE knows
in advance that the thread extra info never changes, it can avoid
requesting it.

Nick and Vladimir were talking about -var-list a month or so ago.
Maybe we should have something similar here - WDYT?

  -thread-list [--extra] [--stack]

Or maybe we should always provide the extra info, and have a way to
tell GDB that it never changes, so it can avoid the expensive queries
to the target.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] -thread-info new command

[Daniel Jacobowitz]

On Wed, Mar 28, 2007 at 09:33:52AM +1200, Nick Roberts wrote:
>  > Once upon a time there were supposed to be more things using "libgdb"
>  > - these gdb_* wrapper functions.  It didn't come to pass.
> 
> I thought the gdb_* wrapper function were just designed to catch exceptions.

They were designed to isolate users of libgdb from GDB's internal
exception handling mechanism, which is basically the same thing.

> Does your statement defeat the logic of my suggestion?

Honestly, I'm not sure.  I looked at your patch again; I don't think I
understand why you want to change gdb_breakpoint.

>  > 2007-03-27  Daniel Jacobowitz  <dan@codesourcery.com>
>  > 
>  > 	* breakpoint.c (gdb_breakpoint_query): Really return an
>  > 	enum gdb_rc.
>  > 	(gdb_breakpoint): Likewise.
>  > 	* thread.c (do_captured_list_thread_ids): Likewise.
>  > 	(do_captured_thread_select): Likewise.
>  > 	* mi/mi-main.c (mi_cmd_thread_select): Expect an enum gdb_rc.
>  > 	(mi_cmd_thread_list_ids): Remove bogus initialization.
> 
> I think that the do_captured_* functions should have return type enum gdb_rc
> not int.

Yes, it would be nice - but unfortunately they can't, since
catch_exceptions_with_msg requires the function to return an int.  So
I checked it in the way it is.  We need to cut down on the number of
exception handling interfaces, but I'm not going to tackle that one
today!

> More generally though, re my patch, does make_cleaunp work on
> deprecated_set_gdb_event_hooks?  Do you think it's a good idea to distinguish
> between user errors, e.g, "No stack." and front end errors, e.g,
> "-var-delete: Usage: [-c] EXPRESSION."?

It would be nice if front ends could separately detect "the front end
has done something silly" -I do not think it's a big deal, but it
might make correct front ends easier to write, and that would help
everybody.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] -thread-info new command

[Nick Roberts]

> > Does your statement defeat the logic of my suggestion?

> Honestly, I'm not sure.  I looked at your patch again; I don't think I
> understand why you want to change gdb_breakpoint.

So the errors get caught in mi_execute_command (since they are not front end
errors) as I've already said.

 > > I think that the do_captured_* functions should have return type enum
 > > gdb_rc not int.
 > 
 > Yes, it would be nice - but unfortunately they can't, since
 > catch_exceptions_with_msg requires the function to return an int.

We must be reading a different book:

/* Print a list of thread ids currently known, and the total number of
   threads. To be used from within catch_errors. */
static int
do_captured_list_thread_ids (struct ui_out *uiout, void *arg)
{
  ...
  do_cleanups (cleanup_chain);
  ui_out_field_int (uiout, "number-of-threads", num);
  return GDB_RC_OK;
}

I think your ChangeLog entry should have read:

	* thread.c (gdb_list_thread_ids): Likewise.
	(gdb_thread_select): Likewise.

 >...
 > > More generally though, re my patch, does make_cleaunp work on
 > > deprecated_set_gdb_event_hooks?  Do you think it's a good idea to
 > > distinguish between user errors, e.g, "No stack." and front end errors,
 > > e.g, "-var-delete: Usage: [-c] EXPRESSION."?
 > 
 > It would be nice if front ends could separately detect "the front end
 > has done something silly" -I do not think it's a big deal, but it
 > might make correct front ends easier to write, and that would help
 > everybody.

It would also help stop bugs in front ends from being reported on the gdb
mailing list.


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

Re: [RFC] -thread-info new command

[Daniel Jacobowitz]

On Wed, Apr 11, 2007 at 09:52:16AM +1200, Nick Roberts wrote:
>  > > I think that the do_captured_* functions should have return type enum
>  > > gdb_rc not int.
>  > 
>  > Yes, it would be nice - but unfortunately they can't, since
>  > catch_exceptions_with_msg requires the function to return an int.
> 
> We must be reading a different book:

Don't understand what you mean.

> /* Print a list of thread ids currently known, and the total number of
>    threads. To be used from within catch_errors. */

This comment is out of date.

> static int

This is the bit I was talking about, since catch_excepions_with_msg
requires it be "int" and not "enum gdb_rc".

> do_captured_list_thread_ids (struct ui_out *uiout, void *arg)
> {
>   ...
>   do_cleanups (cleanup_chain);
>   ui_out_field_int (uiout, "number-of-threads", num);
>   return GDB_RC_OK;
> }
> 
> I think your ChangeLog entry should have read:
> 
> 	* thread.c (gdb_list_thread_ids): Likewise.
> 	(gdb_thread_select): Likewise.

Yes, you're right.  Thanks.  I just fixed it.

(diff -p and my carelessness are to blame)

>  > It would be nice if front ends could separately detect "the front end
>  > has done something silly" -I do not think it's a big deal, but it
>  > might make correct front ends easier to write, and that would help
>  > everybody.
> 
> It would also help stop bugs in front ends from being reported on the gdb
> mailing list.

Maybe - not if the front end didn't handle it...

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] -thread-info new command

[Nick Roberts]

 > This is the bit I was talking about, since catch_excepions_with_msg
 > requires it be "int" and not "enum gdb_rc".
 > 
 > > do_captured_list_thread_ids (struct ui_out *uiout, void *arg)
 > > {
 > >   ...
 > >   do_cleanups (cleanup_chain);
 > >   ui_out_field_int (uiout, "number-of-threads", num);
 > >   return GDB_RC_OK;
 > > }

OK I see now.  There still seems to be some kind of mismatch.  Perhaps
(sometime) enum return_reason and enum gdb_rc can be harmonised.

 >...
 > > It would also help stop bugs in front ends from being reported on the gdb
 > > mailing list.
 > 
 > Maybe - not if the front end didn't handle it...

Sure.  If poeple elect not to use a feature, it won't provide any benefit.

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