Remove StoreTarget/TakeTarget

Author: qxy11

lldb/tools/lldb-dap/DAP.cpp

@@ -1309,18 +1309,34 @@ void DAP::StartEventThreads() {
   StartEventThread();
 }
 
-llvm::Error DAP::InitializeDebugger(std::optional<lldb::user_id_t> target_id) {
+llvm::Error DAP::InitializeDebugger(std::optional<int> debugger_id,
+                                    std::optional<lldb::user_id_t> target_id) {
+  // Validate that both debugger_id and target_id are provided together.
+  if (debugger_id.has_value() != target_id.has_value()) {
+    return llvm::createStringError(
+        "Both debuggerId and targetId must be specified together for debugger "
+        "reuse, or both must be omitted to create a new debugger");
+  }
+
   // Initialize debugger instance (shared or individual).
-  if (target_id) {
-    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) {
+  if (debugger_id && target_id) {
+    // Find the existing debugger by ID
+    debugger = lldb::SBDebugger::FindDebuggerWithID(*debugger_id);
+    if (!debugger.IsValid()) {
+      return llvm::createStringError(
+          "Unable to find existing debugger for debugger ID");
+    }
+
+    // Find the target within the debugger by its globally unique ID
+    lldb::SBTarget shared_target =
+        debugger.FindTargetByGloballyUniqueID(*target_id);
+    if (!shared_target.IsValid()) {
       return llvm::createStringError(
           "Unable to find existing target for target ID");
     }
-    // Get the debugger from the target and set it up.
-    debugger = shared_target->GetDebugger();
+
+    // Set the target for this DAP session.
+    SetTarget(shared_target);
     StartEventThreads();
     return llvm::Error::success();
   }
@@ -1568,18 +1584,18 @@ void DAP::EventThread() {
         } else if (event_mask & lldb::SBTarget::eBroadcastBitNewTargetCreated) {
           auto target = lldb::SBTarget::GetTargetFromEvent(event);
 
-          // Generate unique target ID and store the target for handoff.
           lldb::user_id_t target_id = target.GetGloballyUniqueID();
-          DAPSessionManager::GetInstance().StoreTargetById(target_id, target);
+          lldb::SBDebugger target_debugger = target.GetDebugger();
+          int debugger_id = target_debugger.GetID();
 
-          // We create an attach config that will select the unique
-          // target ID of the created target. The DAP instance will attach to
-          // this existing target and the debug session will be ready to go.
+          // We create an attach config that contains the debugger ID and target
+          // ID. The new DAP instance will use these IDs to find the existing
+          // debugger and target via FindDebuggerWithID and
+          // FindTargetByGloballyUniqueID.
           llvm::json::Object attach_config;
 
-          // If we have a process name, add command to attach to the same
-          // process name.
           attach_config.try_emplace("type", "lldb");
+          attach_config.try_emplace("debuggerId", debugger_id);
           attach_config.try_emplace("targetId", target_id);
           const char *session_name = target.GetTargetSessionName();
           attach_config.try_emplace("name", session_name);

lldb/tools/lldb-dap/DAP.h

@@ -431,7 +431,8 @@ struct DAP final : public DAPTransport::MessageHandler {
 
   /// Perform complete DAP initialization in one call.
   llvm::Error
-  InitializeDebugger(std::optional<lldb::user_id_t> target_idx = std::nullopt);
+  InitializeDebugger(std::optional<int> debugger_id = std::nullopt,
+                     std::optional<lldb::user_id_t> target_id = std::nullopt);
 
   /// Start event handling threads based on client capabilities.
   void StartEventThreads();

lldb/tools/lldb-dap/DAPSessionManager.cpp

@@ -50,11 +50,6 @@ void DAPSessionManager::RegisterSession(lldb_private::MainLoop *loop,
 void DAPSessionManager::UnregisterSession(lldb_private::MainLoop *loop) {
   std::unique_lock<std::mutex> lock(m_sessions_mutex);
   m_active_sessions.erase(loop);
-
-  // Clean up shared resources when the last session exits.
-  if (m_active_sessions.empty())
-    CleanupSharedResources();
-
   std::notify_all_at_thread_exit(m_sessions_condition, std::move(lock));
 }
 
@@ -118,23 +113,6 @@ DAPSessionManager::GetEventThreadForDebugger(lldb::SBDebugger debugger,
   return new_thread_sp;
 }
 
-void DAPSessionManager::StoreTargetById(lldb::user_id_t target_id,
-                                        lldb::SBTarget target) {
-  std::lock_guard<std::mutex> lock(m_sessions_mutex);
-  m_target_map[target_id] = target;
-}
-
-std::optional<lldb::SBTarget>
-DAPSessionManager::TakeTargetById(lldb::user_id_t target_id) {
-  std::lock_guard<std::mutex> lock(m_sessions_mutex);
-  auto pos = m_target_map.find(target_id);
-  if (pos == m_target_map.end())
-    return std::nullopt;
-  lldb::SBTarget target = pos->second;
-  m_target_map.erase(pos);
-  return target;
-}
-
 DAP *DAPSessionManager::FindDAPForTarget(lldb::SBTarget target) {
   std::lock_guard<std::mutex> lock(m_sessions_mutex);
 
@@ -145,13 +123,6 @@ DAP *DAPSessionManager::FindDAPForTarget(lldb::SBTarget target) {
   return nullptr;
 }
 
-void DAPSessionManager::CleanupSharedResources() {
-  // Note: Caller must hold m_sessions_mutex.
-  // SBTarget destructors will handle cleanup when the map entries are
-  // destroyed.
-  m_target_map.clear();
-}
-
 void DAPSessionManager::ReleaseExpiredEventThreads() {
   std::lock_guard<std::mutex> lock(m_sessions_mutex);
   for (auto it = m_debugger_event_threads.begin();

lldb/tools/lldb-dap/DAPSessionManager.h

@@ -71,31 +71,6 @@ class DAPSessionManager {
   /// DisconnectAllSessions() was called and any disconnection failed.
   llvm::Error WaitForAllSessionsToDisconnect();
 
-  /// Store a target for later retrieval by another session.
-  ///
-  /// When a new target is dynamically created (e.g., via scripting hooks or
-  /// child process debugging), it needs to be handed off to a new DAP session.
-  /// This method stores the target in a temporary map using its globally unique
-  /// ID so that the new session can retrieve it via TakeTargetById().
-  ///
-  /// \param target_id The globally unique ID of the target (from
-  ///                  SBTarget::GetGloballyUniqueID()).
-  /// \param target The target to store.
-  void StoreTargetById(lldb::user_id_t target_id, lldb::SBTarget target);
-
-  /// Retrieve and remove a stored target by its globally unique target ID.
-  ///
-  /// This method is called during the attach request when a new DAP session
-  /// wants to attach to a dynamically created target. The target is removed
-  /// from the map after retrieval because:
-  /// 1. Each target should only be attached to by one DAP session
-  /// 2. Once attached, the DAP session takes ownership of the target
-  /// 3. Keeping the target in the map would prevent proper cleanup
-  ///
-  /// \param target_id The globally unique ID of the target to retrieve.
-  /// \return The target if found, std::nullopt otherwise.
-  std::optional<lldb::SBTarget> TakeTargetById(lldb::user_id_t target_id);
-
   /// Get or create event thread for a specific debugger.
   std::shared_ptr<ManagedEventThread>
   GetEventThreadForDebugger(lldb::SBDebugger debugger, DAP *requesting_dap);
@@ -108,9 +83,6 @@ class DAPSessionManager {
     return GetInstance().FindDAPForTarget(target);
   }
 
-  /// Clean up shared resources when the last session exits.
-  void CleanupSharedResources();
-
   /// Clean up expired event threads from the collection.
   void ReleaseExpiredEventThreads();
 
@@ -129,10 +101,6 @@ class DAPSessionManager {
   std::condition_variable m_sessions_condition;
   std::map<lldb_private::MainLoop *, DAP *> m_active_sessions;
 
-  /// Map from target ID to target for handing off newly created targets
-  /// between sessions.
-  std::map<lldb::user_id_t, lldb::SBTarget> m_target_map;
-
   /// 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>>

lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp

@@ -32,8 +32,9 @@ namespace lldb_dap {
 Error AttachRequestHandler::Run(const AttachRequestArguments &args) const {
   // Initialize DAP debugger and related components if not sharing previously
   // launched debugger.
+  std::optional<int> debugger_id = args.debuggerId;
   std::optional<lldb::user_id_t> target_id = args.targetId;
-  if (Error err = dap.InitializeDebugger(target_id))
+  if (Error err = dap.InitializeDebugger(debugger_id, target_id))
     return err;
 
   // Validate that we have a well formed attach request.

lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp

@@ -317,7 +317,8 @@ bool fromJSON(const json::Value &Params, AttachRequestArguments &ARA,
          O.mapOptional("gdb-remote-port", ARA.gdbRemotePort) &&
          O.mapOptional("gdb-remote-hostname", ARA.gdbRemoteHostname) &&
          O.mapOptional("coreFile", ARA.coreFile) &&
-         O.mapOptional("targetId", ARA.targetId);
+         O.mapOptional("targetId", ARA.targetId) &&
+         O.mapOptional("debuggerId", ARA.debuggerId);
 }
 
 bool fromJSON(const json::Value &Params, ContinueArguments &CA, json::Path P) {

lldb/tools/lldb-dap/Protocol/ProtocolRequests.h

@@ -352,6 +352,9 @@ struct AttachRequestArguments {
   /// Unique ID of an existing target to attach to.
   std::optional<lldb::user_id_t> targetId;
 
+  /// ID of an existing debugger instance to use.
+  std::optional<int> debuggerId;
+
   /// @}
 };
 bool fromJSON(const llvm::json::Value &, AttachRequestArguments &,

lldb/unittests/DAP/DAPSessionManagerTest.cpp

@@ -25,117 +25,6 @@ TEST_F(DAPSessionManagerTest, GetInstanceReturnsSameSingleton) {
   EXPECT_EQ(&instance1, &instance2);
 }
 
-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 = target.GetGloballyUniqueID();
-  manager.StoreTargetById(target_id, target);
-
-  // Retrieval consumes the target (removes from map).
-  std::optional<SBTarget> retrieved = manager.TakeTargetById(target_id);
-  ASSERT_TRUE(retrieved.has_value());
-  EXPECT_TRUE(retrieved->IsValid());
-  // Verify we got the correct target back.
-  EXPECT_EQ(retrieved->GetDebugger().GetID(), debugger.GetID());
-  EXPECT_EQ(*retrieved, target);
-
-  // 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, GetTargetWithInvalidId) {
-  DAPSessionManager &manager = DAPSessionManager::GetInstance();
-
-  std::optional<SBTarget> result = manager.TakeTargetById(99999);
-
-  EXPECT_FALSE(result.has_value());
-}
-
-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 = target1.GetGloballyUniqueID();
-  uint32_t target_id_2 = target2.GetGloballyUniqueID();
-
-  // Sanity check the targets have distinct IDs.
-  EXPECT_NE(target_id_1, target_id_2);
-
-  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_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<SBTarget> retrieved2 = manager.TakeTargetById(target_id_2);
-  ASSERT_TRUE(retrieved2.has_value());
-  EXPECT_TRUE(retrieved2->IsValid());
-  EXPECT_EQ(retrieved2->GetDebugger().GetID(), debugger2.GetID());
-  EXPECT_EQ(*retrieved2, target2);
-
-  SBDebugger::Destroy(debugger1);
-  SBDebugger::Destroy(debugger2);
-}
-
-TEST_F(DAPSessionManagerTest, CleanupSharedResources) {
-  DAPSessionManager &manager = DAPSessionManager::GetInstance();
-
-  SBDebugger debugger = SBDebugger::Create();
-  ASSERT_TRUE(debugger.IsValid());
-  // 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();
-
-  // 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);
-}
-
 // UnregisterSession uses std::notify_all_at_thread_exit, so it must be called
 // from a separate thread to properly release the mutex on thread exit.
 TEST_F(DAPSessionManagerTest, RegisterAndUnregisterSession) {