Page MenuHomePhabricator

Make sure deleting all breakpoints clears their sites first

Authored by eugene on Apr 11 2018, 8:26 PM.

Diff Detail


Event Timeline

eugene created this revision.Apr 11 2018, 8:26 PM

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.

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).

120–121 ↗(On Diff #142110)

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

eugene updated this revision to Diff 142209.Apr 12 2018, 10:19 AM
eugene marked 2 inline comments as done.

add comment to the test

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.

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 added a subscriber: davide.Apr 12 2018, 10:23 AM
davide added inline comments.
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.

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 updated this revision to Diff 142319.Apr 12 2018, 7:58 PM

Got rid of the printf in the test

eugene marked 2 inline comments as done.Apr 12 2018, 7:58 PM

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.

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.

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.

That sounds like a good plan to me.

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

jingham accepted this revision.Apr 16 2018, 12:07 PM


This revision is now accepted and ready to land.Apr 16 2018, 12:07 PM
This revision was automatically updated to reflect the committed changes.