Per-closure breakpoints; restructure breakpoint implementation to keep a list of conditions.

Per-closure breakpoints; restructure breakpoint implementation to keep a list of conditions.

BUG=
R=hausner@google.com

Committed: https://github.com/dart-lang/sdk/commit/d37149ad3b988e62e7f87931a06d62de6be1645c

[rmacnak, 2015-05-20 20:32:31]

rmacnak@google.com changed reviewers:
+ hausner@google.com

[hausner, 2015-05-21 18:23:55]

This is much better than the first iteration. I struggle with the naming a bit.
We can talk offline.

https://codereview.chromium.org/1146173003/diff/40001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/1146173003/diff/40001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:2404: condition_met = cond;
This is subtly wrong. If you have a one-shot breakpoint at the same location but
it appears later in the list of BreakpointCondition, it does not get deleted
after the pause event has been signaled.

Another question to ponder: if there are several BreakpointCondition breakpoints
at the same location (SourceBreakpoint), should they all fire if their condition
is met?

https://codereview.chromium.org/1146173003/diff/40001/runtime/vm/debugger.h
File runtime/vm/debugger.h (right):

https://codereview.chromium.org/1146173003/diff/40001/runtime/vm/debugger.h#n...
runtime/vm/debugger.h:26: class BreakpointCondition {
I am struggling with the name. Maybe we just call this "Breakpoint", and the old
SourceBreakpoint becomes a BreakpointLocation.

https://codereview.chromium.org/1146173003/diff/40001/runtime/vm/debugger.h#n...
runtime/vm/debugger.h:70: kRepeated,
What if you want to set a one-shopt breakpoint in a particular closure? I feel
that the "single shot" attribute should be orthogonal to the other kinds.

https://codereview.chromium.org/1146173003/diff/40001/runtime/vm/debugger.h#n...
runtime/vm/debugger.h:108: BreakpointCondition* cond = conditions();
Does the body for the destructor need to be inlined in the header file?

https://codereview.chromium.org/1146173003/diff/40001/runtime/vm/debugger.h#n...
runtime/vm/debugger.h:130: void Enable();
The enabled/disabled property should be part of the Breakpoint, not the location
(which may have multipleof breakpoints).

[rmacnak, 2015-05-21 21:59:35]

rmacnak@google.com changed reviewers:
+ iposva@google.com

[rmacnak, 2015-05-21 21:59:35]

https://codereview.chromium.org/1146173003/diff/40001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/1146173003/diff/40001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:2404: condition_met = cond;
On 2015/05/21 18:23:55, hausner wrote:
> This is subtly wrong. If you have a one-shot breakpoint at the same location
but
> it appears later in the list of BreakpointCondition, it does not get deleted
> after the pause event has been signaled.
> 
> Another question to ponder: if there are several BreakpointCondition
breakpoints
> at the same location (SourceBreakpoint), should they all fire if their
condition
> is met?

As discussed, we should not send a series of breakpoint hit notifications
because the client may respond with a continue for each of them. We should
update the protocol to send all the breakpoint whose condition is satisfied
together. For now, only treating one as hit, with priority single shot > per
closure > general.

https://codereview.chromium.org/1146173003/diff/40001/runtime/vm/debugger.h
File runtime/vm/debugger.h (right):

https://codereview.chromium.org/1146173003/diff/40001/runtime/vm/debugger.h#n...
runtime/vm/debugger.h:26: class BreakpointCondition {
On 2015/05/21 18:23:55, hausner wrote:
> I am struggling with the name. Maybe we just call this "Breakpoint", and the
old
> SourceBreakpoint becomes a BreakpointLocation.

Done.

https://codereview.chromium.org/1146173003/diff/40001/runtime/vm/debugger.h#n...
runtime/vm/debugger.h:108: BreakpointCondition* cond = conditions();
On 2015/05/21 18:23:55, hausner wrote:
> Does the body for the destructor need to be inlined in the header file?

No, moved to cc.

https://codereview.chromium.org/1146173003/diff/40001/runtime/vm/debugger.h#n...
runtime/vm/debugger.h:130: void Enable();
On 2015/05/21 18:23:55, hausner wrote:
> The enabled/disabled property should be part of the Breakpoint, not the
location
> (which may have multipleof breakpoints).

Actually, no one calls Disable(), so deleted this.

[hausner, 2015-05-21 22:46:23]

LGTM with comments.

https://codereview.chromium.org/1146173003/diff/80001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/1146173003/diff/80001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:319: Breakpoint*
BreakpointLocation::AddRepeated(Debugger* dbg) {
Looks like the code for the AddXXXX() functions could be factored out.

https://codereview.chromium.org/1146173003/diff/80001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:2404: Breakpoint* bpt_hit = NULL;
Maybe a quick explanation of the heuristics we use to find a relevant
breakpoint.

https://codereview.chromium.org/1146173003/diff/80001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:2791: while (curr_loc != NULL) {
Might be nice to factor out a function that returns the BreakpointLocation for a
given id. Or alternatively, you could add a pointer to the location to the
Breakpoint object.
.. thinking a bit more ...
On the other hand, you need the previous location to maintain the list, so what
you have is probably fine.

https://codereview.chromium.org/1146173003/diff/80001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:2880: intptr_t token_pos) {
indentation

https://codereview.chromium.org/1146173003/diff/80001/runtime/vm/debugger.h
File runtime/vm/debugger.h (right):

https://codereview.chromium.org/1146173003/diff/80001/runtime/vm/debugger.h#n...
runtime/vm/debugger.h:101: intptr_t token_pos,
Indentation. (Also at other locations where the substitution to
BreakpointLocation was made.)

https://codereview.chromium.org/1146173003/diff/80001/runtime/vm/debugger.h#n...
runtime/vm/debugger.h:338: typedef void BreakpointHandler(Dart_Port isolate_id,
This typedef seems to be obsolete. I don't see this type being used anywhere.

[rmacnak, 2015-05-21 23:51:57]

https://codereview.chromium.org/1146173003/diff/80001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/1146173003/diff/80001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:319: Breakpoint*
BreakpointLocation::AddRepeated(Debugger* dbg) {
On 2015/05/21 22:46:23, hausner wrote:
> Looks like the code for the AddXXXX() functions could be factored out.

Done.

https://codereview.chromium.org/1146173003/diff/80001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:2404: Breakpoint* bpt_hit = NULL;
On 2015/05/21 22:46:23, hausner wrote:
> Maybe a quick explanation of the heuristics we use to find a relevant
> breakpoint.

Done.

https://codereview.chromium.org/1146173003/diff/80001/runtime/vm/debugger.h
File runtime/vm/debugger.h (right):

https://codereview.chromium.org/1146173003/diff/80001/runtime/vm/debugger.h#n...
runtime/vm/debugger.h:338: typedef void BreakpointHandler(Dart_Port isolate_id,
On 2015/05/21 22:46:23, hausner wrote:
> This typedef seems to be obsolete. I don't see this type being used anywhere.

Removed

[rmacnak, 2015-05-21 23:52:16]

Committed patchset #7 (id:120001) manually as
d37149ad3b988e62e7f87931a06d62de6be1645c (presubmit successful).