This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH:MI] Use observers for breakpoints
On Wednesday 04 June 2008 12:33:58 you wrote:
> 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.
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:
> >
> > 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?
One breakpoint becomes disabled.
> > 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.
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 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.
I think that for -break-insert, saying "^done" and then saying, "ah, btw, some
breakpoints were inserted", is a bit indirect. For example, in KDevelop, there's
a class to represent breakpoint. When a breakpoint is created, and -break-insert
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 correctly
inserted. If -break-insert no longer reports the breakpoint directly, I'd have to
somehow match the output =breakpoints-changes to the previous -break-insert. If I
don't, then =breakpoints-changed will actually add new breakpoint to the breakpoint
table, without marking the breakpoint been inserted via -break-insert as inserted.
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 breakpoint
that was inserted.
I'm also not sure that emitting =breakpoints-changes in response to -break-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)
> > > +{
> > > + ?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.
OK.
> > 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.
Or make -break-insert output the new breakpoint directly, by temporary suppressing
the observer.
> This change could be lumped with others that are being proposed,
> e.g, the no stop mode.
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
> 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.
It seems to me that this patch, with minor adjustments, can be checked in soonish.
- Volodya