[lldb][nfc] Improve duplicated code in unexecuted breakpoint detection

[felipepiovezan]

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

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

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 --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;
+}

[JDevlieghere]

Nice cleanup!

[jasonmolenda]

Good idea. I think you need to also update ProcessWindows.cpp?

I'm still a little unsatisfied with my approach of "a thread calls DetectThreadStoppedAtUnexecutedBP() and if it turns out it DID execute the breakpoint, then calls SetThreadHitBreakpointSite(). And in fact, if you do it the other way around -- if a method ends up calling SetThreadHitBreakpointSite() and then DetectThreadStoppedAtUnexecutedBP(), it will lose the fact that it hit the breakpoint and well hit it again on resume, another thing I'm not thrilled about. But none of that is relevant to this change here.