From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5941 invoked by alias); 4 Jun 2008 08:34:49 -0000 Received: (qmail 5924 invoked by uid 22791); 4 Jun 2008 08:34:48 -0000 X-Spam-Check-By: sourceware.org Received: from viper.snap.net.nz (HELO viper.snap.net.nz) (202.37.101.25) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 04 Jun 2008 08:34:23 +0000 Received: from kahikatea.snap.net.nz (152.62.255.123.dynamic.snap.net.nz [123.255.62.152]) by viper.snap.net.nz (Postfix) with ESMTP id DF6E63D9FC4; Wed, 4 Jun 2008 20:34:19 +1200 (NZST) Received: by kahikatea.snap.net.nz (Postfix, from userid 1000) id 333628FC6D; Wed, 4 Jun 2008 20:34:09 +1200 (NZST) From: Nick Roberts MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable Message-ID: <18502.21366.451562.387913@kahikatea.snap.net.nz> Date: Wed, 04 Jun 2008 08:34:00 -0000 To: Vladimir Prus Cc: gdb-patches@sourceware.org Subject: Re: [PATCH:MI] Use observers for breakpoints In-Reply-To: <200806041112.33746.ghost@cs.msu.su> References: <18498.3436.91443.361769@kahikatea.snap.net.nz> <200806041112.33746.ghost@cs.msu.su> X-Mailer: VM 7.19 under Emacs 22.2.50.2 X-IsSubscribed: yes 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/msg00020.txt.bz2 Vladimir Prus writes: > On Sunday 01 June 2008 06:46:04 Nick Roberts wrote: >=20 > > (gdb) > > -break-watch i22 > > =3Dbreakpoints-changed,bkpt=3D{number=3D"4",type=3D"watchpoint",disp= =3D"keep",e > >nabled=3D"y",addr=3D"",what=3D"i22",times=3D"0",original-location=3D"i2= 2"} > > ^done,wpt=3D{number=3D"4",exp=3D"i22"} > > Is this an output we actually want? ;-) > With one breakpoint mentioned twice? I don't know why watchpoints are currently reported in a different way to normal breakpoints or why type=3D"watchpoint" isn't adequate and a special field "wpt" is needed, but I would suggest removing the latter output. > I wonder if you missed this bit in bpstat_stop_status: >=20 > if (b->disposition =3D=3D disp_disable) > { > b->enable_state =3D bp_disabled; > update_global_location_list (); > } Could be. At the moment, as I seaid to Joel, it's just a sketch. I just u= sed the locations in breakpoint.c where the event functions were called. If new locations have been created since they were added then I will have missed t= hem. How does the breakpoint list change at this point? > Probably, breakpoint_re_set_one should call the observer too. >=20 > In fact, it's probably would be nice to add call to the observer to > update_global_location_list. This function is called whenever we change = any > property of a breakpoint that affects what should be inserted in the > target. The only issue is that right now update_global_location_list does > not know which breakpoint has changed -- we probably can add a parameter. >=20 > This won't catch all cases -- there are properties of breakpoints that a= re > not reflected in the target -- like condition, commands and ignore > count. But seems like your patch has it covered already. So is update_global_location_list just updating the target, i.e. putting new breakpoints in, old ones back etc? If so, perhaps changes in the breakpoint list have already been reflected elsewhere and no observer calls need to be made here. > Is there any way to make -break-insert report the new breakpoint directl= y, > as opposed to via notification? That's what it currently does, isn't it? I'm trying to move away from that so that GDB just reports changes no matter how they are made, e.g., MI comm= and or CLI command. > ... > > +static void > > +mi_breakpoints_changed (int bnum) > > +{ > > + =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, "breakpoints-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; > > + =A0breakpoint_query (bnum); >=20 > Shall we have a helper function to do this creation of temporary uiout? The thread-changed observer in my earlier patch uses a very similar function so there probably could be some refactoring. =20 > I think that except for compatibility issues due to -break-insert no lon= ger > reporting the breakpoint that is created as part of ^done response, this > patch is good. The only way I can see around compatibility issues would be to add a new le= vel for MI. This change could be lumped with others that are being proposed, e.g, the no stop mode. Then the logic could be separated according to MI level and notice given that at some stage the old level would be removed. It's a lot of work and I must admit I'm not really sure that I have the sta= mina to go through with all of it. --=20 Nick http://www.inet.net.nz/~nick= rob