Page MenuHomePhabricator

Clear expression when breakpoint location becomes unresolved
ClosedPublic

Authored by labath on Feb 22 2017, 3:56 AM.

Details

Summary

The compiled expression is not needed once the location goes away. Also,
it holds a reference to the module, which prevents cleanup of the module
if somebody holds a reference to the breakpoint object.

This fixes the StepOverBreakpoint test on windows, but I am unsure how
to make a more generic test for this.

Event Timeline

labath created this revision.Feb 22 2017, 3:56 AM

Note that the breakpoint location's site gets cleared when you disable the breakpoint or its location, as well as when you delete it. So if you have a workflow that does: "hit a breakpoint, disable it, hit another, reenable the first, hit the second" you'll end up re-evaluating the condition expression unnecessarily. This isn't a terribly common workflow, and compiling the expression is not hugely expensive. But in this instance, it was probably better to break the link at the SBBreakpoint level, and not here.

Sorry, I thought I understood Greg wanted me to do this. I can certainly do the SBBreakpoint thing instead.

labath planned changes to this revision.Feb 23 2017, 9:32 AM

No worries. I'll come back soon with the weak_ptr thingy.

jingham edited edge metadata.Feb 23 2017, 12:01 PM

Sorry, I sent a reply but did it by replying to the e-mail version of the patch, and the mail server choked on it for reasons that aren't clear to me...

Greg was most likely thinking that BreakpointLocation had a "Clear" method like the Target and Process, which we can call when WE know we're done with something, but we can't tell when it will actually get destroyed. BreakpointLocation hasn't needed such a thing. In fact, if you wanted to fix this by adding a Clear method and removing the expression there, that would be fine too. In the process & target case we have to know that we've cleared them, but since you're only throwing away a cache, you wouldn't need to do any extra work for BreakpointLocations.

However, solving the reference loop with a weak pointer in SBBreakpoint is fine as well. The targets keep the breakpoints alive as long as the target exists, and about the only sensible thing you can do with a breakpoint after it's target is gone, is print it's ID in an error "Hey, I tried to do something with breakpoint 1 but it didn't work because it's target was gone". Now you will either have to keep that info redundantly on the side or say <UNKNOWN> rather than "1".

That doesn't seems worth keeping the breakpoint alive.

labath updated this revision to Diff 89645.Feb 24 2017, 4:38 AM

Switch SBBreakpoint to weak_ptr. This diff is pretty big but it amounts to
s/m_opaque_sp/m_opaque_wp and inserting a lock at the beginning of every
function. Since I had to touch them anyway, I took the opportunity to upgrade
the logging statements.

If this is ok, I'll follow this up with the same change for SBBreakpointLocation
and SBWatchpoint.

jingham accepted this revision.Feb 24 2017, 10:55 AM

This looks fine.

It reads a little odd to me that the BreakpointSP recipient of GetSP is generally called opaque_sp after the patch. Historically it makes sense because it was the result of the substitution m_opaque_sp -> opaque_sp. But if you didn't know that it might seem odd, since there's nothing opaque about it. Something like "bkpt_sp" would read more naturally. OTOH is does make the patch larger, so I don't feel very strongly about it.

This revision is now accepted and ready to land.Feb 24 2017, 10:55 AM

I like bkpt_sp. I will rename it before submission. It does not make the diff any larger as I have had to rename all occurrences of it anyway.

This revision was automatically updated to reflect the committed changes.