[lldb] Implement delayed breakpoints #192971
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -114,6 +114,7 @@ class ProcessProperties : public Properties { | |
| Args GetAlwaysRunThreadNames() const; | ||
| FollowForkMode GetFollowForkMode() const; | ||
| bool TrackMemoryCacheChanges() const; | ||
| bool GetUseDelayedBreakpoints() const; | ||
|
|
||
| protected: | ||
| Process *m_process; // Can be nullptr for global ProcessProperties | ||
|
|
@@ -2246,6 +2247,9 @@ class Process : public std::enable_shared_from_this<Process>, | |
| // Process Breakpoints | ||
| size_t GetSoftwareBreakpointTrapOpcode(BreakpointSite *bp_site); | ||
|
|
||
| enum class BreakpointAction { Enable, Disable }; | ||
|
|
||
| protected: | ||
| virtual Status EnableBreakpointSite(BreakpointSite *bp_site) { | ||
| return Status::FromErrorStringWithFormatv( | ||
| "error: {0} does not support enabling breakpoints", GetPluginName()); | ||
|
|
@@ -2256,6 +2260,24 @@ class Process : public std::enable_shared_from_this<Process>, | |
| "error: {0} does not support disabling breakpoints", GetPluginName()); | ||
| } | ||
|
|
||
| /// Compare BreakpointSiteSPs by ID, so that iteration order is independent | ||
| /// of pointer addresses. | ||
| struct SiteIDCmp { | ||
| bool operator()(const lldb::BreakpointSiteSP &lhs, | ||
| const lldb::BreakpointSiteSP &rhs) const { | ||
| return lhs->GetID() < rhs->GetID(); | ||
| } | ||
| }; | ||
| using BreakpointSiteToActionMap = | ||
| std::map<lldb::BreakpointSiteSP, BreakpointAction, SiteIDCmp>; | ||
|
JDevlieghere marked this conversation as resolved.
|
||
|
|
||
| virtual llvm::Error | ||
| UpdateBreakpointSites(const BreakpointSiteToActionMap &site_to_action); | ||
|
|
||
| public: | ||
| llvm::Error ExecuteBreakpointSiteAction(BreakpointSite &site, | ||
| Process::BreakpointAction action); | ||
|
|
||
| // This is implemented completely using the lldb::Process API. Subclasses | ||
| // don't need to implement this function unless the standard flow of read | ||
| // existing opcode, write breakpoint opcode, verify breakpoint opcode doesn't | ||
|
|
@@ -2286,6 +2308,8 @@ class Process : public std::enable_shared_from_this<Process>, | |
|
|
||
| bool IsBreakpointSiteEnabled(const BreakpointSite &site); | ||
|
|
||
| bool IsBreakpointSitePhysicallyEnabled(const BreakpointSite &site); | ||
|
|
||
| // BreakpointLocations use RemoveConstituentFromBreakpointSite to remove | ||
| // themselves from the constituent's list of this breakpoint sites. | ||
| void RemoveConstituentFromBreakpointSite(lldb::user_id_t site_id, | ||
|
|
@@ -3540,6 +3564,21 @@ void PruneThreadPlans(); | |
| /// GetExtendedCrashInformation. | ||
| StructuredData::DictionarySP m_crash_info_dict_sp; | ||
|
|
||
| struct DelayedBreakpointCache { | ||
| void Enqueue(lldb::BreakpointSiteSP site, BreakpointAction action); | ||
| void RemoveSite(lldb::BreakpointSiteSP site) { | ||
| m_site_to_action.erase(site); | ||
| } | ||
| void Clear() { m_site_to_action.clear(); } | ||
|
|
||
| BreakpointSiteToActionMap m_site_to_action; | ||
| }; | ||
|
|
||
| DelayedBreakpointCache m_delayed_breakpoints; | ||
|
Member
There was a problem hiding this comment. Do we need to protect this from concurrent access, or is the private state the only one messing with this?
Contributor
Author
There was a problem hiding this comment. That's a good question. It is private state, but can breakpoints be set simultaneously from different threads? Maybe @jimingham has some intuition here.
Contributor
There was a problem hiding this comment. Breakpoints get added to the Target's BreakpointList with BreakpointList::AddBreakpoint, which is guarded by a BreakpointList mutex. Ditto for BreakpointList::RemoveBreakpoint. The BreakpointSiteList additions, removals and ForEach's are protected by a BreakpointSiteList mutex. So the main parts of adding breakpoints are serialized in lldb. If access to m_delayed_breakpoints always happens as part of adding a breakpoint, or adding a site, or iterating over sites, then you should not need another access protection for the DelayedBreakpointCache. However, in places like your addition to: Given that a good portion of the DelayedBreakpointCache operations do happen protected by the BreakpointSiteList mutex, you could protect the other in Process by locking the BreakpointList mutex rather than inventing a new one.
Contributor
Author
There was a problem hiding this comment. Ok, I've given this a try. There are three methods that need to be protected: I looked into re-using the BreakpointSiteList mutex, but it felt too implicit to re-use the lock inside a different data structure, so I opted for a new one. |
||
| std::recursive_mutex m_delayed_breakpoints_mutex; | ||
|
|
||
| llvm::Error FlushDelayedBreakpoints(); | ||
|
|
||
| size_t RemoveBreakpointOpcodesFromBuffer(lldb::addr_t addr, size_t size, | ||
| uint8_t *buf) const; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this competing with another comparison operator? If not, why not make this default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow, these are shared pointers, so the default comparison is the pointer comparison. I don't believe we can change that, unless I'm misinterpreting the question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't you do something like this? https://godbolt.org/z/Pb4P3njGa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. Well, I guess we could do that, but this would need to go in the header, as this is where the type is defined. Placing this in the header would then change the BreakpointSiteSP comparison everywhere where this header is used... which sounds a bit invasive? I doubt we sort these anywhere else, but still... not sure how I feel about it, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly about it but I'm just worried that at some point we'll need an ordering elsewhere and I'm not confident that person will find
SiteIDCmp. Maybe a potential compromise (that minimizes risk) is to do this in a follow-up PR (which we don't cherrypick to thestable/21.xbranch on the Swift fork).