[PATCH] Handle deleting all sub-breakpoints at once

[Dodji Seketeli <dodji seketeli org>]

Hello,

While debugging a C++ program which code was mostly inside function
templates, I stumbled across an issue that has to do with breakpoints
with multiple locations.

Suppose you have a function template foo like this:

    template<typename T>
    void
    foo()
    {
      // some code <--- suppose we set a breakpoint here at #1
    }

Then in another function bar, foo is instantiated for multiple types:

    void
    bar()
    {
      foo<int>();
      foo<char>();
    }

So if I set a breakpoint in the body of the foo function template, GDB
will actually set a breakpoint with two locations: one in foo<int> and
another one in foo<char>.  GDB reports that breakpoint with multiple
location like if there were three breakpoints: one with a location in
foo<int>, one with a location in foo<char> and another one with no
location, acting like a parent breakpoint for the first two ones.
Each one of the three breakpoint have their breakpoint id; if the
parent breakpoint has an id "2", the ids of the other breakpoints will
be "2.1" and "2.2".

So in Nemiver, that situation is abstracted in the concept of
breakpoints and sub-breakpoints.  A breakpoint can have several
sub-breakpoint, each sub-breakpoint can have a different location.
Each sub-breakpoint acts and smells like a breakpoint.  It's just that
all sub-breakpoints of a given breakpoints P points to a single
"parent breakpoint" that is P.

Whenever a user points to the breakpoint in #1 above and tries to
delete it, all the sub-breakpoints should be deleted.  And even if the
user tries to delete the sub-breakpoint "2.1", then the sub-breakpoint
"2.2" should be deleted too, along with the breakpoint "2".  Actually
GDB doesn't seem to accept the deleting of a sub-breakpoint.  It
requires that we delete the breakpoint "2"; and then the two
sub-breakpoints "2.1" and "2.2" gets deleted as a consequence.

It's this "delete all sub-breakpoints-at-once" behaviour that Nemiver
is failing to comply with.  When the user points to a sub-breakpoint,
say "2.1" and tries to delete it, Nemiver sends a request to GDB,
asking it to delete the (sub-)breakpoint "2.1".  But then GDB bails
out, saying that "2.1" is not an id for a proper breakpoint; it
expects us to delete "2".

The patch below hopefully implements the proper behaviour.  Ensuring
that Nemiver always requests the deletion of the parent breakpoint.

Tested and applied to master.

        * src/dbgengine/nmv-i-debugger.h
        (IDebugger::Breakpoint::parent_id): New inline method.
        * src/dbgengine/nmv-gdb-engine.cc (GDBEngine::delete_breakpoint):
        When passed the id of a sub-breakpoint, delete its parent
        breakpoint.
        * src/persp/dbgperspective/nmv-breakpoints-view.cc
        (BreakpointsView::Priv::on_debugger_breakpoint_deleted_signal):
        Delete all sub-breakpoint which parent_id equals the id of the
        deleted breakpoint.
        * src/persp/dbgperspective/nmv-dbg-perspective.cc
        (DBGPerspective::on_debugger_breakpoint_deleted_signal): Likewise.
        (DBGPerspective::record_and_save_session): Avoid recording the
        same breakpoint twice b/c we'd recorded to sub-breakpoint of the
        same parent breakpoint.
        (DBGPerspective::execute_program): Do not save/re-set a breakpoint
        twice b/c we'd have saved two sub-breakpoints with the same parent
        id.
        (DBGPerspective::delete_breakpoint): Walk all the breakpoints and
        delete all breakpoints which file location equals the ones of the
        breakpoint to delete.
---
 src/dbgengine/nmv-gdb-engine.cc                  | 10 +++-
 src/dbgengine/nmv-i-debugger.h                   | 16 ++++++
 src/persp/dbgperspective/nmv-breakpoints-view.cc |  4 +-
 src/persp/dbgperspective/nmv-dbg-perspective.cc  | 72 ++++++++++++++++++++++--
 4 files changed, 93 insertions(+), 9 deletions(-)

diff --git a/src/dbgengine/nmv-gdb-engine.cc b/src/dbgengine/nmv-gdb-engine.cc
index 00ac112..7dcf196 100644
--- a/src/dbgengine/nmv-gdb-engine.cc
+++ b/src/dbgengine/nmv-gdb-engine.cc
@@ -5160,8 +5160,16 @@ GDBEngine::delete_breakpoint (const string &a_break_num,
                               const UString &a_cookie)
 {
     LOG_FUNCTION_SCOPE_NORMAL_DD;
+
+    UString id, break_num(a_break_num);
+
+    // If this is a sub-breakpoint ID, then delete its parent
+    // breakpoint as GDB doesn't seem to be able to delete
+    // sub-breakpoints.
+    vector<UString> id_parts = UString(a_break_num).split(".");
+    id = id_parts.size() ? id_parts[0] : break_num;
     queue_command (Command ("delete-breakpoint",
-                            "-break-delete " + a_break_num,
+                            "-break-delete " + id,
                             a_cookie));
 }
 
diff --git a/src/dbgengine/nmv-i-debugger.h b/src/dbgengine/nmv-i-debugger.h
index 36ba2ab..9e05e10 100644
--- a/src/dbgengine/nmv-i-debugger.h
+++ b/src/dbgengine/nmv-i-debugger.h
@@ -154,6 +154,22 @@ public:
                 return str_utils::int_to_string (sub_breakpoint_number ());
         }
 
+        /// Getter of the ID of the parent of this breakpoint.
+        ///
+        ///Note that (at least for GDB) only parent breakpoints can be
+        ///deleted by ID
+        ///
+        ///@return the ID of the parent of this breakpoint.
+        string parent_id () const
+        {
+            string id;
+            if (is_sub_breakpoint ())
+                id = str_utils::int_to_string(parent_breakpoint_number ());
+            else
+                id = str_utils::int_to_string (sub_breakpoint_number ());
+            return id;
+        }
+
         /// Getter of the ID of this breakpoint.  If the breakpoint is
         /// a sub breakpoint, its id has the form of the string "5.4"
         /// where 5 is the number of the parent breakpoint and 4 is
diff --git a/src/persp/dbgperspective/nmv-breakpoints-view.cc 
b/src/persp/dbgperspective/nmv-breakpoints-view.cc
index 32fa51c..53663f4 100644
--- a/src/persp/dbgperspective/nmv-breakpoints-view.cc
+++ b/src/persp/dbgperspective/nmv-breakpoints-view.cc
@@ -553,9 +553,9 @@ public:
         for (Gtk::TreeModel::iterator iter = list_store->children ().begin ();
              iter != list_store->children ().end ();
              ++iter) {
-            if ((*iter)[get_bp_cols ().id] == a_break_number) {
+            IDebugger::Breakpoint bp = (*iter)[get_bp_cols ().breakpoint];
+            if (bp.parent_id () == a_break_number) {
                 iters_to_erase.push_back (iter);
-                break;
             }
         }
         list<Gtk::TreeModel::iterator>::iterator it;
diff --git a/src/persp/dbgperspective/nmv-dbg-perspective.cc b/src/persp/dbgperspective/nmv-dbg-perspective.cc
index 59da587..6e07ce7 100644
--- a/src/persp/dbgperspective/nmv-dbg-perspective.cc
+++ b/src/persp/dbgperspective/nmv-dbg-perspective.cc
@@ -2586,6 +2586,22 @@ DBGPerspective::on_debugger_breakpoint_deleted_signal
     THROW_IF_FAIL (editor);
     update_toggle_menu_text (*editor);
 
+    // Now delete the breakpoints and sub-breakpoints matching
+    // a_break_number.
+    map<string, IDebugger::Breakpoint>::iterator i,
+        end = m_priv->breakpoints.end ();
+    list<map<string, IDebugger::Breakpoint>::iterator> to_erase;
+    list<map<string, IDebugger::Breakpoint>::iterator>::iterator j;
+    for (i = m_priv->breakpoints.begin();i != end; ++i) {
+        UString parent_id = i->second.parent_id ();
+        if (parent_id == a_break_number
+            || i->first == a_break_number)
+            to_erase.push_back (i);
+    }
+
+    for (j = to_erase.begin (); j != to_erase.end (); ++j)
+        m_priv->breakpoints.erase (*j);
+
     NEMIVER_CATCH
 }
 
@@ -5191,6 +5207,9 @@ DBGPerspective::record_and_save_session (ISessMgr::Session &a_session)
     a_session.breakpoints ().clear ();
     a_session.watchpoints ().clear ();
     map<string, IDebugger::Breakpoint>::const_iterator break_iter;
+    map<string, bool> parent_ids_added;
+    map<string, bool>::const_iterator end = parent_ids_added.end ();
+
     for (break_iter = m_priv->breakpoints.begin ();
          break_iter != m_priv->breakpoints.end ();
          ++break_iter) {
@@ -5198,6 +5217,9 @@ DBGPerspective::record_and_save_session (ISessMgr::Session &a_session)
              == IDebugger::Breakpoint::STANDARD_BREAKPOINT_TYPE)
             || (break_iter->second.type ()
              == IDebugger::Breakpoint::COUNTPOINT_TYPE)) {
+            UString parent_id = break_iter->second.parent_id ();
+            if (parent_ids_added.find (parent_id) != end)
+                continue;
             ISessMgr::Breakpoint bp (break_iter->second.file_name (),
                                      break_iter->second.file_full_name (),
                                      break_iter->second.line (),
@@ -5207,6 +5229,7 @@ DBGPerspective::record_and_save_session (ISessMgr::Session &a_session)
                                      debugger ()->is_countpoint
                                      (break_iter->second));
             a_session.breakpoints ().push_back (bp);
+            parent_ids_added[parent_id] = true;
             LOG_DD ("Regular breakpoint scheduled to be stored");
         } else if (break_iter->second.type ()
                    == IDebugger::Breakpoint::WATCHPOINT_TYPE) {
@@ -6182,6 +6205,7 @@ DBGPerspective::execute_program
     map<string, IDebugger::Breakpoint> saved_bps = m_priv->breakpoints;
 
     // delete old breakpoints, if any.
+    m_priv->breakpoints.clear();
     map<string, IDebugger::Breakpoint>::const_iterator bp_it;
     for (bp_it = saved_bps.begin ();
          bp_it != saved_bps.end ();
@@ -6226,10 +6250,31 @@ DBGPerspective::execute_program
         if (!is_new_program) {
             LOG_DD ("here");
             map<string, IDebugger::Breakpoint>::const_iterator it;
+            map<string, bool> bps_set;
+            UString parent_id;
             for (it = saved_bps.begin ();
                  it != saved_bps.end ();
                  ++it) {
-                set_breakpoint (it->second);
+                // for breakpoints that are sub-breakpoints of a parent
+                // breakpoint, only set the parent breakpoint once.
+                if (it->second.has_multiple_locations ()) {
+                    for (vector<IDebugger::Breakpoint>::const_iterator i =
+                             it->second.sub_breakpoints ().begin();
+                         i != it->second.sub_breakpoints ().end ();
+                         ++i) {
+                        parent_id = i->parent_id ();
+                        if (bps_set.find (parent_id) != bps_set.end ())
+                            continue;
+                        set_breakpoint (*i);
+                        bps_set[parent_id] = true;
+                    }
+                } else {
+                    parent_id = it->second.parent_id();
+                    if (bps_set.find (parent_id) != bps_set.end ())
+                        continue;
+                    set_breakpoint (it->second);
+                    bps_set[parent_id] = true;
+                }
             }
             if (!saved_bps.empty())
                 // We are restarting the same program, and we hope that
@@ -6250,8 +6295,14 @@ DBGPerspective::execute_program
     } else {
         LOG_DD ("here");
         vector<IDebugger::Breakpoint>::const_iterator it;
+        map<string, bool> bps_set;
+        UString parent_id;
         for (it = a_breaks.begin (); it != a_breaks.end (); ++it) {
+            parent_id = it->parent_id ();
+            if (bps_set.find (parent_id) != bps_set.end ())
+                continue;
             set_breakpoint (*it);
+            bps_set[parent_id] = true;
         }
         // Here we are starting (or restarting) the program and we
         // hope at least one breakpoint is going to be set; so lets
@@ -7440,11 +7491,20 @@ bool
 DBGPerspective::delete_breakpoint (const UString &a_file_name,
                                    int a_line_num)
 {
-    const IDebugger::Breakpoint *bp;
-    if ((bp = get_breakpoint (a_file_name, a_line_num)) == 0)
-        return false;
-
-    return delete_breakpoint (bp->id ());
+    bool found = false;
+    map<string, IDebugger::Breakpoint>::const_iterator iter;
+    for (iter = m_priv->breakpoints.begin ();
+         iter != m_priv->breakpoints.end ();
+         ++iter) {
+        if (((iter->second.file_full_name () == a_file_name)
+             || (Glib::path_get_basename (iter->second.file_full_name ())
+                 == Glib::path_get_basename (a_file_name)))
+            && (iter->second.line () == a_line_num)) {
+            delete_breakpoint (iter->first);
+            found = true;
+        }
+    }
+    return found;
 }
 
 bool
-- 
1.8.1.4


-- 
                Dodji