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

runtime/vm/debugger.h

Index: runtime/vm/debugger.h
diff --git a/runtime/vm/debugger.h b/runtime/vm/debugger.h
index c95deb426db4c2f621f839bd1e02c25c05f28928..f36e0b5ebff229aa8f5b060bb5b595745ec20b53 100644
--- a/runtime/vm/debugger.h
+++ b/runtime/vm/debugger.h
@@ -21,9 +21,71 @@ class RemoteObjectCache;
 class SourceBreakpoint;
 class StackFrame;
 
-// SourceBreakpoint represents a user-specified breakpoint location in
-// Dart source. There may be more than one CodeBreakpoint object per
-// SourceBreakpoint.
+// A user-defined breakpoint, which either fires once, for a particular closure,
+// or always. The API's notion of a breakpoint corresponds to this object.
+class BreakpointCondition {

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

I am struggling with the name. Maybe we just call this "Breakpoint", and the old
SourceBreakpoint becomes a BreakpointLocation.

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

Done.

+ public:
+  BreakpointCondition(intptr_t id, SourceBreakpoint* src_bpt)
+    : id_(id),
+      kind_(BreakpointCondition::kNone),
+      next_(NULL),
+      closure_(Instance::null()),
+      src_bpt_(src_bpt) {}
+
+  intptr_t id() const { return id_; }
+  BreakpointCondition* next() const { return next_; }
+  void set_next(BreakpointCondition* n) { next_ = n; }
+
+  SourceBreakpoint* src_bpt() const { return src_bpt_; }
+  void set_src_bpt(SourceBreakpoint* new_src_bpt);
+
+  bool IsRepeated() const { return kind_ == kRepeated; }
+  bool IsSingleShot() const { return kind_ == kSingleShot; }
+  bool IsPerClosure() const { return kind_ == kPerClosure; }
+  RawInstance* closure() const { return closure_; }
+
+  void SetIsRepeated() {
+    ASSERT(kind_ == kNone);
+    kind_ = kRepeated;
+  }
+
+  void SetIsSingleShot() {
+    ASSERT(kind_ == kNone);
+    kind_ = kSingleShot;
+  }
+
+  void SetIsPerClosure(const Instance& closure) {
+    ASSERT(kind_ == kNone);
+    kind_ = kPerClosure;
+    closure_ = closure.raw();
+  }
+
+  void PrintJSON(JSONStream* stream);
+
+ private:
+  void VisitObjectPointers(ObjectPointerVisitor* visitor);
+
+  enum ConditionKind {
+    kNone,
+    kRepeated,

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

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.

+    kSingleShot,
+    kPerClosure,
+  };
+
+  intptr_t id_;
+  ConditionKind kind_;
+  BreakpointCondition* next_;
+  RawInstance* closure_;
+  SourceBreakpoint* src_bpt_;
+
+  friend class SourceBreakpoint;
+  DISALLOW_COPY_AND_ASSIGN(BreakpointCondition);
+};
+
+
+// SourceBreakpoint represents a collection of breakpoint conditions at the same
+// token position in Dart source. There may be more than one CodeBreakpoint
+// object per SourceBreakpoint.
 // An unresolved breakpoint is one where the underlying code has not
 // been compiled yet. Since the code has not been compiled, we don't know
 // the definitive source location yet. The requested source location may
@@ -35,19 +97,25 @@ class StackFrame;
 class SourceBreakpoint {
  public:
   // Create a new unresolved breakpoint.
-  SourceBreakpoint(intptr_t id,
-                   const Script& script,
+  SourceBreakpoint(const Script& script,
                    intptr_t token_pos,
                    intptr_t end_token_pos);
   // Create a new latent breakpoint.
-  SourceBreakpoint(intptr_t id,
-                   const String& url,
+  SourceBreakpoint(const String& url,
                    intptr_t line_number);
 
+  ~SourceBreakpoint() {
+    BreakpointCondition* cond = conditions();

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

Does the body for the destructor need to be inlined in the header file?

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

No, moved to cc.

+    while (cond != NULL) {
+      BreakpointCondition* temp = cond;
+      cond = cond->next();
+      delete temp;
+    }
+  }
+
   RawFunction* function() const { return function_; }
   intptr_t token_pos() const { return token_pos_; }
   intptr_t end_token_pos() const { return end_token_pos_; }
-  intptr_t id() const { return id_; }
 
   RawScript* script() const { return script_; }
   RawString* url() const { return url_; }
@@ -55,33 +123,35 @@ class SourceBreakpoint {
 
   void GetCodeLocation(Library* lib, Script* script, intptr_t* token_pos);
 
+  BreakpointCondition* AddRepeated(Debugger* dbg);
+  BreakpointCondition* AddSingleShot(Debugger* dbg);
+  BreakpointCondition* AddPerClosure(Debugger* dbg, const Instance& closure);
+
   void Enable();

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

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]

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

   void Disable();
   bool IsEnabled() const { return is_enabled_; }
   bool IsResolved() const { return is_resolved_; }
   bool IsLatent() const { return token_pos_ < 0; }
 
-  bool IsOneShot() const { return is_one_shot_; }
-  void SetIsOneShot() { is_one_shot_ = true; }
-
-  void PrintJSON(JSONStream* stream);
-
  private:
   void VisitObjectPointers(ObjectPointerVisitor* visitor);
 
   void SetResolved(const Function& func, intptr_t token_pos);
-  void set_next(SourceBreakpoint* value) { next_ = value; }
+
   SourceBreakpoint* next() const { return this->next_; }
+  void set_next(SourceBreakpoint* value) { next_ = value; }
+
+  BreakpointCondition* conditions() const { return this->conditions_; }
+  void set_conditions(BreakpointCondition* c) { this->conditions_ = c; }
 
-  const intptr_t id_;
   RawScript* script_;
   RawString* url_;
   intptr_t token_pos_;
   intptr_t end_token_pos_;
   bool is_resolved_;
   bool is_enabled_;
-  bool is_one_shot_;
   SourceBreakpoint* next_;
+  BreakpointCondition* conditions_;
 
   // Valid for resolved breakpoints:
   RawFunction* function_;
@@ -194,6 +264,8 @@ class ActivationFrame : public ZoneAllocated {
                   Object* value);
 
   RawArray* GetLocalVariables();
+  RawObject* GetParameter(intptr_t index);
+  RawObject* GetClosure();
   RawObject* GetReceiver();
 
   const Context& GetSavedCurrentContext();
@@ -315,11 +387,11 @@ class DebuggerEvent {
     top_frame_ = frame;
   }
 
-  SourceBreakpoint* breakpoint() const {
+  BreakpointCondition* breakpoint() const {
     ASSERT(type_ == kBreakpointReached || type_ == kBreakpointResolved);
     return breakpoint_;
   }
-  void set_breakpoint(SourceBreakpoint* bpt) {
+  void set_breakpoint(BreakpointCondition* bpt) {
     ASSERT(type_ == kBreakpointReached || type_ == kBreakpointResolved);
     breakpoint_ = bpt;
   }
@@ -341,7 +413,7 @@ class DebuggerEvent {
   Isolate* isolate_;
   EventType type_;
   ActivationFrame* top_frame_;
-  SourceBreakpoint* breakpoint_;
+  BreakpointCondition* breakpoint_;
   const Object* exception_;
 };
 
@@ -365,15 +437,21 @@ class Debugger {
                                const String& function_name);
 
   // Set breakpoint at closest location to function entry.
-  SourceBreakpoint* SetBreakpointAtEntry(const Function& target_function);
+  BreakpointCondition* SetBreakpointAtEntry(const Function& target_function,
+                                            bool single_shot);
+  BreakpointCondition* SetBreakpointAtActivation(const Instance& closure);
 
   // TODO(turnidge): script_url may no longer be specific enough.
-  SourceBreakpoint* SetBreakpointAtLine(const String& script_url,
-                                        intptr_t line_number);
+  BreakpointCondition* SetBreakpointAtLine(const String& script_url,
+                                           intptr_t line_number);
   RawError* OneTimeBreakAtEntry(const Function& target_function);
 
+  SourceBreakpoint* SourceBreakpointAtLine(const String& script_url,
+                                           intptr_t line_number);
+
+
   void RemoveBreakpoint(intptr_t bp_id);
-  SourceBreakpoint* GetBreakpointById(intptr_t id);
+  BreakpointCondition* GetBreakpointById(intptr_t id);
 
   void SetStepOver();
   void SetSingleStep();
@@ -504,9 +582,9 @@ class Debugger {
                                      StackFrame* frame,
                                      const Code& code);
   static DebuggerStackTrace* CollectStackTrace();
-  void SignalBpResolved(SourceBreakpoint *bpt);
+  void SignalBpResolved(BreakpointCondition *bpt);
   void SignalPausedEvent(ActivationFrame* top_frame,
-                         SourceBreakpoint* bpt);
+                         BreakpointCondition* bpt);
 
   intptr_t nextId() { return next_id_++; }