[Lldb-commits] [PATCH] D45554: Make sure deleting all breakpoints clears their sites first

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

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?


https://reviews.llvm.org/D45554