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

[Vladimir Prus <ghost at cs dot msu dot su>]

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