Use StoreTargetById/RetrieveTargetById and add more comments

Author: qxy11

lldb/test/API/tools/lldb-dap/attach/TestDAP_attach.py

@@ -83,5 +83,5 @@ def test_attach_with_invalid_targetId(self):
         resp = self.attach(targetId=99999, expectFailure=True)
         self.assertFalse(resp["success"])
         self.assertIn(
-            "Unable to find existing debugger", resp["body"]["error"]["format"]
+            "Unable to find existing target", resp["body"]["error"]["format"]
         )

lldb/tools/lldb-dap/DAP.cpp

@@ -1312,14 +1312,15 @@ void DAP::StartEventThreads() {
 llvm::Error DAP::InitializeDebugger(std::optional<uint32_t> target_id) {
   // Initialize debugger instance (shared or individual).
   if (target_id) {
-    std::optional<lldb::SBDebugger> shared_debugger =
-        DAPSessionManager::GetInstance().GetSharedDebugger(*target_id);
-    // If the target ID is not valid, then we won't find a debugger.
-    if (!shared_debugger) {
+    std::optional<lldb::SBTarget> shared_target =
+        DAPSessionManager::GetInstance().TakeTargetById(*target_id);
+    // If the target ID is not valid, then we won't find a target.
+    if (!shared_target) {
       return llvm::createStringError(
-          "Unable to find existing debugger for target ID");
+          "Unable to find existing target for target ID");
     }
-    debugger = shared_debugger.value();
+    // Get the debugger from the target and set it up.
+    debugger = shared_target->GetDebugger();
     StartEventThreads();
     return llvm::Error::success();
   }
@@ -1567,10 +1568,9 @@ void DAP::EventThread() {
         } else if (event_mask & lldb::SBTarget::eBroadcastBitNewTargetCreated) {
           auto target = lldb::SBTarget::GetTargetFromEvent(event);
 
-          // Generate unique target ID and set the shared debugger.
+          // Generate unique target ID and store the target for handoff.
           uint32_t target_id = target.GetGloballyUniqueID();
-          DAPSessionManager::GetInstance().SetSharedDebugger(target_id,
-                                                             debugger);
+          DAPSessionManager::GetInstance().StoreTargetById(target_id, target);
 
           // We create an attach config that will select the unique
           // target ID of the created target. The DAP instance will attach to

lldb/tools/lldb-dap/DAPSessionManager.cpp

@@ -31,10 +31,10 @@ ManagedEventThread::~ManagedEventThread() {
 }
 
 DAPSessionManager &DAPSessionManager::GetInstance() {
-  // NOTE: intentional leak to avoid issues with C++ destructor chain
-  // Use std::call_once for thread-safe initialization.
   static std::once_flag initialized;
-  static DAPSessionManager *instance = nullptr;
+  static DAPSessionManager *instance =
+      nullptr; // NOTE: intentional leak to avoid issues with C++ destructor
+               // chain
 
   std::call_once(initialized, []() { instance = new DAPSessionManager(); });
 
@@ -104,7 +104,7 @@ DAPSessionManager::GetEventThreadForDebugger(lldb::SBDebugger debugger,
   // Try to use shared event thread, if it exists.
   if (auto it = m_debugger_event_threads.find(debugger_id);
       it != m_debugger_event_threads.end()) {
-    if (auto thread_sp = it->second.lock())
+    if (std::shared_ptr<ManagedEventThread> thread_sp = it->second.lock())
       return thread_sp;
     // Our weak pointer has expired.
     m_debugger_event_threads.erase(it);
@@ -118,21 +118,21 @@ DAPSessionManager::GetEventThreadForDebugger(lldb::SBDebugger debugger,
   return new_thread_sp;
 }
 
-void DAPSessionManager::SetSharedDebugger(uint32_t target_id,
-                                          lldb::SBDebugger debugger) {
+void DAPSessionManager::StoreTargetById(uint32_t target_id,
+                                        lldb::SBTarget target) {
   std::lock_guard<std::mutex> lock(m_sessions_mutex);
-  m_target_to_debugger_map[target_id] = debugger;
+  m_target_map[target_id] = target;
 }
 
-std::optional<lldb::SBDebugger>
-DAPSessionManager::GetSharedDebugger(uint32_t target_id) {
+std::optional<lldb::SBTarget>
+DAPSessionManager::TakeTargetById(uint32_t target_id) {
   std::lock_guard<std::mutex> lock(m_sessions_mutex);
-  auto pos = m_target_to_debugger_map.find(target_id);
-  if (pos == m_target_to_debugger_map.end())
+  auto pos = m_target_map.find(target_id);
+  if (pos == m_target_map.end())
     return std::nullopt;
-  lldb::SBDebugger debugger = pos->second;
-  m_target_to_debugger_map.erase(pos);
-  return debugger;
+  lldb::SBTarget target = pos->second;
+  m_target_map.erase(pos);
+  return target;
 }
 
 DAP *DAPSessionManager::FindDAPForTarget(lldb::SBTarget target) {
@@ -146,9 +146,10 @@ DAP *DAPSessionManager::FindDAPForTarget(lldb::SBTarget target) {
 }
 
 void DAPSessionManager::CleanupSharedResources() {
-  // SBDebugger destructors will handle cleanup when the map entries are
+  // Note: Caller must hold m_sessions_mutex.
+  // SBTarget destructors will handle cleanup when the map entries are
   // destroyed.
-  m_target_to_debugger_map.clear();
+  m_target_map.clear();
 }
 
 void DAPSessionManager::ReleaseExpiredEventThreads() {

lldb/tools/lldb-dap/DAPSessionManager.h

@@ -43,7 +43,10 @@ class ManagedEventThread {
   std::thread m_event_thread;
 };
 
-/// Global DAP session manager.
+/// Global DAP session manager that manages multiple concurrent DAP sessions in
+/// a single lldb-dap process. Handles session lifecycle tracking, coordinates
+/// shared debugger event threads, and facilitates target handoff between
+/// sessions for dynamically created targets.
 class DAPSessionManager {
 public:
   /// Get the singleton instance of the DAP session manager.
@@ -52,24 +55,27 @@ class DAPSessionManager {
   /// Register a DAP session.
   void RegisterSession(lldb_private::MainLoop *loop, DAP *dap);
 
-  /// Unregister a DAP session.
+  /// Unregister a DAP session. Called by sessions when they complete their
+  /// disconnection, which unblocks WaitForAllSessionsToDisconnect().
   void UnregisterSession(lldb_private::MainLoop *loop);
 
   /// Get all active DAP sessions.
   std::vector<DAP *> GetActiveSessions();
 
-  /// Disconnect all active sessions.
+  /// Disconnect all registered sessions by calling Disconnect() on
+  /// each and requesting their event loops to terminate. Used during
+  /// shutdown to force all sessions to begin disconnecting.
   void DisconnectAllSessions();
 
-  /// Wait for all sessions to finish disconnecting.
-  /// Returns an error if any client disconnection failed, otherwise success.
+  /// Block until all sessions disconnect and unregister. Returns an error if
+  /// DisconnectAllSessions() was called and any disconnection failed.
   llvm::Error WaitForAllSessionsToDisconnect();
 
-  /// Set the shared debugger instance for a unique target ID.
-  void SetSharedDebugger(uint32_t target_id, lldb::SBDebugger debugger);
+  /// Store a target for later retrieval by another session.
+  void StoreTargetById(uint32_t target_id, lldb::SBTarget target);
 
-  /// Get the shared debugger instance for a unique target ID.
-  std::optional<lldb::SBDebugger> GetSharedDebugger(uint32_t target_id);
+  /// Retrieve and remove a stored target by its globally unique target ID.
+  std::optional<lldb::SBTarget> TakeTargetById(uint32_t target_id);
 
   /// Get or create event thread for a specific debugger.
   std::shared_ptr<ManagedEventThread>
@@ -104,12 +110,12 @@ class DAPSessionManager {
   std::condition_variable m_sessions_condition;
   std::map<lldb_private::MainLoop *, DAP *> m_active_sessions;
 
-  /// Optional map from target ID to shared debugger set when the native
-  /// process creates a new target.
-  std::map<uint32_t, lldb::SBDebugger> m_target_to_debugger_map;
+  /// Map from target ID to target for handing off newly created targets
+  /// between sessions.
+  std::map<uint32_t, lldb::SBTarget> m_target_map;
 
-  /// Map from debugger ID to its event thread used for when
-  /// multiple DAP sessions are using the same debugger instance.
+  /// Map from debugger ID to its event thread, used when multiple DAP sessions
+  /// share the same debugger instance.
   std::map<lldb::user_id_t, std::weak_ptr<ManagedEventThread>>
       m_debugger_event_threads;
 };

lldb/unittests/DAP/DAPSessionManagerTest.cpp

@@ -25,58 +25,77 @@ TEST_F(DAPSessionManagerTest, GetInstanceReturnsSameSingleton) {
   EXPECT_EQ(&instance1, &instance2);
 }
 
-TEST_F(DAPSessionManagerTest, SharedDebuggerSetAndGet) {
+TEST_F(DAPSessionManagerTest, StoreAndTakeTarget) {
   DAPSessionManager &manager = DAPSessionManager::GetInstance();
 
   SBDebugger debugger = SBDebugger::Create();
   ASSERT_TRUE(debugger.IsValid());
+  // Create an empty target (no executable) since we're only testing session
+  // management functionality, not actual debugging operations.
+  SBTarget target = debugger.CreateTarget("");
+  ASSERT_TRUE(target.IsValid());
 
-  uint32_t target_id = 12345;
-  manager.SetSharedDebugger(target_id, debugger);
+  uint32_t target_id = target.GetGloballyUniqueID();
+  manager.StoreTargetById(target_id, target);
 
-  // Retrieval consumes the debugger (removes from map).
-  std::optional<SBDebugger> retrieved = manager.GetSharedDebugger(target_id);
+  // Retrieval consumes the target (removes from map).
+  std::optional<SBTarget> retrieved = manager.TakeTargetById(target_id);
   ASSERT_TRUE(retrieved.has_value());
   EXPECT_TRUE(retrieved->IsValid());
-  EXPECT_EQ(retrieved->GetID(), debugger.GetID());
+  // Verify we got the correct target back.
+  EXPECT_EQ(retrieved->GetDebugger().GetID(), debugger.GetID());
+  EXPECT_EQ(*retrieved, target);
 
-  // Second retrieval should fail.
-  std::optional<SBDebugger> second_retrieval =
-      manager.GetSharedDebugger(target_id);
+  // Second retrieval should fail (one-time retrieval semantics).
+  std::optional<SBTarget> second_retrieval = manager.TakeTargetById(target_id);
   EXPECT_FALSE(second_retrieval.has_value());
 
   SBDebugger::Destroy(debugger);
 }
 
-TEST_F(DAPSessionManagerTest, GetSharedDebuggerWithInvalidId) {
+TEST_F(DAPSessionManagerTest, GetTargetWithInvalidId) {
   DAPSessionManager &manager = DAPSessionManager::GetInstance();
 
-  std::optional<SBDebugger> result = manager.GetSharedDebugger(99999);
+  std::optional<SBTarget> result = manager.TakeTargetById(99999);
 
   EXPECT_FALSE(result.has_value());
 }
 
-TEST_F(DAPSessionManagerTest, MultipleSharedDebuggersWithDifferentIds) {
+TEST_F(DAPSessionManagerTest, MultipleTargetsWithDifferentIds) {
   DAPSessionManager &manager = DAPSessionManager::GetInstance();
 
   SBDebugger debugger1 = SBDebugger::Create();
   SBDebugger debugger2 = SBDebugger::Create();
   ASSERT_TRUE(debugger1.IsValid());
   ASSERT_TRUE(debugger2.IsValid());
+  // Create empty targets (no executable) since we're only testing session
+  // management functionality, not actual debugging operations.
+  SBTarget target1 = debugger1.CreateTarget("");
+  SBTarget target2 = debugger2.CreateTarget("");
+  ASSERT_TRUE(target1.IsValid());
+  ASSERT_TRUE(target2.IsValid());
 
-  uint32_t target_id_1 = 1001;
-  uint32_t target_id_2 = 1002;
+  uint32_t target_id_1 = target1.GetGloballyUniqueID();
+  uint32_t target_id_2 = target2.GetGloballyUniqueID();
 
-  manager.SetSharedDebugger(target_id_1, debugger1);
-  manager.SetSharedDebugger(target_id_2, debugger2);
+  // Sanity check the targets have distinct IDs.
+  EXPECT_NE(target_id_1, target_id_2);
 
-  std::optional<SBDebugger> retrieved1 = manager.GetSharedDebugger(target_id_1);
+  manager.StoreTargetById(target_id_1, target1);
+  manager.StoreTargetById(target_id_2, target2);
+
+  std::optional<SBTarget> retrieved1 = manager.TakeTargetById(target_id_1);
   ASSERT_TRUE(retrieved1.has_value());
-  EXPECT_EQ(retrieved1->GetID(), debugger1.GetID());
+  EXPECT_TRUE(retrieved1->IsValid());
+  // Verify we got the correct target by comparing debugger and target IDs.
+  EXPECT_EQ(retrieved1->GetDebugger().GetID(), debugger1.GetID());
+  EXPECT_EQ(*retrieved1, target1);
 
-  std::optional<SBDebugger> retrieved2 = manager.GetSharedDebugger(target_id_2);
+  std::optional<SBTarget> retrieved2 = manager.TakeTargetById(target_id_2);
   ASSERT_TRUE(retrieved2.has_value());
-  EXPECT_EQ(retrieved2->GetID(), debugger2.GetID());
+  EXPECT_TRUE(retrieved2->IsValid());
+  EXPECT_EQ(retrieved2->GetDebugger().GetID(), debugger2.GetID());
+  EXPECT_EQ(*retrieved2, target2);
 
   SBDebugger::Destroy(debugger1);
   SBDebugger::Destroy(debugger2);
@@ -87,14 +106,32 @@ TEST_F(DAPSessionManagerTest, CleanupSharedResources) {
 
   SBDebugger debugger = SBDebugger::Create();
   ASSERT_TRUE(debugger.IsValid());
-  manager.SetSharedDebugger(5555, debugger);
-  std::optional<SBDebugger> initial_result = manager.GetSharedDebugger(5555);
-  ASSERT_TRUE(initial_result.has_value());
+  // Create empty targets (no executable) since we're only testing session
+  // management functionality, not actual debugging operations.
+  SBTarget target1 = debugger.CreateTarget("");
+  SBTarget target2 = debugger.CreateTarget("");
+  ASSERT_TRUE(target1.IsValid());
+  ASSERT_TRUE(target2.IsValid());
+
+  uint32_t target_id_1 = target1.GetGloballyUniqueID();
+  uint32_t target_id_2 = target2.GetGloballyUniqueID();
+
+  // Sanity check the targets have distinct IDs.
+  EXPECT_NE(target_id_1, target_id_2);
 
+  // Store multiple targets to verify cleanup removes them all.
+  manager.StoreTargetById(target_id_1, target1);
+  manager.StoreTargetById(target_id_2, target2);
+
+  // Cleanup should remove all stored targets.
   manager.CleanupSharedResources();
 
-  std::optional<SBDebugger> result = manager.GetSharedDebugger(5555);
-  EXPECT_FALSE(result.has_value());
+  // Verify both targets are gone after cleanup.
+  std::optional<SBTarget> result1 = manager.TakeTargetById(target_id_1);
+  EXPECT_FALSE(result1.has_value());
+
+  std::optional<SBTarget> result2 = manager.TakeTargetById(target_id_2);
+  EXPECT_FALSE(result2.has_value());
 
   SBDebugger::Destroy(debugger);
 }