[lldb] Fix missing overloads in ThreadMemory #132734

felipepiovezan · 2025-03-24T13:26:40Z

Please read the two commit messages individually.

llvmbot · 2025-03-24T13:27:16Z

@llvm/pr-subscribers-lldb

Author: Felipe de Azevedo Piovezan (felipepiovezan)

Changes

Please read the two commit messages individually.

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

3 Files Affected:

(modified) lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp (+2-2)
(modified) lldb/source/Plugins/Process/Utility/ThreadMemory.cpp (+11-12)
(modified) lldb/source/Plugins/Process/Utility/ThreadMemory.h (+165-19)

diff --git a/lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp b/lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
index aff521890858c..ef2291f5c1b3d 100644
--- a/lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
+++ b/lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp
@@ -259,8 +259,8 @@ ThreadSP OperatingSystemPython::CreateThreadFromThreadInfo(
   if (!thread_sp) {
     if (did_create_ptr)
       *did_create_ptr = true;
-    thread_sp = std::make_shared<ThreadMemory>(*m_process, tid, name, queue,
-                                               reg_data_addr);
+    thread_sp = std::make_shared<NamedThreadMemoryWithQueue>(
+        *m_process, tid, name, queue, reg_data_addr);
   }
 
   if (core_number < core_thread_list.GetSize(false)) {
diff --git a/lldb/source/Plugins/Process/Utility/ThreadMemory.cpp b/lldb/source/Plugins/Process/Utility/ThreadMemory.cpp
index 550b53688fd39..2824a943f757b 100644
--- a/lldb/source/Plugins/Process/Utility/ThreadMemory.cpp
+++ b/lldb/source/Plugins/Process/Utility/ThreadMemory.cpp
@@ -20,18 +20,17 @@
 using namespace lldb;
 using namespace lldb_private;
 
-ThreadMemory::ThreadMemory(Process &process, lldb::tid_t tid,
-                           const ValueObjectSP &thread_info_valobj_sp)
-    : Thread(process, tid), m_backing_thread_sp(),
-      m_thread_info_valobj_sp(thread_info_valobj_sp), m_name(), m_queue(),
-      m_register_data_addr(LLDB_INVALID_ADDRESS) {}
-
-ThreadMemory::ThreadMemory(Process &process, lldb::tid_t tid,
-                           llvm::StringRef name, llvm::StringRef queue,
-                           lldb::addr_t register_data_addr)
-    : Thread(process, tid), m_backing_thread_sp(), m_thread_info_valobj_sp(),
-      m_name(std::string(name)), m_queue(std::string(queue)),
-      m_register_data_addr(register_data_addr) {}
+NamedThreadMemoryWithQueue::NamedThreadMemoryWithQueue(
+    Process &process, lldb::tid_t tid,
+    const ValueObjectSP &thread_info_valobj_sp)
+    : NamedThreadMemory(process, tid, LLDB_INVALID_ADDRESS, ""),
+      m_thread_info_valobj_sp(thread_info_valobj_sp), m_queue() {}
+
+NamedThreadMemoryWithQueue::NamedThreadMemoryWithQueue(
+    Process &process, lldb::tid_t tid, llvm::StringRef name,
+    llvm::StringRef queue, lldb::addr_t register_data_addr)
+    : NamedThreadMemory(process, tid, register_data_addr, name),
+      m_thread_info_valobj_sp(), m_queue(std::string(queue)) {}
 
 ThreadMemory::~ThreadMemory() { DestroyThread(); }
 
diff --git a/lldb/source/Plugins/Process/Utility/ThreadMemory.h b/lldb/source/Plugins/Process/Utility/ThreadMemory.h
index cebb31538eaf2..2d196c33d475d 100644
--- a/lldb/source/Plugins/Process/Utility/ThreadMemory.h
+++ b/lldb/source/Plugins/Process/Utility/ThreadMemory.h
@@ -13,14 +13,14 @@
 
 #include "lldb/Target/Thread.h"
 
+/// A memory thread optionally backed by a real thread.
+/// All methods of this class dispatch to the real thread, if it is not null.
+/// Otherwise the methods are no-op.
 class ThreadMemory : public lldb_private::Thread {
 public:
   ThreadMemory(lldb_private::Process &process, lldb::tid_t tid,
-               const lldb::ValueObjectSP &thread_info_valobj_sp);
-
-  ThreadMemory(lldb_private::Process &process, lldb::tid_t tid,
-               llvm::StringRef name, llvm::StringRef queue,
-               lldb::addr_t register_data_addr);
+               lldb::addr_t register_data_addr)
+      : Thread(process, tid), m_register_data_addr(register_data_addr) {}
 
   ~ThreadMemory() override;
 
@@ -38,16 +38,12 @@ class ThreadMemory : public lldb_private::Thread {
   }
 
   const char *GetName() override {
-    if (!m_name.empty())
-      return m_name.c_str();
     if (m_backing_thread_sp)
       return m_backing_thread_sp->GetName();
     return nullptr;
   }
 
   const char *GetQueueName() override {
-    if (!m_queue.empty())
-      return m_queue.c_str();
     if (m_backing_thread_sp)
       return m_backing_thread_sp->GetQueueName();
     return nullptr;
@@ -55,6 +51,69 @@ class ThreadMemory : public lldb_private::Thread {
 
   void WillResume(lldb::StateType resume_state) override;
 
+  void SetQueueName(const char *name) override {
+    if (m_backing_thread_sp)
+      m_backing_thread_sp->SetQueueName(name);
+  }
+
+  lldb::queue_id_t GetQueueID() override {
+    if (m_backing_thread_sp)
+      return m_backing_thread_sp->GetQueueID();
+    return LLDB_INVALID_QUEUE_ID;
+  }
+
+  void SetQueueID(lldb::queue_id_t new_val) override {
+    if (m_backing_thread_sp)
+      m_backing_thread_sp->GetQueueID();
+  }
+
+  lldb::QueueKind GetQueueKind() override {
+    if (m_backing_thread_sp)
+      return m_backing_thread_sp->GetQueueKind();
+    return lldb::eQueueKindUnknown;
+  }
+
+  void SetQueueKind(lldb::QueueKind kind) override {
+    if (m_backing_thread_sp)
+      m_backing_thread_sp->SetQueueKind(kind);
+  }
+
+  lldb::QueueSP GetQueue() override {
+    if (m_backing_thread_sp)
+      return m_backing_thread_sp->GetQueue();
+    return lldb::QueueSP();
+  }
+
+  lldb::addr_t GetQueueLibdispatchQueueAddress() override {
+    if (m_backing_thread_sp)
+      return m_backing_thread_sp->GetQueueLibdispatchQueueAddress();
+    return LLDB_INVALID_ADDRESS;
+  }
+
+  void SetQueueLibdispatchQueueAddress(lldb::addr_t dispatch_queue_t) override {
+    if (m_backing_thread_sp)
+      m_backing_thread_sp->SetQueueLibdispatchQueueAddress(dispatch_queue_t);
+  }
+
+  lldb_private::LazyBool GetAssociatedWithLibdispatchQueue() override {
+    if (m_backing_thread_sp)
+      return m_backing_thread_sp->GetAssociatedWithLibdispatchQueue();
+    return lldb_private::eLazyBoolNo;
+  }
+
+  void SetAssociatedWithLibdispatchQueue(
+      lldb_private::LazyBool associated_with_libdispatch_queue) override {
+    if (m_backing_thread_sp)
+      m_backing_thread_sp->SetAssociatedWithLibdispatchQueue(
+          associated_with_libdispatch_queue);
+  }
+
+  bool ThreadHasQueueInformation() const override {
+    if (m_backing_thread_sp)
+      return m_backing_thread_sp->ThreadHasQueueInformation();
+    return false;
+  }
+
   void DidResume() override {
     if (m_backing_thread_sp)
       m_backing_thread_sp->DidResume();
@@ -68,8 +127,6 @@ class ThreadMemory : public lldb_private::Thread {
 
   void RefreshStateAfterStop() override;
 
-  lldb::ValueObjectSP &GetValueObject() { return m_thread_info_valobj_sp; }
-
   void ClearStackFrames() override;
 
   void ClearBackingThread() override {
@@ -79,34 +136,123 @@ class ThreadMemory : public lldb_private::Thread {
   }
 
   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;
+    return thread_sp.get();
   }
 
   lldb::ThreadSP GetBackingThread() const override {
     return m_backing_thread_sp;
   }
 
-protected:
   bool IsOperatingSystemPluginThread() const override { return true; }
 
+private:
+  lldb::addr_t m_register_data_addr;
   // If this memory thread is actually represented by a thread from the
   // lldb_private::Process subclass, then fill in the thread here and
   // all APIs will be routed through this thread object. If m_backing_thread_sp
   // is empty, then this thread is simply in memory with no representation
   // through the process plug-in.
   lldb::ThreadSP m_backing_thread_sp;
-  lldb::ValueObjectSP m_thread_info_valobj_sp;
+
+  ThreadMemory(const ThreadMemory &) = delete;
+  const ThreadMemory &operator=(const ThreadMemory &) = delete;
+};
+
+class NamedThreadMemory : public ThreadMemory {
+public:
+  NamedThreadMemory(lldb_private::Process &process, lldb::tid_t tid,
+                    lldb::addr_t register_data_addr, llvm::StringRef name)
+      : ThreadMemory(process, tid, register_data_addr), m_name(name) {}
+
+  const char *GetName() override {
+    if (!m_name.empty())
+      return m_name.c_str();
+    return ThreadMemory::GetName();
+  }
+
+private:
   std::string m_name;
+};
+
+/// A NamedThreadMemory that has optional queue information.
+class NamedThreadMemoryWithQueue : public NamedThreadMemory {
+public:
+  NamedThreadMemoryWithQueue(lldb_private::Process &process, lldb::tid_t tid,
+                             const lldb::ValueObjectSP &thread_info_valobj_sp);
+
+  NamedThreadMemoryWithQueue(lldb_private::Process &process, lldb::tid_t tid,
+                             llvm::StringRef name, llvm::StringRef queue,
+                             lldb::addr_t register_data_addr);
+
+  ~NamedThreadMemoryWithQueue() override = default;
+
+  const char *GetQueueName() override {
+    if (!m_queue.empty())
+      return m_queue.c_str();
+    return ThreadMemory::GetQueueName();
+  }
+
+  /// This method has not yet been specialized.
+  void SetQueueName(const char *name) override { Thread::SetQueueName(name); }
+
+  /// This method has not yet been specialized.
+  lldb::queue_id_t GetQueueID() override { return Thread::GetQueueID(); }
+
+  /// This method has not yet been specialized.
+  void SetQueueID(lldb::queue_id_t new_val) override {
+    Thread::SetQueueID(new_val);
+  }
+
+  /// This method has not yet been specialized.
+  lldb::QueueKind GetQueueKind() override { return Thread::GetQueueKind(); }
+
+  /// This method has not yet been specialized.
+  void SetQueueKind(lldb::QueueKind kind) override {
+    Thread::SetQueueKind(kind);
+  }
+
+  /// This method has not yet been specialized.
+  lldb::QueueSP GetQueue() override { return Thread::GetQueue(); }
+
+  /// This method has not yet been specialized.
+  lldb::addr_t GetQueueLibdispatchQueueAddress() override {
+    return Thread::GetQueueLibdispatchQueueAddress();
+  }
+
+  /// This method has not yet been specialized.
+  void SetQueueLibdispatchQueueAddress(lldb::addr_t dispatch_queue_t) override {
+    Thread::SetQueueLibdispatchQueueAddress(dispatch_queue_t);
+  }
+
+  /// This method has not yet been specialized.
+  bool ThreadHasQueueInformation() const override {
+    return Thread::ThreadHasQueueInformation();
+  }
+
+  /// This method has not yet been specialized.
+  lldb_private::LazyBool GetAssociatedWithLibdispatchQueue() override {
+    return Thread::GetAssociatedWithLibdispatchQueue();
+  }
+
+  /// This method has not yet been specialized.
+  void SetAssociatedWithLibdispatchQueue(
+      lldb_private::LazyBool associated_with_libdispatch_queue) override {
+    Thread::SetAssociatedWithLibdispatchQueue(
+        associated_with_libdispatch_queue);
+  }
+
+  lldb::ValueObjectSP &GetValueObject() { return m_thread_info_valobj_sp; }
+
+protected:
+  lldb::ValueObjectSP m_thread_info_valobj_sp;
   std::string m_queue;
-  lldb::addr_t m_register_data_addr;
 
 private:
-  ThreadMemory(const ThreadMemory &) = delete;
-  const ThreadMemory &operator=(const ThreadMemory &) = delete;
+  NamedThreadMemoryWithQueue(const NamedThreadMemoryWithQueue &) = delete;
+  const NamedThreadMemoryWithQueue &
+  operator=(const NamedThreadMemoryWithQueue &) = delete;
 };
 
 #endif // LLDB_SOURCE_PLUGINS_PROCESS_UTILITY_THREADMEMORY_H

jimingham · 2025-03-24T21:09:36Z

lldb/source/Plugins/Process/Utility/ThreadMemory.h

  std::string m_name;
+};
+
+/// A NamedThreadMemory that has optional queue information.


has -> provides

Just to be consistent.

Looking into this a bit more, I went with /// A ThreadMemoryProvidingName that optionally overrides queue information. (and likewise for the other class)

jimingham · 2025-03-24T21:11:18Z

lldb/source/Plugins/Process/Utility/ThreadMemory.h

+  ThreadMemory(const ThreadMemory &) = delete;
+  const ThreadMemory &operator=(const ThreadMemory &) = delete;
+};
+


Should have a comment mirroring the ProvidingNameAndQueue

jimingham · 2025-03-24T21:12:27Z

lldb/source/Plugins/Process/Utility/ThreadMemory.h

@@ -13,14 +13,14 @@

 #include "lldb/Target/Thread.h"

+/// A memory thread optionally backed by a real thread.
+/// All methods of this class dispatch to the real thread, if it is not null.
+/// Otherwise the methods are no-op.


This isn't 100% true, the MemoryThread has a different TID from the backing thread (sometimes providing a stable TID is the ONLY job the MemoryThread is doing.

Yup, that's true. Let me try to reword this a bit.

jimingham · 2025-03-24T21:12:50Z

lldb/source/Plugins/Process/Utility/ThreadMemory.h

+    Thread::SetQueueKind(kind);
+  }
+
+  /// This method has not yet been specialized.


This is a bit mysterious, what do you mean by this?

I wanted to emphasize the inconsistency with the current implementation: this class can override queue information, but if we were to call this method, we would not get a queue related to this override. This method needs some implementation.
In fact, we're going to get a nullptr here. At some point we should look into what to do about it, but this patch does not change the existing behavior.

I reworded this in terms of a TODO.

medismailben · 2025-03-24T21:16:34Z

lldb/source/Plugins/Process/Utility/ThreadMemory.h

+    if (m_backing_thread_sp)
+      m_backing_thread_sp->GetQueueID();


Suggested change

if (m_backing_thread_sp)

m_backing_thread_sp->GetQueueID();

if (m_backing_thread_sp)

m_backing_thread_sp->SetQueueID(new_val);

great catch!

medismailben · 2025-03-24T21:18:03Z

lldb/source/Plugins/Process/Utility/ThreadMemory.h

+  const ThreadMemory &operator=(const ThreadMemory &) = delete;
+};
+
+class ThreadMemoryProvidingName : public ThreadMemory {


Some comments here would be nice.

ThreadMemory attempts to be a class abstracting the notion of a "fake" Thread that is backed by a "real" thread. According to its documentation, it is meant to be a class forwarding most methods to the backing thread, but it does so only for a handful of methods. Along the way, it also tries to represent a Thread that may or may not have a different name, and may or may not have a different queue from the backing thread. This can be problematic for a couple of reasons: 1. It forces all users into this optional behavior. 2. The forwarding behavior is incomplete: not all methods are currently being forwarded properly. Some of them involve queues and seem to have been intentionally left unimplemented. This commit creates the following separation: ThreadMemory <- ThreadMemoryProvidingName <- ThreadMemoryProvidingNameAndQueue ThreadMemory captures the notion of a backed thread that _really_ forwards all methods to the backing thread. (Missing methods should be implemented in a later commit, and allowing them to be implemented without changing behavior of other derived classes is the main purpose of this refactor). ThreadMemoryProvidingNameAndQueue is a ThreadMemory that allows users to override the thread name. If a name is present, it is used; otherwise the forwarding behavior is used. ThreadMemoryProvidingNameAndQueue is a ThreadMemoryProvidingName that allows users to override queue information. If queue information is present, it is used; otherwise, the forwarding behavior is used. With this separation, we can more explicitly implement missing methods of the base class and override them, if needed, in ThreadMemoryProvidingNameAndQueue. But this commit really is NFC, no new methods are implemented and no method implementation is changed.

This commit makes ThreadMemory a real "forwarder" class by implementing the missing queue methods: they will just call the corresponding backing thread method. To make this patch NFC(*) and not change the behavior of the Python OS plugin, NamedThreadMemoryWithQueue also overrides these methods to simply call the `Thread` method, just as it was doing before. This also makes it obvious that there are missing pieces of this class if it were to provide full queue support. (*) This patch is NFC in the sense that all llvm.org plugins will not have any behavior change, but downstream consumers of ThreadMemory will benefit from the newly implemented forwarding methods.

felipepiovezan

addressed review comments

felipepiovezan · 2025-03-24T23:24:07Z

lldb/source/Plugins/Process/Utility/ThreadMemory.h

@@ -13,14 +13,14 @@

 #include "lldb/Target/Thread.h"

+/// A memory thread optionally backed by a real thread.
+/// All methods of this class dispatch to the real thread, if it is not null.
+/// Otherwise the methods are no-op.


Yup, that's true. Let me try to reword this a bit.

felipepiovezan · 2025-03-24T23:57:17Z

lldb/source/Plugins/Process/Utility/ThreadMemory.h

  std::string m_name;
+};
+
+/// A NamedThreadMemory that has optional queue information.


Looking into this a bit more, I went with /// A ThreadMemoryProvidingName that optionally overrides queue information. (and likewise for the other class)

felipepiovezan · 2025-03-24T23:58:07Z

lldb/source/Plugins/Process/Utility/ThreadMemory.h

+    if (m_backing_thread_sp)
+      m_backing_thread_sp->GetQueueID();


great catch!

felipepiovezan · 2025-03-25T00:00:43Z

lldb/source/Plugins/Process/Utility/ThreadMemory.h

+    Thread::SetQueueKind(kind);
+  }
+
+  /// This method has not yet been specialized.


I wanted to emphasize the inconsistency with the current implementation: this class can override queue information, but if we were to call this method, we would not get a queue related to this override. This method needs some implementation.
In fact, we're going to get a nullptr here. At some point we should look into what to do about it, but this patch does not change the existing behavior.

felipepiovezan · 2025-03-25T00:02:13Z

lldb/source/Plugins/Process/Utility/ThreadMemory.h

+    Thread::SetQueueKind(kind);
+  }
+
+  /// This method has not yet been specialized.


I reworded this in terms of a TODO.

jimingham

LGTM

felipepiovezan · 2025-03-25T09:52:58Z

In order to preserve the two separate commits (and not squash them) I've merged this into two separate PRs:
#132905
#132906

So I'll close this now.

felipepiovezan requested a review from JDevlieghere as a code owner March 24, 2025 13:26

llvmbot added the lldb label Mar 24, 2025

felipepiovezan requested a review from jimingham March 24, 2025 17:01

felipepiovezan force-pushed the felipe/thread_memory_missing_methods branch from 36c9b8d to de3ba83 Compare March 24, 2025 19:39

jimingham reviewed Mar 24, 2025

View reviewed changes

medismailben reviewed Mar 24, 2025

View reviewed changes

felipepiovezan added 2 commits March 24, 2025 21:04

felipepiovezan force-pushed the felipe/thread_memory_missing_methods branch from de3ba83 to 211c82e Compare March 25, 2025 00:04

felipepiovezan commented Mar 25, 2025

View reviewed changes

jimingham approved these changes Mar 25, 2025

View reviewed changes

This was referenced Mar 25, 2025

[lldb][NFC] Break ThreadMemory into smaller abstractions #132905

Merged

[lldb] Implement missing queue overloads from ThreadMemory #132906

Merged

felipepiovezan closed this Mar 25, 2025

felipepiovezan deleted the felipe/thread_memory_missing_methods branch March 25, 2025 09:53

[lldb] Fix missing overloads in ThreadMemory #132734

[lldb] Fix missing overloads in ThreadMemory #132734

Uh oh!

Conversation

felipepiovezan commented Mar 24, 2025

Uh oh!

llvmbot commented Mar 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipepiovezan Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipepiovezan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipepiovezan Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimingham left a comment

Choose a reason for hiding this comment

Uh oh!

felipepiovezan commented Mar 25, 2025

Uh oh!

Uh oh!

felipepiovezan Mar 25, 2025 •

edited

Loading

felipepiovezan Mar 25, 2025 •

edited

Loading