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
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
