This is an archive of the discontinued LLVM Phabricator instance.

Rename StoppointLocation to StoppointSite and drop its relationship with BreakpointLocation
ClosedPublic

Authored by tatyana-krasnukha on Jul 24 2020, 6:43 AM.

Details

Summary

Working on the patch D84257 I noticed that both BreakpointLocation and BreakpointSite were inherited from StoppointLocation. Also, I noticed that they have not so much in common, except the id and hit counting logic.
There is no polymorphic code that uses a pointer/reference to StoppointLocation to handle one of BreakpointLocation and BreakpointSite either.

The patch renames StoppointLocation to StoppointSite and stops BreakpointLocation's inheriting from it.
Hit counting is encapsulated into StoppointHitCounter which is re-used in StoppointSite, BreakpointLocation, and Breakpoint.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2020, 6:43 AM
tatyana-krasnukha edited the summary of this revision. (Show Details)Jul 24 2020, 6:43 AM

Some nits but overall this looks good.

lldb/include/lldb/Breakpoint/Breakpoint.h
656–657

Please make this a Doxygen comment /// above the variable.

lldb/include/lldb/Breakpoint/StoppointHitCounter.h
38

With the 80 col limit the trailing comments are really hard to read and maintain. Please put them above the variable.

lldb/include/lldb/Breakpoint/StoppointSite.h
21

I'm slowly getting rid of these (rather useless) comments, so let's not add any new ones :-)

53

Same comment as earlier.

67

Private already conveys that.

This overall change makes sense to me.

It seems a little awkward that Target has to know that Watchpoints have a m_hit_counter. It's also a little weird that we're clearing watchpoint hit counts when a process dies, but not breakpoint hit counts. If we do one we should do the other... That isn't your doing, but if you need to have Target resetting both the WP & BP hit counts, it might be nicer to do this with an API.

Addressed comments.

It's also a little weird that we're clearing watchpoint hit counts when a process dies, but not breakpoint hit counts. If we do one we should do the other...

BreakpointSites die with the Process, so there is no need to reset their hit count. I'm not sure what logic Breakpoint should follow (as it lives without a process). If we are going to change it, I would do it in a different revision.

This revision is now accepted and ready to land.Jul 29 2020, 9:15 AM
jingham accepted this revision.Jul 29 2020, 10:12 AM

Remember to do the action thingie...

This revision was landed with ongoing or failed builds.Jul 29 2020, 12:08 PM
This revision was automatically updated to reflect the committed changes.
lldb/source/Breakpoint/CMakeLists.txt