Re: [PATCH:MI] Use observers for breakpoints

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

Vladimir Prus writes:
 > On Sunday 01 June 2008 06:46:04 Nick Roberts wrote:
 > 
 > > (gdb)
 > > -break-watch i22
 > > =breakpoints-changed,bkpt={number="4",type="watchpoint",disp="keep",e
 > >nabled="y",addr="",what="i22",times="0",original-location="i22"}
 > > ^done,wpt={number="4",exp="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="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:
 > 
 > 	if (b->disposition == disp_disable)
 > 	  {
 > 	    b->enable_state = bp_disabled;
 > 	    update_global_location_list ();
 > 	  }

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.

How does the breakpoint list change at this point?


 > Probably, breakpoint_re_set_one should call the observer too.
 > 
 > 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.
 > 
 > 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.

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 directly,
 > 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 command
or CLI command.


 > ...
 > > +static void
 > > +mi_breakpoints_changed (int bnum)
 > > +{
 > > + ?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, "breakpoints-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;
 > > + ?breakpoint_query (bnum);
 > 
 > 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.

 
 > I think that except for compatibility issues due to -break-insert no longer
 > 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 level
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 stamina
to go through with all of it.

-- 
Nick                                           http://www.inet.net.nz/~nickrob