Page MenuHomePhabricator

[lldb] Make BreakpointResolver hold weak_ptr instead of raw pointer to breakpoint
ClosedPublic

Authored by tatyana-krasnukha on Feb 13 2020, 7:05 AM.

Details

Summary

This prevents calling Breakpoint::shared_from_this of an object that is not owned by any shared_ptr.

The patch is a consequence of D74556

Diff Detail

Event Timeline

I wonder if it wouldn't be better to assert in GetBreakpoint. Except when you are making the resolver, you should never have a breakpoint resolver without a valid breakpoint. And there's no point in calling GetBreakpoint when you know you haven't set it yet. You assert after most of the calls to GetBreakpoint, but not all. Of the ones you don't, I think most of them should be.

Were there any places you found where it was legit to ask for the Breakpoint for a resolver and not have one?

Other than that LGTM.

Moved assertion into GetBreakpoint().

Were there any places you found where it was legit to ask for the Breakpoint for a resolver and not have one?

I didn't find any, however, it seems to be possible in theory as BreakpointResolver is allowed to be constructed with nullptr instead of breakpoint (it is expected to call SetBreakpoint later).

tatyana-krasnukha updated this revision to Diff 244727.EditedFeb 14 2020, 11:46 AM
tatyana-krasnukha added a reviewer: jingham.

Just realized that BreakpointResolverScripted::CreateImplementationIfNeeded should work when m_breakpoint == nullptr.
GetBreakpoint with assertion is not suitable here, so I pass the breakpoint as a parameter.

jingham accepted this revision.Mar 3 2020, 10:01 AM

You mean for BreakpointResolverScripted::CreateImplementationIfNeeded? Passing in the BreakpointSP is fine.

This revision is now accepted and ready to land.Mar 3 2020, 10:01 AM
This revision was automatically updated to reflect the committed changes.