[Lldb-commits] [lldb] [lldb][nfc] Improve duplicated code in unexecuted breakpoint detection (PR #128724)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

<details>
<summary>Changes</summary>

This code is replicated in multiple places, and a subsequent commit would introduce another copy of it in ThreadMemory.

---
Full diff: https://github.com/llvm/llvm-project/pull/128724.diff


5 Files Affected:

- (modified) lldb/include/lldb/Target/Thread.h (+6-5) 
- (modified) lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp (+1-5) 
- (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+1-4) 
- (modified) lldb/source/Plugins/Process/scripted/ScriptedThread.cpp (+1-10) 
- (modified) lldb/source/Target/Thread.cpp (+13) 


``````````diff
diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h
index 1d1e3dcfc1dc6..d1988407965da 100644
--- a/lldb/include/lldb/Target/Thread.h
+++ b/lldb/include/lldb/Target/Thread.h
@@ -385,21 +385,22 @@ class Thread : public std::enable_shared_from_this<Thread>,
   virtual void SetQueueLibdispatchQueueAddress(lldb::addr_t dispatch_queue_t) {}
 
   /// When a thread stops at an enabled BreakpointSite that has not executed,
-  /// the Process plugin should call SetThreadStoppedAtUnexecutedBP(pc).
+  /// the Process plugin should call DetectThreadStoppedAtUnexecutedBP().
   /// If that BreakpointSite was actually triggered (the instruction was
   /// executed, for a software breakpoint), regardless of whether the
   /// breakpoint is valid for this thread, SetThreadHitBreakpointSite()
   /// should be called to record that fact.
   ///
   /// Depending on the structure of the Process plugin, it may be easiest
-  /// to call SetThreadStoppedAtUnexecutedBP(pc) unconditionally when at
+  /// to call DetectThreadStoppedAtUnexecutedBP() unconditionally when at
   /// a BreakpointSite, and later when it is known that it was triggered,
   /// SetThreadHitBreakpointSite() can be called.  These two methods
   /// overwrite the same piece of state in the Thread, the last one
   /// called on a Thread wins.
-  void SetThreadStoppedAtUnexecutedBP(lldb::addr_t pc) {
-    m_stopped_at_unexecuted_bp = pc;
-  }
+  ///
+  /// Returns the BreakpointSite this thread is stopped at, if any.
+  lldb::BreakpointSiteSP DetectThreadStoppedAtUnexecutedBP();
+
   void SetThreadHitBreakpointSite() {
     m_stopped_at_unexecuted_bp = LLDB_INVALID_ADDRESS;
   }
diff --git a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
index 29a64a2a03bf0..daa7ccc2086c3 100644
--- a/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
+++ b/lldb/source/Plugins/Process/Utility/StopInfoMachException.cpp
@@ -591,11 +591,7 @@ StopInfoSP StopInfoMachException::CreateStopReasonWithMachException(
   // breakpoint at 0x100.
   // The fact that the pc may be off by one at this point
   // (for an x86 KDP breakpoint hit) is not a problem.
-  addr_t pc = reg_ctx_sp->GetPC();
-  BreakpointSiteSP bp_site_sp =
-      process_sp->GetBreakpointSiteList().FindByAddress(pc);
-  if (bp_site_sp && bp_site_sp->IsEnabled())
-    thread.SetThreadStoppedAtUnexecutedBP(pc);
+  BreakpointSiteSP bp_site_sp = thread.DetectThreadStoppedAtUnexecutedBP();
 
   switch (exc_type) {
   case 1: // EXC_BAD_ACCESS
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index 8a8c0f92fbbc2..4ce543f8810a3 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1706,11 +1706,8 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
   if (!thread_sp->StopInfoIsUpToDate()) {
     thread_sp->SetStopInfo(StopInfoSP());
 
-    addr_t pc = thread_sp->GetRegisterContext()->GetPC();
     BreakpointSiteSP bp_site_sp =
-        thread_sp->GetProcess()->GetBreakpointSiteList().FindByAddress(pc);
-    if (bp_site_sp && bp_site_sp->IsEnabled())
-      thread_sp->SetThreadStoppedAtUnexecutedBP(pc);
+        thread_sp->DetectThreadStoppedAtUnexecutedBP();
 
     if (exc_type != 0) {
       // For thread plan async interrupt, creating stop info on the
diff --git a/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp b/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
index d0d1508e85172..12474402af909 100644
--- a/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
+++ b/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp
@@ -229,16 +229,7 @@ bool ScriptedThread::CalculateStopInfo() {
         LLVM_PRETTY_FUNCTION, "Failed to get scripted thread stop info.", error,
         LLDBLog::Thread);
 
-  // If we're at a BreakpointSite, mark that we stopped there and
-  // need to hit the breakpoint when we resume.  This will be cleared
-  // if we CreateStopReasonWithBreakpointSiteID.
-  if (RegisterContextSP reg_ctx_sp = GetRegisterContext()) {
-    addr_t pc = reg_ctx_sp->GetPC();
-    if (BreakpointSiteSP bp_site_sp =
-            GetProcess()->GetBreakpointSiteList().FindByAddress(pc))
-      if (bp_site_sp->IsEnabled())
-        SetThreadStoppedAtUnexecutedBP(pc);
-  }
+  DetectThreadStoppedAtUnexecutedBP();
 
   lldb::StopInfoSP stop_info_sp;
   lldb::StopReason stop_reason_type;
diff --git a/lldb/source/Target/Thread.cpp b/lldb/source/Target/Thread.cpp
index 50f7c73f2c4c1..7ece48ab9e8e2 100644
--- a/lldb/source/Target/Thread.cpp
+++ b/lldb/source/Target/Thread.cpp
@@ -2108,3 +2108,16 @@ lldb::ValueObjectSP Thread::GetSiginfoValue() {
     process_sp->GetByteOrder(), arch.GetAddressByteSize()};
   return ValueObjectConstResult::Create(&target, type, ConstString("__lldb_siginfo"), data_extractor);
 }
+
+BreakpointSiteSP Thread::DetectThreadStoppedAtUnexecutedBP() {
+  if (RegisterContextSP reg_ctx_sp = GetRegisterContext()) {
+    addr_t pc = reg_ctx_sp->GetPC();
+    BreakpointSiteSP bp_site_sp =
+        GetProcess()->GetBreakpointSiteList().FindByAddress(pc);
+    if (bp_site_sp && bp_site_sp->IsEnabled()) {
+      m_stopped_at_unexecuted_bp = pc;
+      return bp_site_sp;
+    }
+  }
+  return nullptr;
+}

``````````

</details>


https://github.com/llvm/llvm-project/pull/128724