From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 26181 invoked by alias); 10 Jun 2008 09:19:48 -0000 Received: (qmail 26173 invoked by uid 22791); 10 Jun 2008 09:19:46 -0000 X-Spam-Check-By: sourceware.org Received: from zigzag.lvk.cs.msu.su (HELO zigzag.lvk.cs.msu.su) (158.250.17.23) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 10 Jun 2008 09:19:29 +0000 Received: from Debian-exim by zigzag.lvk.cs.msu.su with spam-scanned (Exim 4.63) (envelope-from ) id 1K600f-0001L0-8T for gdb-patches@sources.redhat.com; Tue, 10 Jun 2008 13:19:25 +0400 Received: from localhost ([127.0.0.1] helo=ip6-localhost) by zigzag.lvk.cs.msu.su with esmtps (TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.63) (envelope-from ) id 1K600Y-0001Kf-Fn; Tue, 10 Jun 2008 13:19:14 +0400 From: Vladimir Prus To: Nick Roberts Subject: Re: [patch:MI] Observer for thread-changed Date: Tue, 10 Jun 2008 09:36:00 -0000 User-Agent: KMail/1.9.9 Cc: gdb-patches@sources.redhat.com References: <18509.7945.19078.399646@kahikatea.snap.net.nz> <200806101057.19272.ghost@cs.msu.su> <18510.14942.143700.410083@kahikatea.snap.net.nz> In-Reply-To: <18510.14942.143700.410083@kahikatea.snap.net.nz> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline Message-Id: <200806101232.37186.ghost@cs.msu.su> Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-06/txt/msg00195.txt.bz2 On Tuesday 10 June 2008 12:25:02 Nick Roberts wrote: > > > Index: infrun.c > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > > RCS file: /cvs/src/src/gdb/infrun.c,v > > > retrieving revision 1.278 > > > diff -p -u -p -r1.278 infrun.c > > > --- infrun.c=A0=A0=A0=A06 Jun 2008 00:33:52 -0000=A0=A0=A0=A0=A0=A0= =A01.278 > > > +++ infrun.c=A0=A0=A0=A09 Jun 2008 12:13:25 -0000 > > > @@ -3605,6 +3605,7 @@ normal_stop (void) > > > =A0 =A0 =A0 =A0target_terminal_ours_for_output (); > > > =A0 =A0 =A0 =A0printf_filtered (_("[Switching to %s]\n"), > > > =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 =A0 =A0 =A0 target_= pid_to_str (inferior_ptid)); > > > + =A0 =A0 =A0observer_notify_thread_changed (); > > > =A0 =A0 =A0 =A0annotate_thread_changed (); > > > =A0 =A0 =A0 =A0previous_inferior_ptid =3D inferior_ptid; > > > =A0 =A0 =A0} > >=20 > > 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? >=20 > 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 () > > > +{ > > > + =A0struct mi_interp *mi =3D top_level_interpreter_data (); > > > + =A0struct interp *interp_to_use; > > > + =A0struct ui_out *old_uiout, *temp_uiout; > > > + =A0int version; > > > + > > > + =A0fprintf_unfiltered (mi->event_channel, "thread-changed"); > > > + =A0interp_to_use =3D top_level_interpreter (); > > > + =A0old_uiout =3D uiout; > > > + =A0temp_uiout =3D interp_ui_out (interp_to_use); > > > + =A0version =3D mi_version (temp_uiout); > > > + =A0temp_uiout =3D mi_out_new (version); > > > + =A0uiout =3D temp_uiout; > > > + =A0ui_out_field_int (uiout, "new-thread-id", pid_to_thread_id (inf= erior_ptid)); > > > + =A0mi_out_put (uiout, mi->event_channel); > > > + =A0uiout =3D old_uiout; > > > + =A0gdb_flush (mi->event_channel); > > > +} > >=20 > > Since your other patches have similar code, how about introducing a he= lper > > function for creating temporary uiout, as part of this patch? >=20 > Refactoring requires pointers to functions and function arguments and mak= es the > code harder to read. I am happy to do this when there is more than one s= uch > function. Ok. > > > @@ -241,6 +242,7 @@ mi_cmd_thread_select (char *command, cha > > > =A0 =A0 =A0error ("mi_cmd_thread_select: USAGE: threadnum."); > > > =A0 > > > =A0 =A0rc =3D gdb_thread_select (uiout, argv[0], &mi_error_message); > > > + =A0observer_notify_thread_changed (); > >=20 > > As I've explained in the other email, I think that thread-changed > > notification should not be emitted for -thread-select, since the front= end > > does not need notification about something it just explicitly did > > itself. So, this call probably should disappear. >=20 > 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 s= ame > 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 t= hread you told it to switch to. If GDB did not switch, you'd get ^error. So, I th= ink there's no need to either process direct output of -thread-select, or output any no= tification for -thread-select. > > OK with the above changes. > >=20 > > Are you planning to write tests, and document the new notification? >=20 > There certainly should be tests and documentation, and I will do write so= me, > but I don't see any for the existing notifications, namely "thread-create= d" and > "thread-exited". You probably missed those. Here's what my checkout of gdb.texinfo reads: @item =3Dthread-created,id=3D"@var{id}" @itemx =3Dthread-exited,id=3D"@var{id}" A thread either was created, or has exited. The @var{id} field contains the @value{GDBN} identifier of the thread. - Volodya