From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 14969 invoked by alias); 6 Jun 2008 05:08:35 -0000 Received: (qmail 14954 invoked by uid 22791); 6 Jun 2008 05:08:34 -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; Fri, 06 Jun 2008 05:08:16 +0000 Received: from Debian-exim by zigzag.lvk.cs.msu.su with spam-scanned (Exim 4.63) (envelope-from ) id 1K4UBO-0008Tj-2Y for gdb-patches@sources.redhat.com; Fri, 06 Jun 2008 09:08:13 +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 1K4UBG-0008TW-SG; Fri, 06 Jun 2008 09:08:03 +0400 From: Vladimir Prus To: Nick Roberts , gdb-patches@sources.redhat.com Subject: Re: [PATCH:MI] Use observers for breakpoints Date: Fri, 06 Jun 2008 05:08:00 -0000 User-Agent: KMail/1.9.9 References: <18498.3436.91443.361769@kahikatea.snap.net.nz> <200806041112.33746.ghost@cs.msu.su> <18502.21366.451562.387913@kahikatea.snap.net.nz> In-Reply-To: <18502.21366.451562.387913@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: <200806060907.36539.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/msg00102.txt.bz2 On Wednesday 04 June 2008 12:33:58 you wrote: > 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"= i22"} > > > ^done,wpt=3D{number=3D"4",exp=3D"i22"} > > > > Is this an output we actually want? ;-) > > With one breakpoint mentioned twice? >=20 > 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. Good. You did not mention this initially, so I wondered if you consider this OK. The 'wpt' comes from the mention function. > > 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 (); > > } >=20 > Could be. At the moment, as I seaid to Joel, it's just a sketch. I just= used > 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= them. >=20 > How does the breakpoint list change at this point? One breakpoint becomes disabled. > > 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 chang= e 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 d= oes > > not know which breakpoint has changed -- we probably can add a paramet= er. > >=20 > > This won't catch all cases -- there are properties of breakpoints that= are > > not reflected in the target -- like condition, commands and ignore > > count. But seems like your patch has it covered already. >=20 > 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 breakpo= int > list have already been reflected elsewhere and no observer calls need to = be > made here. Depends on your design. I think it makes perfect sense to report changes in= properties that affect inserted breakpoints in update_global_location_list, and report= changes in other properties in the corresponding functions (condition_command, etc). > > Is there any way to make -break-insert report the new breakpoint direc= tly, > > as opposed to via notification? >=20 > That's what it currently does, isn't it? I'm trying to move away from th= at > so that GDB just reports changes no matter how they are made, e.g., MI co= mmand > or CLI command. I think that for -break-insert, saying "^done" and then saying, "ah, btw, s= ome breakpoints were inserted", is a bit indirect. For example, in KDevelop, th= ere's a class to represent breakpoint. When a breakpoint is created, and -break-i= nsert is sent, and breakpoint is inserted, the 'id' field of that class is set to= breakpoint number that gdb reports, and at this point we know that the breakpoint is c= orrectly inserted. If -break-insert no longer reports the breakpoint directly, I'd h= ave to somehow match the output =3Dbreakpoints-changes to the previous -break-inse= rt. If I don't, then =3Dbreakpoints-changed will actually add new breakpoint to the = breakpoint table, without marking the breakpoint been inserted via -break-insert as in= serted. One can argue that such matching is not very hard to do. But if so, then it= 's not hard to do for GDB, either, and -break-insert can still report the brea= kpoint that was inserted. I'm also not sure that emitting =3Dbreakpoints-changes in response to -brea= k-condition, or -break-disable, is a good idea. It does not add any new information. Presumably, we can suppress the breakpoint observer during executing of MI = commands that directly create/modify breakpoint, and leave the observer enabled for = other commands -- where we don't mean no breakpoint changes but might get them. > > ... > > > +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? >=20 > The thread-changed observer in my earlier patch uses a very similar funct= ion > so there probably could be some refactoring. OK. > > I think that except for compatibility issues due to -break-insert no l= onger > > reporting the breakpoint that is created as part of ^done response, th= is > > patch is good. >=20 > The only way I can see around compatibility issues would be to add a new = level > for MI.=20=20 Or make -break-insert output the new breakpoint directly, by temporary supp= ressing the observer. > This change could be lumped with others that are being proposed,=20 > e.g, the no stop mode.=20=20 Just to clarify -- the non stop mode is optional, and so does not introduce= any backward compatibility issues. > Then the logic could be separated according to MI=20 > level and notice given that at some stage the old level would be removed. >=20 > It's a lot of work and I must admit I'm not really sure that I have the s= tamina > to go through with all of it. It seems to me that this patch, with minor adjustments, can be checked in s= oonish. - Volodya