-
Couldn't load subscription status.
- Fork 15k
[lldb] Refactor LLDB Breakpoint Event Notifications to centralize and eliminate code duplication #164739
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[lldb] Refactor LLDB Breakpoint Event Notifications to centralize and eliminate code duplication #164739
Conversation
|
@llvm/pr-subscribers-lldb Author: Piyush Jaiswal (piyushjaiswal98) ChangesSummaryThis PR refactors breakpoint event notification in LLDB to centralize and eliminate code duplication. It creates a unified method in the Test<img width="1532" height="76" alt="Screenshot 2025-10-23 at 12 49 31 PM" src="https://github.com/user-attachments/assets/6d6a6da6-9684-463c-aeeb-90663cdbd077" /> Full diff: https://github.com/llvm/llvm-project/pull/164739.diff 5 Files Affected:
diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index f4a09237ce897..4d4355212a70b 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -1346,6 +1346,21 @@ class Target : public std::enable_shared_from_this<Target>,
const lldb_private::RegisterFlags &flags,
uint32_t byte_size);
+ /// Sends a breakpoint notification event if any listener is registered.
+ /// \param[in] bp
+ /// The breakpoint to send a nofication for.
+ /// \param[in] eventKind
+ /// The kind of event that occurred.
+ void NotifyBreakpointChanged(Breakpoint &bp,
+ lldb::BreakpointEventType eventKind);
+ /// Sends a breakpoint notification event if any listener is registered.
+ /// \param[in] bp
+ /// The breakpoint that has changed.
+ /// \param[in] data
+ /// The data associated with the event.
+ void NotifyBreakpointChanged(Breakpoint &bp,
+ const lldb::EventDataSP &breakpoint_data_sp);
+
llvm::Expected<lldb::DisassemblerSP>
ReadInstructions(const Address &start_addr, uint32_t count,
const char *flavor_string = nullptr);
diff --git a/lldb/source/Breakpoint/Breakpoint.cpp b/lldb/source/Breakpoint/Breakpoint.cpp
index b23d1143d60c4..63604bfb406e5 100644
--- a/lldb/source/Breakpoint/Breakpoint.cpp
+++ b/lldb/source/Breakpoint/Breakpoint.cpp
@@ -1099,12 +1099,8 @@ bool Breakpoint::EvaluatePrecondition(StoppointCallbackContext &context) {
void Breakpoint::SendBreakpointChangedEvent(
lldb::BreakpointEventType eventKind) {
- if (!IsInternal() && GetTarget().EventTypeHasListeners(
- Target::eBroadcastBitBreakpointChanged)) {
- std::shared_ptr<BreakpointEventData> data =
- std::make_shared<BreakpointEventData>(eventKind, shared_from_this());
-
- GetTarget().BroadcastEvent(Target::eBroadcastBitBreakpointChanged, data);
+ if (!IsInternal()) {
+ GetTarget().NotifyBreakpointChanged(*this, eventKind);
}
}
@@ -1113,10 +1109,8 @@ void Breakpoint::SendBreakpointChangedEvent(
if (!breakpoint_data_sp)
return;
- if (!IsInternal() &&
- GetTarget().EventTypeHasListeners(Target::eBroadcastBitBreakpointChanged))
- GetTarget().BroadcastEvent(Target::eBroadcastBitBreakpointChanged,
- breakpoint_data_sp);
+ if (!IsInternal())
+ GetTarget().NotifyBreakpointChanged(*this, breakpoint_data_sp);
}
const char *Breakpoint::BreakpointEventTypeAsCString(BreakpointEventType type) {
diff --git a/lldb/source/Breakpoint/BreakpointList.cpp b/lldb/source/Breakpoint/BreakpointList.cpp
index 779490ae0316a..e3dd62bfa329d 100644
--- a/lldb/source/Breakpoint/BreakpointList.cpp
+++ b/lldb/source/Breakpoint/BreakpointList.cpp
@@ -16,13 +16,7 @@ using namespace lldb;
using namespace lldb_private;
static void NotifyChange(const BreakpointSP &bp, BreakpointEventType event) {
- Target &target = bp->GetTarget();
- if (target.EventTypeHasListeners(Target::eBroadcastBitBreakpointChanged)) {
- auto event_data_sp =
- std::make_shared<Breakpoint::BreakpointEventData>(event, bp);
- target.BroadcastEvent(Target::eBroadcastBitBreakpointChanged,
- event_data_sp);
- }
+ bp->GetTarget().NotifyBreakpointChanged(*bp, event);
}
BreakpointList::BreakpointList(bool is_internal)
diff --git a/lldb/source/Breakpoint/BreakpointLocation.cpp b/lldb/source/Breakpoint/BreakpointLocation.cpp
index 22c98acda8c59..f25209c15e007 100644
--- a/lldb/source/Breakpoint/BreakpointLocation.cpp
+++ b/lldb/source/Breakpoint/BreakpointLocation.cpp
@@ -749,13 +749,11 @@ void BreakpointLocation::Dump(Stream *s) const {
void BreakpointLocation::SendBreakpointLocationChangedEvent(
lldb::BreakpointEventType eventKind) {
- if (!m_owner.IsInternal() && m_owner.GetTarget().EventTypeHasListeners(
- Target::eBroadcastBitBreakpointChanged)) {
+ if (!m_owner.IsInternal()) {
auto data_sp = std::make_shared<Breakpoint::BreakpointEventData>(
eventKind, m_owner.shared_from_this());
data_sp->GetBreakpointLocationCollection().Add(shared_from_this());
- m_owner.GetTarget().BroadcastEvent(Target::eBroadcastBitBreakpointChanged,
- data_sp);
+ m_owner.GetTarget().NotifyBreakpointChanged(m_owner, data_sp);
}
}
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index e224a12e33463..8780d46afb331 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "lldb/Target/Target.h"
+#include "lldb/Breakpoint/Breakpoint.h"
#include "lldb/Breakpoint/BreakpointIDList.h"
#include "lldb/Breakpoint/BreakpointPrecondition.h"
#include "lldb/Breakpoint/BreakpointResolver.h"
@@ -5244,3 +5245,19 @@ void Target::ClearSectionLoadList() { GetSectionLoadList().Clear(); }
void Target::DumpSectionLoadList(Stream &s) {
GetSectionLoadList().Dump(s, this);
}
+
+void Target::NotifyBreakpointChanged(Breakpoint &bp,
+ lldb::BreakpointEventType eventKind) {
+ if (EventTypeHasListeners(Target::eBroadcastBitBreakpointChanged)) {
+ std::shared_ptr<Breakpoint::BreakpointEventData> data_sp =
+ std::make_shared<Breakpoint::BreakpointEventData>(
+ eventKind, bp.shared_from_this());
+ BroadcastEvent(Target::eBroadcastBitBreakpointChanged, data_sp);
+ }
+}
+
+void Target::NotifyBreakpointChanged(
+ Breakpoint &bp, const lldb::EventDataSP &breakpoint_data_sp) {
+ if (EventTypeHasListeners(Target::eBroadcastBitBreakpointChanged))
+ BroadcastEvent(Target::eBroadcastBitBreakpointChanged, breakpoint_data_sp);
+}
|
lldb/include/lldb/Target/Target.h
Outdated
| const lldb_private::RegisterFlags &flags, | ||
| uint32_t byte_size); | ||
|
|
||
| /// Sends a breakpoint notification event if any listener is registered. |
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.
Broadcaster::BroadcastEvent will only send out the event if a listener is registered, so this isn't a special feature of NotifyBreakpointChanged. What the old code, and your new API's add is that mostly we won't bother to cons up the breakpoint event at all if there are no listeners. But I'm not sure that's worth calling out, and it might confuse people into thinking that wasn't a general behavior of Broadcaster/Listeners.
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.
Other than that comment comment, this looks fine to em.
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.
Makes sense....Updated
Summary
This PR refactors breakpoint event notification in LLDB to centralize and eliminate code duplication. It creates a unified method in the
Targetclass for sending breakpoint change events. The new methods check if listeners exist before broadcasting eventsTest