[Lldb-commits] [lldb] [lldb][NFCI] Remove m_being_created from Breakpoint classes (PR #79716)
Alex Langford via lldb-commits
lldb-commits at lists.llvm.org
Mon Jan 29 14:16:12 PST 2024
https://github.com/bulbazord updated https://github.com/llvm/llvm-project/pull/79716
>From 1f81c3e01a12682ad514038d2e9f1baea80dcf03 Mon Sep 17 00:00:00 2001
From: Alex Langford <alangford at apple.com>
Date: Sat, 27 Jan 2024 15:54:16 -0800
Subject: [PATCH 1/3] [lldb][NFCI] Remove m_being_created from Breakpoint
classes
The purpose of m_being_created in these classes was to prevent
broadcasting an event related to these Breakpoints during the creation
of the breakpoint (i.e. in the constructor). In Breakpoint and
Watchpoint, m_being_created had no effect. That is to say, removing it
does not change behavior.
However, BreakpointLocation does still use m_being_created. In the
constructor, SetThreadID is called which does broadcast an event only if
`m_being_created` is false. Instead of having this logic be roundabout,
the constructor instead calls `SetThreadIDInternal`, which actually
changes the thread ID. `SetThreadID` also will call
`SetThreadIDInternal` in addition to broadcasting a changed event.
---
lldb/include/lldb/Breakpoint/Breakpoint.h | 1 -
.../lldb/Breakpoint/BreakpointLocation.h | 3 +-
lldb/include/lldb/Breakpoint/Watchpoint.h | 2 --
lldb/source/Breakpoint/Breakpoint.cpp | 22 +++++-------
lldb/source/Breakpoint/BreakpointLocation.cpp | 34 ++++++++++---------
lldb/source/Breakpoint/Watchpoint.cpp | 8 ++---
6 files changed, 32 insertions(+), 38 deletions(-)
diff --git a/lldb/include/lldb/Breakpoint/Breakpoint.h b/lldb/include/lldb/Breakpoint/Breakpoint.h
index 8c4308ab0bc12..0b6bf593be37a 100644
--- a/lldb/include/lldb/Breakpoint/Breakpoint.h
+++ b/lldb/include/lldb/Breakpoint/Breakpoint.h
@@ -637,7 +637,6 @@ class Breakpoint : public std::enable_shared_from_this<Breakpoint>,
Breakpoint(Target &new_target, const Breakpoint &bp_to_copy_from);
// For Breakpoint only
- bool m_being_created;
bool
m_hardware; // If this breakpoint is required to use a hardware breakpoint
Target &m_target; // The target that holds this breakpoint.
diff --git a/lldb/include/lldb/Breakpoint/BreakpointLocation.h b/lldb/include/lldb/Breakpoint/BreakpointLocation.h
index 2a4f9fc01bf32..273132c950825 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointLocation.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointLocation.h
@@ -313,6 +313,8 @@ class BreakpointLocation
void UndoBumpHitCount();
+ void SetThreadIDInternal(lldb::tid_t thread_id);
+
// Constructors and Destructors
//
// Only the Breakpoint can make breakpoint locations, and it owns them.
@@ -337,7 +339,6 @@ class BreakpointLocation
bool check_for_resolver = true);
// Data members:
- bool m_being_created;
bool m_should_resolve_indirect_functions;
bool m_is_reexported;
bool m_is_indirect;
diff --git a/lldb/include/lldb/Breakpoint/Watchpoint.h b/lldb/include/lldb/Breakpoint/Watchpoint.h
index 22fdfd686c3f1..fc11a466ba605 100644
--- a/lldb/include/lldb/Breakpoint/Watchpoint.h
+++ b/lldb/include/lldb/Breakpoint/Watchpoint.h
@@ -227,8 +227,6 @@ class Watchpoint : public std::enable_shared_from_this<Watchpoint>,
WatchpointOptions
m_options; // Settable watchpoint options, which is a delegate to handle
// the callback machinery.
- bool m_being_created;
-
std::unique_ptr<UserExpression> m_condition_up; // The condition to test.
void SetID(lldb::watch_id_t id) { m_id = id; }
diff --git a/lldb/source/Breakpoint/Breakpoint.cpp b/lldb/source/Breakpoint/Breakpoint.cpp
index af5dcc9cd88d4..ae845e92762b9 100644
--- a/lldb/source/Breakpoint/Breakpoint.cpp
+++ b/lldb/source/Breakpoint/Breakpoint.cpp
@@ -44,17 +44,14 @@ const char *Breakpoint::g_option_names[static_cast<uint32_t>(
Breakpoint::Breakpoint(Target &target, SearchFilterSP &filter_sp,
BreakpointResolverSP &resolver_sp, bool hardware,
bool resolve_indirect_symbols)
- : m_being_created(true), m_hardware(hardware), m_target(target),
- m_filter_sp(filter_sp), m_resolver_sp(resolver_sp), m_options(true),
- m_locations(*this), m_resolve_indirect_symbols(resolve_indirect_symbols),
- m_hit_counter() {
- m_being_created = false;
-}
+ : m_hardware(hardware), m_target(target), m_filter_sp(filter_sp),
+ m_resolver_sp(resolver_sp), m_options(true), m_locations(*this),
+ m_resolve_indirect_symbols(resolve_indirect_symbols), m_hit_counter() {}
Breakpoint::Breakpoint(Target &new_target, const Breakpoint &source_bp)
- : m_being_created(true), m_hardware(source_bp.m_hardware),
- m_target(new_target), m_name_list(source_bp.m_name_list),
- m_options(source_bp.m_options), m_locations(*this),
+ : m_hardware(source_bp.m_hardware), m_target(new_target),
+ m_name_list(source_bp.m_name_list), m_options(source_bp.m_options),
+ m_locations(*this),
m_resolve_indirect_symbols(source_bp.m_resolve_indirect_symbols),
m_hit_counter() {}
@@ -979,9 +976,8 @@ bool Breakpoint::EvaluatePrecondition(StoppointCallbackContext &context) {
void Breakpoint::SendBreakpointChangedEvent(
lldb::BreakpointEventType eventKind) {
- if (!m_being_created && !IsInternal() &&
- GetTarget().EventTypeHasListeners(
- Target::eBroadcastBitBreakpointChanged)) {
+ if (!IsInternal() && GetTarget().EventTypeHasListeners(
+ Target::eBroadcastBitBreakpointChanged)) {
std::shared_ptr<BreakpointEventData> data =
std::make_shared<BreakpointEventData>(eventKind, shared_from_this());
@@ -994,7 +990,7 @@ void Breakpoint::SendBreakpointChangedEvent(
if (!breakpoint_data_sp)
return;
- if (!m_being_created && !IsInternal() &&
+ if (!IsInternal() &&
GetTarget().EventTypeHasListeners(Target::eBroadcastBitBreakpointChanged))
GetTarget().BroadcastEvent(Target::eBroadcastBitBreakpointChanged,
breakpoint_data_sp);
diff --git a/lldb/source/Breakpoint/BreakpointLocation.cpp b/lldb/source/Breakpoint/BreakpointLocation.cpp
index 98de059c2e296..f7b8ca1f5506f 100644
--- a/lldb/source/Breakpoint/BreakpointLocation.cpp
+++ b/lldb/source/Breakpoint/BreakpointLocation.cpp
@@ -32,9 +32,9 @@ using namespace lldb_private;
BreakpointLocation::BreakpointLocation(break_id_t loc_id, Breakpoint &owner,
const Address &addr, lldb::tid_t tid,
bool hardware, bool check_for_resolver)
- : m_being_created(true), m_should_resolve_indirect_functions(false),
- m_is_reexported(false), m_is_indirect(false), m_address(addr),
- m_owner(owner), m_condition_hash(0), m_loc_id(loc_id), m_hit_counter() {
+ : m_should_resolve_indirect_functions(false), m_is_reexported(false),
+ m_is_indirect(false), m_address(addr), m_owner(owner),
+ m_condition_hash(0), m_loc_id(loc_id), m_hit_counter() {
if (check_for_resolver) {
Symbol *symbol = m_address.CalculateSymbolContextSymbol();
if (symbol && symbol->IsIndirect()) {
@@ -42,8 +42,7 @@ BreakpointLocation::BreakpointLocation(break_id_t loc_id, Breakpoint &owner,
}
}
- SetThreadID(tid);
- m_being_created = false;
+ SetThreadIDInternal(tid);
}
BreakpointLocation::~BreakpointLocation() { ClearBreakpointSite(); }
@@ -100,14 +99,7 @@ void BreakpointLocation::SetAutoContinue(bool auto_continue) {
}
void BreakpointLocation::SetThreadID(lldb::tid_t thread_id) {
- if (thread_id != LLDB_INVALID_THREAD_ID)
- GetLocationOptions().SetThreadID(thread_id);
- else {
- // If we're resetting this to an invalid thread id, then don't make an
- // options pointer just to do that.
- if (m_options_up != nullptr)
- m_options_up->SetThreadID(thread_id);
- }
+ SetThreadIDInternal(thread_id);
SendBreakpointLocationChangedEvent(eBreakpointEventTypeThreadChanged);
}
@@ -646,9 +638,8 @@ void BreakpointLocation::Dump(Stream *s) const {
void BreakpointLocation::SendBreakpointLocationChangedEvent(
lldb::BreakpointEventType eventKind) {
- if (!m_being_created && !m_owner.IsInternal() &&
- m_owner.GetTarget().EventTypeHasListeners(
- Target::eBroadcastBitBreakpointChanged)) {
+ if (!m_owner.IsInternal() && m_owner.GetTarget().EventTypeHasListeners(
+ Target::eBroadcastBitBreakpointChanged)) {
auto data_sp = std::make_shared<Breakpoint::BreakpointEventData>(
eventKind, m_owner.shared_from_this());
data_sp->GetBreakpointLocationCollection().Add(shared_from_this());
@@ -665,3 +656,14 @@ void BreakpointLocation::SwapLocation(BreakpointLocationSP swap_from) {
m_is_indirect = swap_from->m_is_indirect;
m_user_expression_sp.reset();
}
+
+void BreakpointLocation::SetThreadIDInternal(lldb::tid_t thread_id) {
+ if (thread_id != LLDB_INVALID_THREAD_ID)
+ GetLocationOptions().SetThreadID(thread_id);
+ else {
+ // If we're resetting this to an invalid thread id, then don't make an
+ // options pointer just to do that.
+ if (m_options_up != nullptr)
+ m_options_up->SetThreadID(thread_id);
+ }
+}
diff --git a/lldb/source/Breakpoint/Watchpoint.cpp b/lldb/source/Breakpoint/Watchpoint.cpp
index 1a8ef87c1e67a..79f5741bc1052 100644
--- a/lldb/source/Breakpoint/Watchpoint.cpp
+++ b/lldb/source/Breakpoint/Watchpoint.cpp
@@ -31,8 +31,7 @@ Watchpoint::Watchpoint(Target &target, lldb::addr_t addr, uint32_t size,
: StoppointSite(0, addr, size, hardware), m_target(target),
m_enabled(false), m_is_hardware(hardware), m_is_watch_variable(false),
m_is_ephemeral(false), m_disabled_count(0), m_watch_read(0),
- m_watch_write(0), m_watch_modify(0), m_ignore_count(0),
- m_being_created(true) {
+ m_watch_write(0), m_watch_modify(0), m_ignore_count(0) {
if (type && type->IsValid())
m_type = *type;
@@ -60,7 +59,6 @@ Watchpoint::Watchpoint(Target &target, lldb::addr_t addr, uint32_t size,
m_target.GetProcessSP()->CalculateExecutionContext(exe_ctx);
CaptureWatchedValue(exe_ctx);
}
- m_being_created = false;
}
Watchpoint::~Watchpoint() = default;
@@ -462,8 +460,8 @@ const char *Watchpoint::GetConditionText() const {
void Watchpoint::SendWatchpointChangedEvent(
lldb::WatchpointEventType eventKind) {
- if (!m_being_created && GetTarget().EventTypeHasListeners(
- Target::eBroadcastBitWatchpointChanged)) {
+ if (GetTarget().EventTypeHasListeners(
+ Target::eBroadcastBitWatchpointChanged)) {
auto data_sp =
std::make_shared<WatchpointEventData>(eventKind, shared_from_this());
GetTarget().BroadcastEvent(Target::eBroadcastBitWatchpointChanged, data_sp);
>From 54cd33c073568b47e620b22a65dc35be57cd78d6 Mon Sep 17 00:00:00 2001
From: Alex Langford <alangford at apple.com>
Date: Mon, 29 Jan 2024 10:31:14 -0800
Subject: [PATCH 2/3] Add doxygen comment
---
lldb/include/lldb/Breakpoint/BreakpointLocation.h | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/lldb/include/lldb/Breakpoint/BreakpointLocation.h b/lldb/include/lldb/Breakpoint/BreakpointLocation.h
index 273132c950825..9b07957bef400 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointLocation.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointLocation.h
@@ -313,6 +313,15 @@ class BreakpointLocation
void UndoBumpHitCount();
+ /// Updates the thread ID internally.
+ ///
+ /// This method was created to handle actually mutating the thread ID
+ /// internally because SetThreadID broadcasts an event in addition to mutating
+ /// state. If the BreakpointLocation is in the middle of being created,
+ /// broadcasting an event will crash LLDB.
+ ///
+ /// \param[in] thread_id
+ /// The new thread ID.
void SetThreadIDInternal(lldb::tid_t thread_id);
// Constructors and Destructors
>From ce3ab7d3eae60fcc90e6c9408113706e39e2efe6 Mon Sep 17 00:00:00 2001
From: Alex Langford <alangford at apple.com>
Date: Mon, 29 Jan 2024 14:15:57 -0800
Subject: [PATCH 3/3] reword docs
---
lldb/include/lldb/Breakpoint/BreakpointLocation.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lldb/include/lldb/Breakpoint/BreakpointLocation.h b/lldb/include/lldb/Breakpoint/BreakpointLocation.h
index 9b07957bef400..cca00335bc3c6 100644
--- a/lldb/include/lldb/Breakpoint/BreakpointLocation.h
+++ b/lldb/include/lldb/Breakpoint/BreakpointLocation.h
@@ -317,8 +317,8 @@ class BreakpointLocation
///
/// This method was created to handle actually mutating the thread ID
/// internally because SetThreadID broadcasts an event in addition to mutating
- /// state. If the BreakpointLocation is in the middle of being created,
- /// broadcasting an event will crash LLDB.
+ /// state. The constructor calls this instead of SetThreadID to avoid the
+ /// broadcast.
///
/// \param[in] thread_id
/// The new thread ID.
More information about the lldb-commits
mailing list