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

[Nick Roberts <nickrob at snap dot net dot nz>]

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