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 Monday 09 June 2008 14:46:47 you 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'm just trying to find agreement the MI should use event notification for
> change of state and not directly MI command output.
I think you're mixing two unrelated issues together. One is that MI client should
be notified if breakpoints are changed, no matter what command changed breakpoints.
There's no argument that it's necessary. The one issues if whether -break-insert
should report newly created breakpoint directly, of via notification.
> I should have used RFC in
> the subject header. I will submit a proper patch later. Note that removing
> the "wpt" field requires a new MI level.
I don't think so -- there are no breakpoint change notification in current GDB,
so no backward compatibility issue with removing this bit from =breakpoint-changed.
> > ...
> > > 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).
>
> When I submit a patch you'll probably have to explain to me where breakpoints
> might change.
Well, if you prefer to first write a patch, and then adjust it, fine :-)
> > > > 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.
>
> The event notification reports the breakpoint number so what is there to
> match?.
Frontend has one breakpoint in its breakpoint table, having id of -1, because
the breakpoint is not inserted yet. Now, notification arrives, saying that
there's new breakpoint with id of 7. Should the frontend add a new breakpoint
to its breakpoint table, or should it update the breakpoint already in its
table with the properties reported by GDB? The frontend can decide by
knowing that the last command was -break-insert, but GDB can do it easily, too.
> I don't see why it should be reported twice, except perhaps not to
> break existing behaviour. In that case, I think it would be better to
> condition on the MI level, so current level outputs as now and mi=3 outputs
> notifications only.
Of course, we can use mi level to do this, but should we? Current frontends
work fine, except that they miss breakpoint changes caused by CLI commands.
We want to fix that, and to that end, we want to introduce breakpoint change
notifications. Why force the frontend to modify the logic for the commands that
already work fine -- like -break-insert, or -break-disable?
> > 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.
>
> In a frontend that uses the console, e.g., Eclipse, if the user enters a
> command like "disable 1", how does it know that this breakpoint has been
> disabled?
I did not say anything about 'disable 1', I'm talking specifically about MI
commands which indended purpose is to modify a breakpoint.
> > Presumably, we can suppress the breakpoint observer during executing of MI
> > commands that directly create/modify breakpoint,
>
> That seems to defeat the object.
> > and leave the observer
> > enabled for other commands -- where we don't mean no breakpoint changes but
> > might get them.
>
> Where would that happen?
"that" been what? The "other commands" are all CLI commands, as well as MI commands
that don't change breakpoints as their primary purpose but might change them
accidentally -- for example, run commands update hit count of breakpoints and
might auto-delete breakpoints. The logic for temporary disabling notification
belongs, probably, to individual breakpoint commands.
> > > > 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.
>
> I guess it could output the breakpoint directly in addition to the notification.
> Current frontends should ignore such notifications (if they don't they will
> presumably break with the ones already added).
I think we can assume the frontends ignore notifications they don't understand.
> > > 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.
>
> I'm thinking of changes to accommodate non stop mode, e.g., no token in stopped
> output (I realise that this has already been made). Note these examples are
> still present in the GDB/MI Program Execution node of th manual:
>
> (gdb)
> 111-exec-continue
> 111^running
>
> (gdb)
> 000-exec-run
> 000^running
>
> changing ^running to *running?,
No such change is planned. *running will be output in addition to ^running. While
I'll surely kill ^running if we bump MI level, removing ^running does not appear
to be very important in itself.
> removing the (gdb) `prompt'?
So far, my non-stop patches don't remove it.
> > > 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.
>
> I'll see what I can do but a lot of questions seem to remain.
I'm sure we can sort those questions.
- Volodya