[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 10:31:48 PST 2024


https://github.com/bulbazord updated https://github.com/llvm/llvm-project/pull/79716

>From fc0c5da871d32a7cf5a6c96ef7f36c49aeccd074 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/2] [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 8c4308ab0bc12d..0b6bf593be37a9 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 2a4f9fc01bf326..273132c950825a 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 22fdfd686c3f11..fc11a466ba6054 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 af5dcc9cd88d4f..ae845e92762b97 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 98de059c2e2967..f7b8ca1f5506f3 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 1a8ef87c1e67a3..79f5741bc10520 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 12e224a046efb5b78b88488b99f6a39c4c3feb6c 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/2] 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 273132c950825a..9b07957bef4000 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



More information about the lldb-commits mailing list