Fixing crash after "breakpoint delete". Bug: https://bugs.llvm.org/show_bug.cgi?id=36430
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.
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).
Don't we need to do the same thing in the other "remove" functions (Remove, RemoveAll).
add comment to the test
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.
RemoveAll does the same at the line 83.
you don't really need printf.
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.
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?
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.
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.
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.