Make sure deleting all breakpoints clears their sites first

[eugene]

Summary

Fixing crash after "breakpoint delete". Bug: https://bugs.llvm.org/show_bug.cgi?id=36430

[labath]

The fix seems simple enough, but Jim needs to say whether this is the right way to fix this bug, as I am not sure about what are our assumptions about Breakpoint object lifetimes.

packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c

13 ↗

(On Diff #142110)

Why did you need to add this? This seems like something that could easily be removed/reshuffled in the future (breaking your test, if it depends on it).

source/Breakpoint/BreakpointList.cpp

120–121 ↗

(On Diff #142110)

Don't we need to do the same thing in the other "remove" functions (Remove, RemoveAll).

[eugene]

add comment to the test

packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c

13 ↗

(On Diff #142110)

I just need more than one bp location in the function. "bp main" and a breakpoint on the line 14 were the same thing before that.
I'll add a comment.

source/Breakpoint/BreakpointList.cpp

120–121 ↗

(On Diff #142110)

RemoveAll does the same at the line 83.
With Remove it's a more complicated. Target removes sites manually by disabling breakpoint, but it has some extra logic that I don't fully understand and not sure if I should touch.

[davide]

packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c

14–15 ↗

(On Diff #142209)

you don't really need printf.
You can just declare a set of few new fresh variables, one per line, I think.

14–15 ↗

(On Diff #142209)

The reason why I'm saying this is because this test doesn't really rely on stdio.h and I'd rather leave it as is.

[jingham]

Calling ClearAllBreakpointSites twice does no harm, it just sees the list is empty and goes on. So even though Target::RemoveBreakpointByID clears the breakpoint sites by disabling them and then calls BreakpointList::Remove, it is fine for BreakpointList::Remove to also call ClearAllBreakpointSites and it probably should do so. It currently has no other callers than Target::RemoveBreakpointByID , but if somebody else did it would be good for it to work properly.

On the lifetime question, the policy over what we should do if somebody is holding on to an SP to the breakpoint when the breakpoint gets removed from the target's list isn't solid. I originally used Shared Pointers because I wasn't sure what a good mechanism was for preserving breakpoints against general deletion ("break delete"). But actually a breakpoint that isn't in the Target breakpoint list should just go away, I didn't end up needing to do anything useful with them after they are removed. So longer term it is a good idea to remove Breakpoint SP's, make the breakpoint list own the breakpoints, then make it have fast access for ID->Breakpoint * lookups, and have all external clients hold onto Breakpoint ID's instead. You can use the Internal list, or you can now use the hidden names feature to keep breakpoints you care about from getting inadvertently deleted. So I don't think we'll ever need another clever way to keep breakpoints alive.

But in practice I don't think this is a big problem. As long as we get rid of the BreakpointSite's when the breakpoint gets removed from the list, it becomes inert. Breakpoints outside the breakpoint list won't be told to update themselves, so they won't re-acquire sites.

I am a little surprised however that in such a simple scenario there is another agent holding onto the BreakpointSP. After all, if there weren't another reference, erasing the SP from the list should have destroyed the last instance, and the Breakpoint destructor will then destroy the location list, which removes the sites. Eugene, did you see who that was when you were poking around at this?

[eugene]

Got rid of the printf in the test

[eugene]

There is an ownership cycle between BreakpointSite::m_owners and BreakpointLocation::m_bp_site_sp.
We should probably make m_owners a collection of weak references.
But currently most of the code just works it around by calling Breakpoint::ClearAllBreakpointSites() before deleting a breakpoint.

[clayborg]

In D45554#1066730, @eugene wrote:

There is an ownership cycle between BreakpointSite::m_owners and BreakpointLocation::m_bp_site_sp.
We should probably make m_owners a collection of weak references.
But currently most of the code just works it around by calling Breakpoint::ClearAllBreakpointSites() before deleting a breakpoint.

Anytime there is a clear hierarchy we should use shared and weak pointers correctly. I would vote to fix this issue.

[jingham]

Sure, if somebody has the time fixing this to use weak pointers would be great.

But that doesn't seem like the real issue to me.

When a breakpoint gets removed from the Target BreakpointList(s), regardless of who else is holding onto it, it needs to get its breakpoint sites removed (since the Target can no longer reason about them they will look like unrecognized traps.) We can't rely on Destructors to do this as long as we are handing out BreakpointSP's. So you still need an explicit operation to clear the breakpoint sites.

I can't think of any compelling use for preserving a Breakpoint once it is removed from the Target. So I'm pretty convinced we should go to the model where removing a Breakpoint from the Target's BreakpointList immediately deletes the Breakpoint. Then we can fix this dependency so the destructors do the right thing. Note, for the sake of making an important step less indirect, I would vote to also have the Breakpoint destructor call ClearBreakpointSites().

This is a more intrusive change, since it means either that everybody who is now holding BreakpointSP's should switch over to holding BreakpointID's or BreakpointWP's. But that models more closely what should happen with breakpoints.

[eugene]

Well, I agree that breakpoints, locations and sites could benefit from ownership refactoring.
shared_ptr cycles are bad.
Let's discuss it at lldb-dev.
Meanwhile I think it's still ok to fix this bug right now, by doing what has already been done in other places.

[jingham]

That sounds like a good plan to me.

[eugene]

If nobody minds, I'd appreciate if somebody would accept this patch.

[jingham]

Sure.

lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/TestBreakpointCommand.py

def setUp(self):
         TestBase.setUp(self)
         # Find the line number to break inside main().
         self.line = line_number('main.c', '// Set break point at this line.')
         # disable "There is a running process, kill it and restart?" prompt
         self.runCmd("settings set auto-confirm true")
         self.addTearDownHook(
             lambda: self.runCmd("settings clear auto-confirm"))
 
+    @expectedFailureAll(oslist=["windows"], bugnumber="llvm.org/pr24528")
+    def test_delete_all_breakpoints(self):
+        """Test that deleting all breakpoints works."""
+        self.build()
+        exe = self.getBuildArtifact("a.out")
+        self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
+
+        lldbutil.run_break_set_by_symbol(self, "main")
+        lldbutil.run_break_set_by_file_and_line(
+            self, "main.c", self.line, num_expected_locations=1, loc_exact=True)
+
+        self.runCmd("run", RUN_SUCCEEDED)
+
+        self.runCmd("breakpoint delete")
+        self.runCmd("process continue")
+        self.expect("process status", PROCESS_STOPPED,
+                    patterns=['Process .* exited with status = 0'])
+
+
     def breakpoint_command_sequence(self):
         """Test a sequence of breakpoint command add, list, and delete."""
         exe = self.getBuildArtifact("a.out")
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
 
         # Add three breakpoints on the same line.  The first time we don't specify the file,
         # since the default file is the one containing main:
         lldbutil.run_break_set_by_file_and_line(

lldb/trunk/packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_command/main.c

 //===-- main.c --------------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 int main (int argc, char const *argv[])
 {
+    // Add a body to the function, so we can set more than one
+    // breakpoint in it.
+    static volatile int var = 0;
+    var++;
     return 0; // Set break point at this line.
 }

lldb/trunk/source/Breakpoint/BreakpointList.cpp

for (pos = m_breakpoints.begin(); pos != end; ++pos) {
       }
     }
   }
   m_breakpoints.erase(m_breakpoints.begin(), m_breakpoints.end());
 }
 
 void BreakpointList::RemoveAllowed(bool notify) {
   std::lock_guard<std::recursive_mutex> guard(m_mutex);
-  
   bp_collection::iterator pos, end = m_breakpoints.end();
   if (notify) {
     for (pos = m_breakpoints.begin(); pos != end; ++pos) {
       if(!(*pos)->AllowDelete())
         continue;
       if ((*pos)->GetTarget().EventTypeHasListeners(
               Target::eBroadcastBitBreakpointChanged)) {
         (*pos)->GetTarget().BroadcastEvent(
             Target::eBroadcastBitBreakpointChanged,
             new Breakpoint::BreakpointEventData(eBreakpointEventTypeRemoved,
                                                 *pos));
       }
     }
   }
   pos = m_breakpoints.begin();
   while ( pos != end) {
-      if((*pos)->AllowDelete())
-        pos = m_breakpoints.erase(pos);
-      else
-        pos++;
+    auto bp = *pos;
+    if (bp->AllowDelete()) {
+      bp->ClearAllBreakpointSites();
+    } else
   }
 }
 
 class BreakpointIDMatches {
 public:
   BreakpointIDMatches(break_id_t break_id) : m_break_id(break_id) {}
 
   bool operator()(const BreakpointSP &bp) const {