[patch:MI] Observer for thread-changed

[patch:MI] Observer for thread-changed

[Nick Roberts]

This patch is the thread-changed notification for MI using an observer.

No regressions.

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


2008-06-09  Nick Roberts  <nickrob@snap.net.nz>

	* mi/mi-interp.c (mi_thread_changed): New static function.
	(mi_interpreter_init): Register mi_thread_changed as thread_changed
	observer .
	* thread.c (thread_command): Notify thread changes using observer.
	* infrun.c (normal_stop): Ditto.
	* mi/mi-main.c (mi_cmd_thread_select): Ditto.
	* Makefile.in (mi-main.o) : Add dependency on observer.h.

2008-06-09  Nick Roberts  <nickrob@snap.net.nz>

	* observer.texi (GDB Observers): New observer for thread_changed.


Index: thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.71
diff -p -u -p -r1.71 thread.c
--- thread.c	6 Jun 2008 00:32:51 -0000	1.71
+++ thread.c	9 Jun 2008 12:13:23 -0000
@@ -740,6 +740,7 @@ thread_command (char *tidstr, int from_t
 
   annotate_thread_changed ();
   gdb_thread_select (uiout, tidstr, NULL);
+  observer_notify_thread_changed ();
 }
 
 /* Print notices when new threads are attached and detached.  */
Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.278
diff -p -u -p -r1.278 infrun.c
--- infrun.c	6 Jun 2008 00:33:52 -0000	1.278
+++ infrun.c	9 Jun 2008 12:13:25 -0000
@@ -3605,6 +3605,7 @@ normal_stop (void)
       target_terminal_ours_for_output ();
       printf_filtered (_("[Switching to %s]\n"),
 		       target_pid_to_str (inferior_ptid));
+      observer_notify_thread_changed ();
       annotate_thread_changed ();
       previous_inferior_ptid = inferior_ptid;
     }
Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.1026
diff -p -u -p -r1.1026 Makefile.in
--- Makefile.in	6 Jun 2008 20:58:08 -0000	1.1026
+++ Makefile.in	9 Jun 2008 12:13:28 -0000
@@ -3285,7 +3285,8 @@ mi-main.o: $(srcdir)/mi/mi-main.c $(defs
 	$(gdb_string_h) $(exceptions_h) $(top_h) $(gdbthread_h) $(mi_cmds_h) \
 	$(mi_parse_h) $(mi_getopt_h) $(mi_console_h) $(ui_out_h) $(mi_out_h) \
 	$(interps_h) $(event_loop_h) $(event_top_h) $(gdbcore_h) $(value_h) \
-	$(regcache_h) $(gdb_h) $(frame_h) $(mi_main_h) $(language_h)
+	$(regcache_h) $(gdb_h) $(frame_h) $(mi_main_h) $(language_h) \
+	$(observer_h)
 	$(CC) -c $(INTERNAL_CFLAGS) $(srcdir)/mi/mi-main.c
 mi-out.o: $(srcdir)/mi/mi-out.c $(defs_h) $(ui_out_h) $(mi_out_h)
 	$(CC) -c $(INTERNAL_CFLAGS) $(srcdir)/mi/mi-out.c
Index: mi/mi-interp.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-interp.c,v
retrieving revision 1.30
diff -p -u -p -r1.30 mi-interp.c
--- mi/mi-interp.c	3 May 2008 15:10:42 -0000	1.30
+++ mi/mi-interp.c	9 Jun 2008 12:13:28 -0000
@@ -68,6 +68,7 @@ static void mi_remove_notify_hooks (void
 
 static void mi_new_thread (struct thread_info *t);
 static void mi_thread_exit (struct thread_info *t);
+static void mi_thread_changed (void);
 
 static void *
 mi_interpreter_init (int top_level)
@@ -92,6 +93,7 @@ mi_interpreter_init (int top_level)
     {
       observer_attach_new_thread (mi_new_thread);
       observer_attach_thread_exit (mi_thread_exit);
+      observer_attach_thread_changed (mi_thread_changed);
     }
 
   return mi;
@@ -331,6 +333,27 @@ mi_thread_exit (struct thread_info *t)
   gdb_flush (mi->event_channel);
 }
 
+static void
+mi_thread_changed ()
+{
+  struct mi_interp *mi = top_level_interpreter_data ();
+  struct interp *interp_to_use;
+  struct ui_out *old_uiout, *temp_uiout;
+  int version;
+
+  fprintf_unfiltered (mi->event_channel, "thread-changed");
+  interp_to_use = top_level_interpreter ();
+  old_uiout = uiout;
+  temp_uiout = interp_ui_out (interp_to_use);
+  version = mi_version (temp_uiout);
+  temp_uiout = mi_out_new (version);
+  uiout = temp_uiout;
+  ui_out_field_int (uiout, "new-thread-id", pid_to_thread_id (inferior_ptid));
+  mi_out_put (uiout, mi->event_channel);
+  uiout = old_uiout;
+  gdb_flush (mi->event_channel);
+}
+
 extern initialize_file_ftype _initialize_mi_interp; /* -Wmissing-prototypes */
 
 void
Index: mi/mi-main.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-main.c,v
retrieving revision 1.115
diff -p -u -p -r1.115 mi-main.c
--- mi/mi-main.c	6 May 2008 21:35:01 -0000	1.115
+++ mi/mi-main.c	9 Jun 2008 12:13:28 -0000
@@ -45,6 +45,7 @@
 #include "frame.h"
 #include "mi-main.h"
 #include "language.h"
+#include "observer.h"
 
 #include <ctype.h>
 #include <sys/time.h>
@@ -241,6 +242,7 @@ mi_cmd_thread_select (char *command, cha
     error ("mi_cmd_thread_select: USAGE: threadnum.");
 
   rc = gdb_thread_select (uiout, argv[0], &mi_error_message);
+  observer_notify_thread_changed ();
 
   if (rc == GDB_RC_FAIL)
     {
Index: doc/observer.texi
===================================================================
RCS file: /cvs/src/src/gdb/doc/observer.texi,v
retrieving revision 1.15
diff -p -u -p -r1.15 observer.texi
--- doc/observer.texi	3 May 2008 15:10:42 -0000	1.15
+++ doc/observer.texi	9 Jun 2008 12:13:28 -0000
@@ -137,3 +137,6 @@ The thread specified by @var{t} has been
 The thread specified by @var{t} has exited.
 @end deftypefun
 
+@deftypefun void thread_changed ()
+A new thread has been selected.
+@end deftypefun

Re: [patch:MI] Observer for thread-changed

[Pedro Alves]

Sorry, if I missed the discussion on it, but,

A Monday 09 June 2008 13:16:09, Nick Roberts wrote:
>    annotate_thread_changed ();
>    gdb_thread_select (uiout, tidstr, NULL);
> +  observer_notify_thread_changed ();
>  }

This is conceptually not right. gdb_thread_select is a libgdb
function, that filters exceptions.  If do_captured_thread_select
throws an error, you will still call the observer.  Plus, 
do_captured_thread_select is already printing the thread change
to MI, which means you'll get the output twice now, in MI?

Why not call the observer from inside do_captured_thread_select,
instead of on both CLI and MI commands?

-- 
Pedro Alves

Re: [patch:MI] Observer for thread-changed

[Pedro Alves]

Sorry, if I missed the discussion on it, but,

A Monday 09 June 2008 13:16:09, Nick Roberts wrote:
>    annotate_thread_changed ();
>    gdb_thread_select (uiout, tidstr, NULL);
> +  observer_notify_thread_changed ();
>  }

This is conceptually not right. gdb_thread_select is a libgdb
function, that filters exceptions.  If do_captured_thread_select
throws an error, you will still call the observer.  Plus, 
do_captured_thread_select is already printing the thread change
to MI, which means you'll get the output twice now, in MI?

Why not call the observer from inside do_captured_thread_select,
instead of on both CLI and MI commands?

-- 
Pedro Alves

Re: [patch:MI] Observer for thread-changed

[Pedro Alves]

A Monday 09 June 2008 14:28:28, Pedro Alves wrote:

> Plus, do_captured_thread_select is already printing the thread change
> to MI, which means you'll get the output twice now, in MI?

Oh, got it, you're the output as an event in the observer.  The rest
of the comment still applies though.

Another issue:

A Monday 09 June 2008 13:16:09, Nick Roberts wrote:
> /* Print notices when new threads are attached and detached. */
> Index: infrun.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infrun.c,v
> retrieving revision 1.278
> diff -p -u -p -r1.278 infrun.c
> --- infrun.c 6 Jun 2008 00:33:52 -0000 1.278
> +++ infrun.c 9 Jun 2008 12:13:25 -0000
> @@ -3605,6 +3605,7 @@ normal_stop (void)
>      target_terminal_ours_for_output ();
>      printf_filtered (_("[Switching to %s]\n"),
>      target_pid_to_str (inferior_ptid));
> +    observer_notify_thread_changed ();
>      annotate_thread_changed ();
>      previous_inferior_ptid = inferior_ptid;
> }

Hmm, will we want the observer to have access to the selected frame
of the new selected thread?  If so, then, the observer call should be
moved to the end of normal_stop after the "done:" label,
so any dummy frame is poped; if not, then I guess a comment here would
be good, as it seems something natural to be doing from inside
the observer in the future.

Also, it may make sense to add a "reason" parameter to
the observer, as in "changed due to user/frontend request", or
"due to a stop event", but that's not actually required right now.

-- 
Pedro Alves

Re: [patch:MI] Observer for thread-changed

[Pedro Alves]

A Monday 09 June 2008 14:28:28, Pedro Alves wrote:

> Plus, do_captured_thread_select is already printing the thread change
> to MI, which means you'll get the output twice now, in MI?

Oh, got it, you're the output as an event in the observer.  The rest
of the comment still applies though.

Another issue:

A Monday 09 June 2008 13:16:09, Nick Roberts wrote:
> /* Print notices when new threads are attached and detached. */
> Index: infrun.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infrun.c,v
> retrieving revision 1.278
> diff -p -u -p -r1.278 infrun.c
> --- infrun.c 6 Jun 2008 00:33:52 -0000 1.278
> +++ infrun.c 9 Jun 2008 12:13:25 -0000
> @@ -3605,6 +3605,7 @@ normal_stop (void)
>      target_terminal_ours_for_output ();
>      printf_filtered (_("[Switching to %s]\n"),
>      target_pid_to_str (inferior_ptid));
> +    observer_notify_thread_changed ();
>      annotate_thread_changed ();
>      previous_inferior_ptid = inferior_ptid;
> }

Hmm, will we want the observer to have access to the selected frame
of the new selected thread?  If so, then, the observer call should be
moved to the end of normal_stop after the "done:" label,
so any dummy frame is poped; if not, then I guess a comment here would
be good, as it seems something natural to be doing from inside
the observer in the future.

Also, it may make sense to add a "reason" parameter to
the observer, as in "changed due to user/frontend request", or
"due to a stop event", but that's not actually required right now.

-- 
Pedro Alves

Re: [patch:MI] Observer for thread-changed

[Nick Roberts]

Pedro Alves writes:
 > Sorry, if I missed the discussion on it, but,
 > 
 > A Monday 09 June 2008 13:16:09, Nick Roberts wrote:
 > >    annotate_thread_changed ();
 > >    gdb_thread_select (uiout, tidstr, NULL);
 > > +  observer_notify_thread_changed ();
 > >  }
 > 
 > This is conceptually not right. gdb_thread_select is a libgdb
 > function, that filters exceptions.  If do_captured_thread_select
 > throws an error, you will still call the observer.  Plus, 
 > do_captured_thread_select is already printing the thread change
 > to MI, which means you'll get the output twice now, in MI?

I don't think that's a problem.  Removing the output from -thread-select
would make it backwardly incompatible.

 > Why not call the observer from inside do_captured_thread_select,
 > instead of on both CLI and MI commands?

Yes, you're right.  Presumably I could also put it in a clause in
gdb_thread_select?  I did have it in do_captured_thread_select in the first
patch but I moved it.  I can't fully explain why now but I think I must have
got confused by output of the frame_changed observer, which was also part of
that patch, being triggered by "info threads".

I'll have to move the call to annotate_thread_changed too.

> > /* Print notices when new threads are attached and detached. */
> > Index: infrun.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/infrun.c,v
> > retrieving revision 1.278
> > diff -p -u -p -r1.278 infrun.c
> > --- infrun.c 6 Jun 2008 00:33:52 -0000 1.278
> > +++ infrun.c 9 Jun 2008 12:13:25 -0000
> > @@ -3605,6 +3605,7 @@ normal_stop (void)
> >      target_terminal_ours_for_output ();
> >      printf_filtered (_("[Switching to %s]\n"),
> >      target_pid_to_str (inferior_ptid));
> > +    observer_notify_thread_changed ();
> >      annotate_thread_changed ();
> >      previous_inferior_ptid = inferior_ptid;
> > }

> Hmm, will we want the observer to have access to the selected frame
> of the new selected thread?  If so, then, the observer call should be
> moved to the end of normal_stop after the "done:" label,
> so any dummy frame is poped; if not, then I guess a comment here would
> be good, as it seems something natural to be doing from inside
> the observer in the future.

Doesn't GDB already output the selected frame with "*stopped"

> Also, it may make sense to add a "reason" parameter to
> the observer, as in "changed due to user/frontend request", or
> "due to a stop event", but that's not actually required right now.

I'm not sure what use this information would be.  If it's due to a stop event
then the reason should be given in the async output.

How about the change below instead?  This, of course, requires no change to
mi-main.c.

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




--- thread.c	09 Jun 2008 21:06:46 +1200	1.71
+++ thread.c	10 Jun 2008 01:37:12 +1200	
@@ -738,8 +738,8 @@ thread_command (char *tidstr, int from_t
       return;
     }
 
-  annotate_thread_changed ();
   gdb_thread_select (uiout, tidstr, NULL);
+  observer_notify_thread_changed ();
 }
 
 /* Print notices when new threads are attached and detached.  */
@@ -786,8 +786,13 @@ gdb_thread_select (struct ui_out *uiout,
 {
   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;
+      return GDB_RC_FAIL;
+  else
+    {
+      observer_notify_thread_changed ();
+      annotate_thread_changed ();
+      return GDB_RC_OK;
+    }
 }
 
 /* Commands with a prefix of `thread'.  */

Re: [patch:MI] Observer for thread-changed

[Pedro Alves]

A Monday 09 June 2008 23:50:05, Nick Roberts wrote:
> Pedro Alves writes:
>  > Sorry, if I missed the discussion on it, but,
>  >
>  > A Monday 09 June 2008 13:16:09, Nick Roberts wrote:
>  > >    annotate_thread_changed ();
>  > >    gdb_thread_select (uiout, tidstr, NULL);
>  > > +  observer_notify_thread_changed ();
>  > >  }
>  >
>  > This is conceptually not right. gdb_thread_select is a libgdb
>  > function, that filters exceptions.  If do_captured_thread_select
>  > throws an error, you will still call the observer.  Plus,
>  > do_captured_thread_select is already printing the thread change
>  > to MI, which means you'll get the output twice now, in MI?
>
> I don't think that's a problem.  Removing the output from -thread-select
> would make it backwardly incompatible.

>  > Why not call the observer from inside do_captured_thread_select,
>  > instead of on both CLI and MI commands?
>
> Yes, you're right.  Presumably I could also put it in a clause in
> gdb_thread_select?  I did have it in do_captured_thread_select in the first
> patch but I moved it.  I can't fully explain why now but I think I must
> have got confused by output of the frame_changed observer, which was also
> part of that patch, being triggered by "info threads".
>

Right, and this happens due to the overload GDB makes on
internally selected thread/frame, and user selected thread/frame.
More on that Real Soon Now (TM)...  Stay tuned.

> > Also, it may make sense to add a "reason" parameter to
> > the observer, as in "changed due to user/frontend request", or
> > "due to a stop event", but that's not actually required right now.
>
> I'm not sure what use this information would be.  If it's due to a stop
> event then the reason should be given in the async output.

It all amounts to:

 - should there be an MI async event on -thread-select if the
   reply already carries that information?
 - if a command requires a synchronous reply, then it should be
   implemented in the command itself, not in an observer.

> How about the change below instead?  This, of course, requires no change to
> mi-main.c.

I'd really prefer to keep gdb_thread_select just an exception
wrapper, and do the observer call in do_captured_thread_select.

-- 
Pedro Alves

Re: [patch:MI] Observer for thread-changed

[Nick Roberts]

 > It all amounts to:
 > 
 >  - should there be an MI async event on -thread-select if the
 >    reply already carries that information?

But the CLI command "thread" doesn't.  I think MI should try to reflect the
state of GDB and the inferior.  It shouldn't really matter what commands were
used to put it in that state.

 >  - if a command requires a synchronous reply, then it should be
 >    implemented in the command itself, not in an observer.

Which commands require a synchronous reply?

 > > How about the change below instead?  This, of course, requires no change to
 > > mi-main.c.
 > 
 > I'd really prefer to keep gdb_thread_select just an exception
 > wrapper, and do the observer call in do_captured_thread_select.

If it goes at the end of do_captured_thread_select then I guess that will be
after any exceptions but, to me, putting the logic in gdb_thread_select makes
it clearer that the thread only gets reported when there is no exception.

As libgdb seems to be dead in the water (gdb_breakpoint in breakpoint.c
has gone altogether) do we need to be so precious about these function now?

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

Re: [patch:MI] Observer for thread-changed

[Pedro Alves]

A Tuesday 10 June 2008 03:03:46, Nick Roberts wrote:
>  > It all amounts to:
>  >
>  >  - should there be an MI async event on -thread-select if the
>  >    reply already carries that information?
>
> But the CLI command "thread" doesn't.  I think MI should try to reflect the
> state of GDB and the inferior.  It shouldn't really matter what commands
> were used to put it in that state.

I see.  I hadn't thought of CLI in MI.  Thanks.

> If it goes at the end of do_captured_thread_select then I guess that will
> be after any exceptions but, to me, putting the logic in gdb_thread_select
> makes it clearer that the thread only gets reported when there is no
> exception.

I think it's clearer to put the observer close to where to switch
is performed.  Plus, if/when we remove libgdb and the wrapper, we
have again to move the observer call... Not the end of the world, but
might as well put it in the right place now, IMHO.  Oh well, opinions :-)  

-- 
Pedro Alves

Re: [patch:MI] Observer for thread-changed

[Nick Roberts]

 > > If it goes at the end of do_captured_thread_select then I guess that will
 > > be after any exceptions but, to me, putting the logic in gdb_thread_select
 > > makes it clearer that the thread only gets reported when there is no
 > > exception.
 > 
 > I think it's clearer to put the observer close to where to switch
 > is performed.  Plus, if/when we remove libgdb and the wrapper, we
 > have again to move the observer call... Not the end of the world, but
 > might as well put it in the right place now, IMHO.  Oh well, opinions :-)  

Seems reasonable.  I'll do that.

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


--- thread.c	09 Jun 2008 21:06:46 +1200	1.71
+++ thread.c	10 Jun 2008 15:11:13 +1200	
@@ -738,7 +738,6 @@ thread_command (char *tidstr, int from_t
       return;
     }
 
-  annotate_thread_changed ();
   gdb_thread_select (uiout, tidstr, NULL);
 }
 
@@ -770,6 +769,8 @@ do_captured_thread_select (struct ui_out
     error (_("Thread ID %d has terminated."), num);
 
   switch_to_thread (tp->ptid);
+  observer_notify_thread_changed ();
+  annotate_thread_changed ();
 
   ui_out_text (uiout, "[Switching to thread ");
   ui_out_field_int (uiout, "new-thread-id", pid_to_thread_id (inferior_ptid));

Re: [patch:MI] Observer for thread-changed

[Vladimir Prus]

On Tuesday 10 June 2008 06:03:46 Nick Roberts wrote:
>  > It all amounts to:
>  > 
>  >  - should there be an MI async event on -thread-select if the
>  >    reply already carries that information?
> 
> But the CLI command "thread" doesn't.  I think MI should try to reflect the
> state of GDB and the inferior.  It shouldn't really matter what commands were
> used to put it in that state.

We had this discussion in context of breakpoint notifications already, but I
think the thread notification makes those issues easier to discuss.

What is the purpose of the thread change notification? I think the purpose is
to enable the frontend to notice when the thread change, unexpectedly. For
example, due to "thread" CLI command or user-defined command that switches
threads, or due to an otherwise magical way. The frontend, most likely, should
react by marking the thread reported as selected by GDB also selected in GUI.

The question, then, if whether -thread-select should output this notification?
Suppose a frontend uses -thread-select to get some data in some thread without
making it selected. Then, if a notification is emitted, the frontend has to 
take special care not to mark the thread as selected in GUI.

As an aside, this is similar to notifications/signals in GUI libraries -- for
example, line edit control often has 'text changed' signal. If this signal is
emitted even when the text is changed programmatically, the application 
often has to specially prevent signals emitted as result of programmatic change
to be handled as if it was the user input.

So, I think that -thread-select *should not* emit thread-changed notification.
With the original version of your patch, it would be a one-line change, it's
probably a bit harder with the last version.

- Volodya

Re: [patch:MI] Observer for thread-changed

[Vladimir Prus]

On Monday 09 June 2008 16:16:09 Nick Roberts wrote:
> 2008-06-09  Nick Roberts  <nickrob@snap.net.nz>
> 
>         * mi/mi-interp.c (mi_thread_changed): New static function.
>         (mi_interpreter_init): Register mi_thread_changed as thread_changed
>         observer .
>         * thread.c (thread_command): Notify thread changes using observer.
>         * infrun.c (normal_stop): Ditto.
>         * mi/mi-main.c (mi_cmd_thread_select): Ditto.
>         * Makefile.in (mi-main.o) : Add dependency on observer.h.
> 
> 2008-06-09  Nick Roberts  <nickrob@snap.net.nz>
> 
>         * observer.texi (GDB Observers): New observer for thread_changed.
> 
> 
> Index: thread.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/thread.c,v
> retrieving revision 1.71
> diff -p -u -p -r1.71 thread.c
> --- thread.c    6 Jun 2008 00:32:51 -0000       1.71
> +++ thread.c    9 Jun 2008 12:13:23 -0000
> @@ -740,6 +740,7 @@ thread_command (char *tidstr, int from_t
>  
>    annotate_thread_changed ();
>    gdb_thread_select (uiout, tidstr, NULL);
> +  observer_notify_thread_changed ();
>  }
>  
>  /* Print notices when new threads are attached and detached.  */
> Index: infrun.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/infrun.c,v
> retrieving revision 1.278
> diff -p -u -p -r1.278 infrun.c
> --- infrun.c    6 Jun 2008 00:33:52 -0000       1.278
> +++ infrun.c    9 Jun 2008 12:13:25 -0000
> @@ -3605,6 +3605,7 @@ normal_stop (void)
>        target_terminal_ours_for_output ();
>        printf_filtered (_("[Switching to %s]\n"),
>                        target_pid_to_str (inferior_ptid));
> +      observer_notify_thread_changed ();
>        annotate_thread_changed ();
>        previous_inferior_ptid = inferior_ptid;
>      }

Pedro's has asked if we'd want to call the observer only when we've
selected a frame in the new thread. Have you decided if that's a good
idea or not?

>  static void *
>  mi_interpreter_init (int top_level)
> @@ -92,6 +93,7 @@ mi_interpreter_init (int top_level)
>      {
>        observer_attach_new_thread (mi_new_thread);
>        observer_attach_thread_exit (mi_thread_exit);
> +      observer_attach_thread_changed (mi_thread_changed);
>      }
>  
>    return mi;
> @@ -331,6 +333,27 @@ mi_thread_exit (struct thread_info *t)
>    gdb_flush (mi->event_channel);
>  }
>  
> +static void
> +mi_thread_changed ()
> +{
> +  struct mi_interp *mi = top_level_interpreter_data ();
> +  struct interp *interp_to_use;
> +  struct ui_out *old_uiout, *temp_uiout;
> +  int version;
> +
> +  fprintf_unfiltered (mi->event_channel, "thread-changed");
> +  interp_to_use = top_level_interpreter ();
> +  old_uiout = uiout;
> +  temp_uiout = interp_ui_out (interp_to_use);
> +  version = mi_version (temp_uiout);
> +  temp_uiout = mi_out_new (version);
> +  uiout = temp_uiout;
> +  ui_out_field_int (uiout, "new-thread-id", pid_to_thread_id (inferior_ptid));
> +  mi_out_put (uiout, mi->event_channel);
> +  uiout = old_uiout;
> +  gdb_flush (mi->event_channel);
> +}

Since your other patches have similar code, how about introducing a helper
function for creating temporary uiout, as part of this patch?

> +
>  extern initialize_file_ftype _initialize_mi_interp; /* -Wmissing-prototypes */
>  
>  void
> Index: mi/mi-main.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/mi/mi-main.c,v
> retrieving revision 1.115
> diff -p -u -p -r1.115 mi-main.c
> --- mi/mi-main.c        6 May 2008 21:35:01 -0000       1.115
> +++ mi/mi-main.c        9 Jun 2008 12:13:28 -0000
> @@ -45,6 +45,7 @@
>  #include "frame.h"
>  #include "mi-main.h"
>  #include "language.h"
> +#include "observer.h"
>  
>  #include <ctype.h>
>  #include <sys/time.h>
> @@ -241,6 +242,7 @@ mi_cmd_thread_select (char *command, cha
>      error ("mi_cmd_thread_select: USAGE: threadnum.");
>  
>    rc = gdb_thread_select (uiout, argv[0], &mi_error_message);
> +  observer_notify_thread_changed ();

As I've explained in the other email, I think that thread-changed notification
should not be emitted for -thread-select, since the frontend does not need
notification about something it just explicitly did itself. So, this call
probably should disappear.

OK with the above changes.

Are you planning to write tests, and document the new notification?

- Volodya

Re: [patch:MI] Observer for thread-changed

[Nick Roberts]

 > > Index: infrun.c
 > > ===================================================================
 > > RCS file: /cvs/src/src/gdb/infrun.c,v
 > > retrieving revision 1.278
 > > diff -p -u -p -r1.278 infrun.c
 > > --- infrun.c    6 Jun 2008 00:33:52 -0000       1.278
 > > +++ infrun.c    9 Jun 2008 12:13:25 -0000
 > > @@ -3605,6 +3605,7 @@ normal_stop (void)
 > >        target_terminal_ours_for_output ();
 > >        printf_filtered (_("[Switching to %s]\n"),
 > >                        target_pid_to_str (inferior_ptid));
 > > +      observer_notify_thread_changed ();
 > >        annotate_thread_changed ();
 > >        previous_inferior_ptid = inferior_ptid;
 > >      }
 > 
 > Pedro's has asked if we'd want to call the observer only when we've
 > selected a frame in the new thread. Have you decided if that's a good
 > idea or not?

My observer doesn't use the selected frame, so I don't have an opinion.

 > > ...
 > > +static void
 > > +mi_thread_changed ()
 > > +{
 > > +  struct mi_interp *mi = top_level_interpreter_data ();
 > > +  struct interp *interp_to_use;
 > > +  struct ui_out *old_uiout, *temp_uiout;
 > > +  int version;
 > > +
 > > +  fprintf_unfiltered (mi->event_channel, "thread-changed");
 > > +  interp_to_use = top_level_interpreter ();
 > > +  old_uiout = uiout;
 > > +  temp_uiout = interp_ui_out (interp_to_use);
 > > +  version = mi_version (temp_uiout);
 > > +  temp_uiout = mi_out_new (version);
 > > +  uiout = temp_uiout;
 > > +  ui_out_field_int (uiout, "new-thread-id", pid_to_thread_id (inferior_ptid));
 > > +  mi_out_put (uiout, mi->event_channel);
 > > +  uiout = old_uiout;
 > > +  gdb_flush (mi->event_channel);
 > > +}
 > 
 > Since your other patches have similar code, how about introducing a helper
 > function for creating temporary uiout, as part of this patch?

Refactoring requires pointers to functions and function arguments and makes the
code harder to read.  I am happy to do this when there is more than one such
function.

 > > @@ -241,6 +242,7 @@ mi_cmd_thread_select (char *command, cha
 > >      error ("mi_cmd_thread_select: USAGE: threadnum.");
 > >  
 > >    rc = gdb_thread_select (uiout, argv[0], &mi_error_message);
 > > +  observer_notify_thread_changed ();
 > 
 > As I've explained in the other email, I think that thread-changed
 > notification should not be emitted for -thread-select, since the frontend
 > does not need notification about something it just explicitly did
 > itself. So, this call probably should disappear.

When Emacs migrates fully to MI I anticipate it would just process the
the notification and ignore the direct output from -thread-select.  That would
mean that -thread-select and the CLI "thread" command are processed the same
way.

 > OK with the above changes.
 > 
 > Are you planning to write tests, and document the new notification?

There certainly should be tests and documentation, and I will do write some,
but I don't see any for the existing notifications, namely "thread-created" and
"thread-exited".

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

Re: [patch:MI] Observer for thread-changed

[Nick Roberts]

 > The question, then, if whether -thread-select should output this
 > notification?  Suppose a frontend uses -thread-select to get some data in
 > some thread without making it selected. Then, if a notification is emitted,
 > the frontend has to take special care not to mark the thread as selected in
 > GUI.

You could say the same about user-defined functions that use the "threads"
command.

I don't think MI should be used as a programming language.  I don't think
-thread-select should be used by the front end except when the user explicitly
requests to change threads.  In fact, I would even suggest that there should be
no -thread-select and that all MI commands should be reflective and not change
the state of GDB or the inferior.  With a complete set of notifications,
the usual CLI commands could be used to change state and the front end could
just parse the MI output.

 > As an aside, this is similar to notifications/signals in GUI libraries --
 > for example, line edit control often has 'text changed' signal. If this
 > signal is emitted even when the text is changed programmatically, the
 > application often has to specially prevent signals emitted as result of
 > programmatic change to be handled as if it was the user input.
 > 
 > So, I think that -thread-select *should not* emit thread-changed
 > notification.  With the original version of your patch, it would be a
 > one-line change, it's probably a bit harder with the last version.

But the first patch was wrong for other reasons, as Pedro pointed out.

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

Re: [patch:MI] Observer for thread-changed

[Vladimir Prus]

On Tuesday 10 June 2008 12:25:02 Nick Roberts wrote:
>  > > Index: infrun.c
>  > > ===================================================================
>  > > RCS file: /cvs/src/src/gdb/infrun.c,v
>  > > retrieving revision 1.278
>  > > diff -p -u -p -r1.278 infrun.c
>  > > --- infrun.c    6 Jun 2008 00:33:52 -0000       1.278
>  > > +++ infrun.c    9 Jun 2008 12:13:25 -0000
>  > > @@ -3605,6 +3605,7 @@ normal_stop (void)
>  > >        target_terminal_ours_for_output ();
>  > >        printf_filtered (_("[Switching to %s]\n"),
>  > >                        target_pid_to_str (inferior_ptid));
>  > > +      observer_notify_thread_changed ();
>  > >        annotate_thread_changed ();
>  > >        previous_inferior_ptid = inferior_ptid;
>  > >      }
>  > 
>  > Pedro's has asked if we'd want to call the observer only when we've
>  > selected a frame in the new thread. Have you decided if that's a good
>  > idea or not?
> 
> My observer doesn't use the selected frame, so I don't have an opinion.

I don't have an opinion, either; guess what you have is fine.

>  > > ...
>  > > +static void
>  > > +mi_thread_changed ()
>  > > +{
>  > > +  struct mi_interp *mi = top_level_interpreter_data ();
>  > > +  struct interp *interp_to_use;
>  > > +  struct ui_out *old_uiout, *temp_uiout;
>  > > +  int version;
>  > > +
>  > > +  fprintf_unfiltered (mi->event_channel, "thread-changed");
>  > > +  interp_to_use = top_level_interpreter ();
>  > > +  old_uiout = uiout;
>  > > +  temp_uiout = interp_ui_out (interp_to_use);
>  > > +  version = mi_version (temp_uiout);
>  > > +  temp_uiout = mi_out_new (version);
>  > > +  uiout = temp_uiout;
>  > > +  ui_out_field_int (uiout, "new-thread-id", pid_to_thread_id (inferior_ptid));
>  > > +  mi_out_put (uiout, mi->event_channel);
>  > > +  uiout = old_uiout;
>  > > +  gdb_flush (mi->event_channel);
>  > > +}
>  > 
>  > Since your other patches have similar code, how about introducing a helper
>  > function for creating temporary uiout, as part of this patch?
> 
> Refactoring requires pointers to functions and function arguments and makes the
> code harder to read.  I am happy to do this when there is more than one such
> function.

Ok.

>  > > @@ -241,6 +242,7 @@ mi_cmd_thread_select (char *command, cha
>  > >      error ("mi_cmd_thread_select: USAGE: threadnum.");
>  > >  
>  > >    rc = gdb_thread_select (uiout, argv[0], &mi_error_message);
>  > > +  observer_notify_thread_changed ();
>  > 
>  > As I've explained in the other email, I think that thread-changed
>  > notification should not be emitted for -thread-select, since the frontend
>  > does not need notification about something it just explicitly did
>  > itself. So, this call probably should disappear.
> 
> When Emacs migrates fully to MI I anticipate it would just process the
> the notification and ignore the direct output from -thread-select.  That would
> mean that -thread-select and the CLI "thread" command are processed the same
> way.

Why would you care about direct output from -thread-select, anyway? It does not
say anything interesting, it merely confirms that GDB has switched to the thread
you told it to switch to. If GDB did not switch, you'd get ^error. So, I think there's
no need to either process direct output of -thread-select, or output any notification
for -thread-select.

>  > OK with the above changes.
>  > 
>  > Are you planning to write tests, and document the new notification?
> 
> There certainly should be tests and documentation, and I will do write some,
> but I don't see any for the existing notifications, namely "thread-created" and
> "thread-exited".

You probably missed those. Here's what my checkout of gdb.texinfo reads:

	@item =thread-created,id="@var{id}"
	@itemx =thread-exited,id="@var{id}"
	A thread either was created, or has exited.  The @var{id} field
	contains the @value{GDBN} identifier of the thread.

- Volodya

Re: [patch:MI] Observer for thread-changed

[Vladimir Prus]

On Tuesday 10 June 2008 12:39:49 you wrote:
>  > The question, then, if whether -thread-select should output this
>  > notification?  Suppose a frontend uses -thread-select to get some data in
>  > some thread without making it selected. Then, if a notification is emitted,
>  > the frontend has to take special care not to mark the thread as selected in
>  > GUI.
> 
> You could say the same about user-defined functions that use the "threads"
> command.

No, because user-defined functions are typed by the user and passed through to
GDB, and if those commands change thread, UI should update. On the contrary,
if the frontend sent -thread-select, it means it wanted to set GDB current
thread to be the same as the current thread presently shown in the UI, so
I see no point for frontend to be notified. Can you outline a use case where
frontend would actually like to be notified about the thing it just did?

> I don't think MI should be used as a programming language.  I don't think
> -thread-select should be used by the front end except when the user explicitly
> requests to change threads.  In fact, I would even suggest that there should be
> no -thread-select and that all MI commands should be reflective

What is "reflective"?

> and not change 
> the state of GDB or the inferior.  

Even though the patch that adds --thread option to all MI commands is almost ready,
I'd expect some frontends not to update for a while, and IIUC, Eclipse DSF is not
actually willing to update at all. So, frontends will be using -thread-select
for all purposes.

> With a complete set of notifications, 
> the usual CLI commands could be used to change state and the front end could
> just parse the MI output.

Could be used where? If in GDB console, then sure, but that does not require
that -thread-select output notification.

>  > As an aside, this is similar to notifications/signals in GUI libraries --
>  > for example, line edit control often has 'text changed' signal. If this
>  > signal is emitted even when the text is changed programmatically, the
>  > application often has to specially prevent signals emitted as result of
>  > programmatic change to be handled as if it was the user input.
>  > 
>  > So, I think that -thread-select *should not* emit thread-changed
>  > notification.  With the original version of your patch, it would be a
>  > one-line change, it's probably a bit harder with the last version.
> 
> But the first patch was wrong for other reasons, as Pedro pointed out.

IIUC, the primary objection was that we'd emit notification even if
gdb_thread_select caught an exception. Can we protect against this by
checking that inferior_ptid actually changed?

- Volodya

Re: [patch:MI] Observer for thread-changed

[Daniel Jacobowitz]

On Tue, Jun 10, 2008 at 10:27:38AM +0400, Vladimir Prus wrote:
> The question, then, if whether -thread-select should output this notification?
> Suppose a frontend uses -thread-select to get some data in some thread without
> making it selected. Then, if a notification is emitted, the frontend has to 
> take special care not to mark the thread as selected in GUI.
> 
> As an aside, this is similar to notifications/signals in GUI libraries -- for
> example, line edit control often has 'text changed' signal. If this signal is
> emitted even when the text is changed programmatically, the application 
> often has to specially prevent signals emitted as result of programmatic change
> to be handled as if it was the user input.
> 
> So, I think that -thread-select *should not* emit thread-changed notification.
> With the original version of your patch, it would be a one-line change, it's
> probably a bit harder with the last version.

One method I use is to ask myself how complicated the documentation
for something will be.  It's much clearer to say "the notification is
emitted whenever the active thread changes" than "... unless it
changed because of -thread-select".

Will detecting notifications due to its own commands be that
complicated for any IDE?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch:MI] Observer for thread-changed

[Nick Roberts]

 > >  > Are you planning to write tests, and document the new notification?
 > > 
 > > There certainly should be tests and documentation, and I will do write
 > > some, but I don't see any for the existing notifications, namely
 > > "thread-created" and "thread-exited".
 > 
 > You probably missed those. Here's what my checkout of gdb.texinfo reads:
 > 
 > 	@item =thread-created,id="@var{id}"
 > 	@itemx =thread-exited,id="@var{id}"
 > 	A thread either was created, or has exited.  The @var{id} field
 > 	contains the @value{GDBN} identifier of the thread.

OK.  Are there any tests?

I think that event notifications should have their own node.  I also think that
the manual should be restructured so that the node "GDB/MI Output Records"
comes _under_ the node "GDB/MI Command Syntax" and that the nodes in "GDB/MI
Output Records" are linked with their description in "GDB/MI Output Syntax"


 > > You could say the same about user-defined functions that use the "threads"
 > > command.

 > No, because user-defined functions are typed by the user and passed through
 > to GDB, and if those commands change thread, UI should update. On the
 > contrary, if the frontend sent -thread-select, it means it wanted to set GDB
 > current thread to be the same as the current thread presently shown in the
 > UI, so I see no point for frontend to be notified. Can you outline a use case
 > where frontend would actually like to be notified about the thing it just
 > did?

In my scenario the frontend would automatically display the selected thread,
not select the thread that is displayed.


 > > I don't think MI should be used as a programming language.  I don't think
 > > -thread-select should be used by the front end except when the user
 > > explicitly requests to change threads.  In fact, I would even suggest that
 > > there should be no -thread-select and that all MI commands should be
 > > reflective

 > What is "reflective"?

Maybe it's not the right word.  Perhaps I mean introspective, i.e., just
reports the state like -break-list or -environment-pwd and doesn't change it
like -thread-select or -stack-select-frame, for example.

 
> > With a complete set of notifications, the usual CLI commands could be used
 > > to change state and the front end could just parse the MI output.

 > Could be used where? If in GDB console, then sure, but that does not require
 > that -thread-select output notification.

I mean the front end could just use CLI commands to change the state provided
there were MI notifications that would report that change.


 > > But the first patch was wrong for other reasons, as Pedro pointed out.

 > IIUC, the primary objection was that we'd emit notification even if
 > gdb_thread_select caught an exception. Can we protect against this by
 > checking that inferior_ptid actually changed?

Pedro says he has a patch that splits the notion of user/frontend selected
thread and frame, from the internally selected thread and frame.  Let's wait
to see how that fits in.


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

Re: [patch:MI] Observer for thread-changed

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Wed, 11 Jun 2008 10:03:29 +1200
> Cc: gdb-patches@sources.redhat.com
> 
> I also think that
> the manual should be restructured so that the node "GDB/MI Output Records"
> comes _under_ the node "GDB/MI Command Syntax" and that the nodes in "GDB/MI
> Output Records" are linked with their description in "GDB/MI Output Syntax"

Sounds like a good idea.  Please suggest a patch and I will review it.

Thanks.

Re: [patch:MI] Observer for thread-changed

[Vladimir Prus]

On Tuesday 10 June 2008 17:25:12 Daniel Jacobowitz wrote:
> On Tue, Jun 10, 2008 at 10:27:38AM +0400, Vladimir Prus wrote:
> > The question, then, if whether -thread-select should output this notification?
> > Suppose a frontend uses -thread-select to get some data in some thread without
> > making it selected. Then, if a notification is emitted, the frontend has to 
> > take special care not to mark the thread as selected in GUI.
> > 
> > As an aside, this is similar to notifications/signals in GUI libraries -- for
> > example, line edit control often has 'text changed' signal. If this signal is
> > emitted even when the text is changed programmatically, the application 
> > often has to specially prevent signals emitted as result of programmatic change
> > to be handled as if it was the user input.
> > 
> > So, I think that -thread-select *should not* emit thread-changed notification.
> > With the original version of your patch, it would be a one-line change, it's
> > probably a bit harder with the last version.
> 
> One method I use is to ask myself how complicated the documentation
> for something will be.  It's much clearer to say "the notification is
> emitted whenever the active thread changes" than "... unless it
> changed because of -thread-select".

I'm afraid you are oversimplifying -- the "the notification is
emitted whenever the active thread changes" is very nice, but it's very
likely that every frontend author will not realize he has to ignore this
notification from -thread-select. So, the documentation should either say
that the notification is better ignored, or say that is not emitted. Now, what
is better?

I'd like this notification to communicate, from GDB to frontend, that GDB
has encountered a command that indicates explicit user desire to change
the thread he looks at, and that frontend typically should react by making
that thread selected in UI. 

Maybe, the notification should be called something like:

	=thread-selection-request

to indicate this better? Of course, we can also have another notification,
that is emitted each time inferior_ptid changes, but then the notification
should be named

       =inferior-ptid-changed

In other words, I argue for notification to be designed with the view of
what frontend is supposed to do with it, not with what internal detail of
GDB is been reported.


Alternatively, may I suggest we rename this notification? Let me be

   =user-has-explicitly-requested-that-the-current-thread-be-changed

In which case it's apparent 

> Will detecting notifications due to its own commands be that
> complicated for any IDE?

Probably not very complicated, but it's easier if a frontend should
not care about this.

- Volodya

Re: [patch:MI] Observer for thread-changed

[Tom Tromey]

Daniel> One method I use is to ask myself how complicated the documentation
Daniel> for something will be.  It's much clearer to say "the notification is
Daniel> emitted whenever the active thread changes" than "... unless it
Daniel> changed because of -thread-select".

Vladimir> I'm afraid you are oversimplifying -- the "the notification
Vladimir> is emitted whenever the active thread changes" is very nice,
Vladimir> but it's very likely that every frontend author will not
Vladimir> realize he has to ignore this notification from
Vladimir> -thread-select. So, the documentation should either say that
Vladimir> the notification is better ignored, or say that is not
Vladimir> emitted. Now, what is better?

I think that the Python layer would like to be notified of every
thread change.  That way, it will be possible to write Python
libraries which react to these kinds of events.

If the thread-changed observer is not always notified, is there a way
to do this?  I suppose we could introduce a second observer.

Would it be possible to isolate the "don't print some thread change"
logic into MI-specific code?  Say, by having MI attach to a generic
observer and then filter out the notifications it does not want to
report?

Tom

Re: [patch:MI] Observer for thread-changed

[Bob Rossi]

On Sat, Jun 14, 2008 at 12:14:43PM -0600, Tom Tromey wrote:
> Daniel> One method I use is to ask myself how complicated the documentation
> Daniel> for something will be.  It's much clearer to say "the notification is
> Daniel> emitted whenever the active thread changes" than "... unless it
> Daniel> changed because of -thread-select".
> 
> Vladimir> I'm afraid you are oversimplifying -- the "the notification
> Vladimir> is emitted whenever the active thread changes" is very nice,
> Vladimir> but it's very likely that every frontend author will not
> Vladimir> realize he has to ignore this notification from
> Vladimir> -thread-select. So, the documentation should either say that
> Vladimir> the notification is better ignored, or say that is not
> Vladimir> emitted. Now, what is better?
> 
> I think that the Python layer would like to be notified of every
> thread change.  That way, it will be possible to write Python
> libraries which react to these kinds of events.

I can tell you this. The annotate-1 interface used to send out things
like breakpoints-changed whenever something happened that triggered a
breakpoint change. This data would come out so fast, it would effect the
performance of the front end from getting meaningful data.

I'm just giving a little advice, make sure that gdb doesn't auto send
out information if the inferior can manipulate itself in such a way that
it would effectively flood the communication between gdb and the front
end.

Can thread changes happen often? how often? how will this effect the
communication load?

Bob Rossi

Re: [patch:MI] Observer for thread-changed

[Daniel Jacobowitz]

On Sat, Jun 14, 2008 at 07:09:37PM +0400, Vladimir Prus wrote:
> I'm afraid you are oversimplifying -- the "the notification is
> emitted whenever the active thread changes" is very nice, but it's very
> likely that every frontend author will not realize he has to ignore this
> notification from -thread-select. So, the documentation should either say
> that the notification is better ignored, or say that is not emitted. Now, what
> is better?

Seems clear to me: it should be in the example output of
-thread-select, and a reference to there from the description of the
event.

> I'd like this notification to communicate, from GDB to frontend, that GDB
> has encountered a command that indicates explicit user desire to change
> the thread he looks at, and that frontend typically should react by making
> that thread selected in UI. 

Wouldn't it make sense to use the same notification if the inferior
stopping causes the selected thread to change, which is how things
work today in all-stop?

> In other words, I argue for notification to be designed with the view of
> what frontend is supposed to do with it, not with what internal detail of
> GDB is been reported.

This is a good principle, but it's not right either.  Reporting the
internal state of GDB is bad design, but reporting based on what
frontends are supposed to do is also bad design: it assumes that you
can think of everything a frontend might want to do.  We need to
report logical interface events based on GDB's state.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [patch:MI] Observer for thread-changed

[Vladimir Prus]

On Saturday 14 June 2008 22:14:43 Tom Tromey wrote:
> Daniel> One method I use is to ask myself how complicated the documentation
> Daniel> for something will be.  It's much clearer to say "the notification is
> Daniel> emitted whenever the active thread changes" than "... unless it
> Daniel> changed because of -thread-select".
> 
> Vladimir> I'm afraid you are oversimplifying -- the "the notification
> Vladimir> is emitted whenever the active thread changes" is very nice,
> Vladimir> but it's very likely that every frontend author will not
> Vladimir> realize he has to ignore this notification from
> Vladimir> -thread-select. So, the documentation should either say that
> Vladimir> the notification is better ignored, or say that is not
> Vladimir> emitted. Now, what is better?
> 
> I think that the Python layer would like to be notified of every
> thread change.  That way, it will be possible to write Python
> libraries which react to these kinds of events.

What's "these kinds of events"? Are you interested in any change in
inferiour_ptid? Or in any change of user's idea of what thread he
wants to look at? Pedro's a patch to make those different.

> If the thread-changed observer is not always notified, is there a way
> to do this?  I suppose we could introduce a second observer.
> 
> Would it be possible to isolate the "don't print some thread change"
> logic into MI-specific code?  Say, by having MI attach to a generic
> observer and then filter out the notifications it does not want to
> report?

I think the above question is most important to answer, but we can surely
make the filtering happen in MI code, if Python wants to see all changes.

- Volodya

Re: [patch:MI] Observer for thread-changed

[Nick Roberts]

 > > In other words, I argue for notification to be designed with the view of
 > > what frontend is supposed to do with it, not with what internal detail of
 > > GDB is been reported.
 > 
 > This is a good principle, but it's not right either.  Reporting the
 > internal state of GDB is bad design, but reporting based on what
 > frontends are supposed to do is also bad design: it assumes that you
 > can think of everything a frontend might want to do.  We need to
 > report logical interface events based on GDB's state.

I agree.  I don't think that we should second guess what front ends will do.  I
think the role of MI is to provide a mechanism to report the state of GDB and
the inferior, not to provide a policy.  The front end developer can then filter
out information that he doesn't need.  However he can't factor in information
that GDB developers leave out because they consider it's not needed.

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

Re: [patch:MI] Observer for thread-changed

[Nick Roberts]

 > > I think that the Python layer would like to be notified of every
 > > thread change.  That way, it will be possible to write Python
 > > libraries which react to these kinds of events.
 > 
 > I can tell you this. The annotate-1 interface used to send out things
 > like breakpoints-changed whenever something happened that triggered a
 > breakpoint change. This data would come out so fast, it would effect the
 > performance of the front end from getting meaningful data.

You mean level 2 annotations?  Level 1 annotations is just --fullname which
only emits the one (source) annotation.  Yes, it does fire too frequently.  I
don't know why, but I think it fires when the breakpoints are removed and
re-inserted each time execution stops.

 > I'm just giving a little advice, make sure that gdb doesn't auto send
 > out information if the inferior can manipulate itself in such a way that
 > it would effectively flood the communication between gdb and the front
 > end.
 > 
 > Can thread changes happen often? how often? how will this effect the
 > communication load?

I think it can only change, at most, once every time execution stops and
when the user selects a new thread.  So the load is very light.

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

Re: [patch:MI] Observer for thread-changed

[Vladimir Prus]

On Saturday 14 June 2008 23:13:28 Daniel Jacobowitz wrote:
> On Sat, Jun 14, 2008 at 07:09:37PM +0400, Vladimir Prus wrote:
> > I'm afraid you are oversimplifying -- the "the notification is
> > emitted whenever the active thread changes" is very nice, but it's very
> > likely that every frontend author will not realize he has to ignore this
> > notification from -thread-select. So, the documentation should either say
> > that the notification is better ignored, or say that is not emitted. Now, what
> > is better?
> 
> Seems clear to me: it should be in the example output of
> -thread-select, and a reference to there from the description of the
> event.

Which would nicely eliminate any difference in documentation complexity you
originally presented as argument.

> > I'd like this notification to communicate, from GDB to frontend, that GDB
> > has encountered a command that indicates explicit user desire to change
> > the thread he looks at, and that frontend typically should react by making
> > that thread selected in UI. 
> 
> Wouldn't it make sense to use the same notification if the inferior
> stopping causes the selected thread to change, which is how things
> work today in all-stop?

It would. Why GDB changes the selected thread on stop, in the first place? Presumably,
because it thinks that user really want to look at this thread. Then, emitting a
notification suggesting that the frontend shows that thread, makes perfect sense.

> > In other words, I argue for notification to be designed with the view of
> > what frontend is supposed to do with it, not with what internal detail of
> > GDB is been reported.
> 
> This is a good principle, but it's not right either.  Reporting the
> internal state of GDB is bad design, but reporting based on what
> frontends are supposed to do is also bad design: it assumes that you
> can think of everything a frontend might want to do.  We need to
> report logical interface events based on GDB's state.

Oh, we're in too abstract fields now :-) How are you planning to design
logical interface without any consideration what frontend needs, and without
necessary limiting what frontend may be able to do? For all new MI features, 
it's necessary to start with what current frontends need, and build interface
as reasonable generalization of that cases. And, each MI interface,
especially notifications, *must* come with exact documentation of semantics
for frontend. Really, saying that there's current thread and whenever it changes,
frontend is notified, does not actually tell frontend enough to accurately handle
anything.

In light of that, I've started writing a spec about how MI handles threads and
frames, including in non-stop mode. I'm gonna post that soonish, and hopefully
we'll get more concrete discussion.

- Volodya

Re: [patch:MI] Observer for thread-changed

[Vladimir Prus]

On Sunday 15 June 2008 02:04:35 you wrote:
>  > > In other words, I argue for notification to be designed with the view of
>  > > what frontend is supposed to do with it, not with what internal detail of
>  > > GDB is been reported.
>  > 
>  > This is a good principle, but it's not right either.  Reporting the
>  > internal state of GDB is bad design, but reporting based on what
>  > frontends are supposed to do is also bad design: it assumes that you
>  > can think of everything a frontend might want to do.  We need to
>  > report logical interface events based on GDB's state.
> 
> I agree.  I don't think that we should second guess what front ends will do.  

We should, or frontends will second guess what MI tells them. "Current thread"
is not a exact thing, and "current thread changed" is not an exact thing either,
so we should provide specific meaning that is most useful to frontends, and opposed
to providing a meaning that is most easy for gdb.

> I 
> think the role of MI is to provide a mechanism to report the state of GDB and 
> the inferior, not to provide a policy.  The front end developer can then filter
> out information that he doesn't need.  However he can't factor in information
> that GDB developers leave out because they consider it's not needed.

I'd say that gdb already provides sufficient information in response to -thread-select,
namely either ^done or ^error. Unless GDB has a bug, the output of ^done means that
the thread has changed.

- Volodya

Re: [patch:MI] Observer for thread-changed

[Tom Tromey]

>>>>> "Vladimir" == Vladimir Prus <ghost@cs.msu.su> writes:

Vladimir> What's "these kinds of events"? Are you interested in any change in
Vladimir> inferiour_ptid? Or in any change of user's idea of what thread he
Vladimir> wants to look at? Pedro's a patch to make those different.

Yeah, I'm not sure.  I don't actually have a use case (other than the
obvious one of rewriting Insight in Python ... but that one probably
has needs similar to an MI client).  Instead I'm mostly interested in
transmitting gdb's state and state transitions to Python, so that
users can write interesting apps.

Maybe this is too general an approach given the maturity of the Python
branch.

Tom

Re: [patch:MI] Observer for thread-changed

[Nick Roberts]

 > > I agree.  I don't think that we should second guess what front ends will
 > > do.
 > 
 > We should, or frontends will second guess what MI tells them.

If the behaviour of MI is fully documented the frontend won't need to do
guessing of any kind.

 >                                                                "Current
 > thread" is not a exact thing, and "current thread changed" is not an exact
 > thing either, so we should provide specific meaning that is most useful to
 > frontends, and opposed to providing a meaning that is most easy for gdb.

I know that Pedro has a patch that might change things but what different
meanings does "Current thread" have now.  It seems pretty unambiguous to me:
it's the value stored inferior_ptid and the thread that has a "*" next to it
in the output of "info threads".

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

Re: [patch:MI] Observer for thread-changed

[Daniel Jacobowitz]

On Sun, Jun 15, 2008 at 09:42:35PM +0400, Vladimir Prus wrote:
> We should, or frontends will second guess what MI tells
> them. "Current thread" is not a exact thing, and "current thread
> changed" is not an exact thing either, so we should provide specific
> meaning that is most useful to frontends, and opposed to providing a
> meaning that is most easy for gdb.

This is true.  But shouldn't we err on the side of providing too many
notifications, not too few?  I can easily see this: a front end whose
state changes are driven off observer responses, and the author
complaining /* Synthesize =thread-changed since GDB doesn't notify
after -thread-select */.

Anyway it's clear we're not getting anywhere... since you prefer it,
I'm ok with "whenever the thread changes, except in response to
explicit -thread-select".  Let's move on.

-- 
Daniel Jacobowitz
CodeSourcery

[PATCH]:annotations [was Re: [patch:MI] Observer for thread-changed]

[Nick Roberts]

On 10 Jun 2008 15:12:46 +1200 Nick Roberts writes:

 >  > > If it goes at the end of do_captured_thread_select then I guess that
 >  > > will be after any exceptions but, to me, putting the logic in
 >  > > gdb_thread_select makes it clearer that the thread only gets reported
 >  > > when there is no exception.
 >  > 
 >  > I think it's clearer to put the observer close to where to switch is
 >  > performed.  Plus, if/when we remove libgdb and the wrapper, we have again
 >  > to move the observer call... Not the end of the world, but might as well
 >  > put it in the right place now, IMHO.  Oh well, opinions :-)
 > 
 > Seems reasonable.  I'll do that.

Ping.

 > -- 
 > Nick                                           http://www.inet.net.nz/~nickrob
 > 
 > 
 > --- thread.c	09 Jun 2008 21:06:46 +1200	1.71
 > +++ thread.c	10 Jun 2008 15:11:13 +1200	
 > @@ -738,7 +738,6 @@ thread_command (char *tidstr, int from_t
 >        return;
 >      }
 >  
 > -  annotate_thread_changed ();
 >    gdb_thread_select (uiout, tidstr, NULL);
 >  }
 >  
 > @@ -770,6 +769,8 @@ do_captured_thread_select (struct ui_out
 >      error (_("Thread ID %d has terminated."), num);
 >  
 >    switch_to_thread (tp->ptid);
 > +  observer_notify_thread_changed ();
 > +  annotate_thread_changed ();
 >  
 >    ui_out_text (uiout, "[Switching to thread ");
 >    ui_out_field_int (uiout, "new-thread-id", pid_to_thread_id (inferior_ptid));
 > 

Vladimir has created his own notifications for thread changes now, so that
part is no longer relevant but moving the call to annotate_thread_changed
still is.

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


2009-01-17  Nick Roberts  <nickrob@snap.net.nz>

	* thread.c (thread_command): Don't print an annotation if the
          thread change generate an exception.



Index: thread.c
===================================================================
RCS file: /cvs/src/src/gdb/thread.c,v
retrieving revision 1.99
diff -p -u -p -r1.99 thread.c
--- thread.c	3 Jan 2009 05:57:53 -0000	1.99
+++ thread.c	16 Jan 2009 23:58:47 -0000
@@ -1045,7 +1045,6 @@ thread_command (char *tidstr, int from_t
       return;
     }
 
-  annotate_thread_changed ();
   gdb_thread_select (uiout, tidstr, NULL);
 }
 
@@ -1078,6 +1077,8 @@ do_captured_thread_select (struct ui_out
 
   switch_to_thread (tp->ptid);
 
+  annotate_thread_changed ();
+
   ui_out_text (uiout, "[Switching to thread ");
   ui_out_field_int (uiout, "new-thread-id", pid_to_thread_id (inferior_ptid));
   ui_out_text (uiout, " (");

Re: [PATCH]:annotations

[Tom Tromey]

>>>>> "Nick" == Nick Roberts <nickrob@snap.net.nz> writes:

Nick> 2009-01-17  Nick Roberts  <nickrob@snap.net.nz>
Nick> 	* thread.c (thread_command): Don't print an annotation if the
Nick>           thread change generate an exception.

This is ok.

thanks,
Tom