[lldb] Implement bidirectional access for backing<->backed thread relationship

[felipepiovezan]

This enables finding the backed thread from the backing thread without going through the thread list, and it will be useful for subsequent commits.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

This enables finding the backed thread from the backing thread without going through the thread list, and it will be useful for subsequent commits.

---

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

5 Files Affected:

• (modified) lldb/include/lldb/Target/Thread.h (+15)
• (modified) lldb/include/lldb/Target/ThreadList.h (-2)
• (modified) lldb/source/Plugins/Process/Utility/ThreadMemory.h (+6-1)
• (modified) lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp (+1-1)
• (modified) lldb/source/Target/ThreadList.cpp (-14)

diff --git a/lldb/include/lldb/Target/Thread.h b/lldb/include/lldb/Target/Thread.h
index ef66fa11574db9..d5fe33586fa3cf 100644
--- a/lldb/include/lldb/Target/Thread.h
+++ b/lldb/include/lldb/Target/Thread.h
@@ -470,6 +470,18 @@ class Thread : public std::enable_shared_from_this<Thread>,
 
   virtual void ClearStackFrames();
 
+  /// Derived classes implementing SetBackingThread should use this to provide
+  /// bidirectional access to the Backing-Backed relationship.
+  void SetBackedThread(Thread &backed_thread) {
+    assert(backed_thread.GetBackingThread().get() == this);
+    m_backed_thread = backed_thread.shared_from_this();
+  }
+
+  void ClearBackedThread() { m_backed_thread.reset(); }
+
+  /// Returns the thread that is backed by this thread, if any.
+  lldb::ThreadSP GetBackedThread() const { return m_backed_thread; }
+
   virtual bool SetBackingThread(const lldb::ThreadSP &thread_sp) {
     return false;
   }
@@ -1349,6 +1361,9 @@ class Thread : public std::enable_shared_from_this<Thread>,
   LazyBool m_override_should_notify;
   mutable std::unique_ptr<ThreadPlanStack> m_null_plan_stack_up;
 
+  /// The Thread backed by this thread, if any.
+  lldb::ThreadSP m_backed_thread;
+
 private:
   bool m_extended_info_fetched; // Have we tried to retrieve the m_extended_info
                                 // for this thread?
diff --git a/lldb/include/lldb/Target/ThreadList.h b/lldb/include/lldb/Target/ThreadList.h
index f931bb83a8ceaf..bca01f5fe2083e 100644
--- a/lldb/include/lldb/Target/ThreadList.h
+++ b/lldb/include/lldb/Target/ThreadList.h
@@ -101,8 +101,6 @@ class ThreadList : public ThreadCollection {
 
   lldb::ThreadSP GetThreadSPForThreadPtr(Thread *thread_ptr);
 
-  lldb::ThreadSP GetBackingThread(const lldb::ThreadSP &real_thread);
-
   bool ShouldStop(Event *event_ptr);
 
   Vote ShouldReportStop(Event *event_ptr);
diff --git a/lldb/source/Plugins/Process/Utility/ThreadMemory.h b/lldb/source/Plugins/Process/Utility/ThreadMemory.h
index d124f5780ea9bd..1e309671e85c65 100644
--- a/lldb/source/Plugins/Process/Utility/ThreadMemory.h
+++ b/lldb/source/Plugins/Process/Utility/ThreadMemory.h
@@ -72,12 +72,17 @@ class ThreadMemory : public lldb_private::Thread {
 
   void ClearStackFrames() override;
 
-  void ClearBackingThread() override { m_backing_thread_sp.reset(); }
+  void ClearBackingThread() override {
+    if (m_backing_thread_sp)
+      m_backing_thread_sp->ClearBackedThread();
+    m_backing_thread_sp.reset();
+  }
 
   bool SetBackingThread(const lldb::ThreadSP &thread_sp) override {
     // printf ("Thread 0x%llx is being backed by thread 0x%llx\n", GetID(),
     // thread_sp->GetID());
     m_backing_thread_sp = thread_sp;
+    thread_sp->SetBackedThread(*this);
     return (bool)thread_sp;
   }
 
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
index 21a0fa283644d6..3f34ea2930a66a 100644
--- a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -1728,7 +1728,7 @@ ThreadSP ProcessGDBRemote::SetThreadStopInfo(
     thread_sp->SetStopInfo(StopInfoSP());
     // If there's a memory thread backed by this thread, we need to use it to
     // calculate StopInfo.
-    if (ThreadSP memory_thread_sp = m_thread_list.GetBackingThread(thread_sp))
+    if (ThreadSP memory_thread_sp = thread_sp->GetBackedThread())
       thread_sp = memory_thread_sp;
 
     if (exc_type != 0) {
diff --git a/lldb/source/Target/ThreadList.cpp b/lldb/source/Target/ThreadList.cpp
index 6cbef330bf4888..c0440d82fd1ffc 100644
--- a/lldb/source/Target/ThreadList.cpp
+++ b/lldb/source/Target/ThreadList.cpp
@@ -191,20 +191,6 @@ ThreadSP ThreadList::GetThreadSPForThreadPtr(Thread *thread_ptr) {
   return thread_sp;
 }
 
-ThreadSP ThreadList::GetBackingThread(const ThreadSP &real_thread) {
-  std::lock_guard<std::recursive_mutex> guard(GetMutex());
-
-  ThreadSP thread_sp;
-  const uint32_t num_threads = m_threads.size();
-  for (uint32_t idx = 0; idx < num_threads; ++idx) {
-    if (m_threads[idx]->GetBackingThread() == real_thread) {
-      thread_sp = m_threads[idx];
-      break;
-    }
-  }
-  return thread_sp;
-}
-
 ThreadSP ThreadList::FindThreadByIndexID(uint32_t index_id, bool can_update) {
   std::lock_guard<std::recursive_mutex> guard(GetMutex());
 

[felipepiovezan]

I really wanted to make ClearBackedThread a protected member of Thread, but then we can't access it here. Protected only works if you're going through this.

[jimingham]

You have an assert that's requiring that backed_thread has to have had its backing thread set first, then passed to this API. Would it be a more user-friendly API if it set the backed_thread's backing thread here, if it isn't already set.
It's still fine to assert if somebody set the wrong backing thread, that seems like an unintended error. But if the backed_thread.GetBackingThread was null, is there any reason not to set it for them here?

[felipepiovezan]

Good point, this also addresses my concern of people calling SetBackedThread without also calling SetBackingThread. Will update.

[jimingham]

LGTM

[labath]

This creates a shared_pointer loop between the two objects, which doesn't sound like a good design. Maybe one of the pointers ought to be weak?

[felipepiovezan]

Reference in new issue

This creates a shared_pointer loop between the two objects, which doesn't sound like a good design. Maybe one of the pointers ought to be weak?

great catch!, I'll fix that

[felipepiovezan]

Turns out github has been stuck for hours without picking up my pushed fixup commit :/

[felipepiovezan]

Forcing push seems to have made it work