[lldb-dap] Add multi-session support with shared debugger instances #163653
+842
−141
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: This change introduces a DAPSessionManager to enable multiple concurrent lldb-dap sessions and allow them to share debugger instances when needed. Key changes: - Add DAPSessionManager singleton to track and coordinate all active DAP sessions - Support attaching to an existing target via unique target ID (targetId parameter) - Share debugger instances across sessions when spawning new targets (e.g., GPU debugging) - Refactor event thread management to allow sharing event threads between sessions - Add eBroadcastBitNewTargetCreated event to notify when new targets are spawned - Extract session names from target creation events for better identification - Defer debugger initialization from 'initialize' request to 'launch'/'attach' requests This enables scenarios where a native process spawns a new target (like a child process) and the debug adapter can automatically start a new debug session for the spawned target while sharing the parent's debugger instance. Tests: The refactoring maintains backward compatibility. All existing test cases pass.
|
@llvm/pr-subscribers-lldb Author: Janet Yang (qxy11) ChangesSummary:This change introduces a DAPSessionManager to enable multiple DAP sessions to share debugger instances when needed, for things like child process debugging and some scripting hooks that create dynamically new targets. Changes include:
This enables scenarios when new targets are created dynamically so that the debug adapter can automatically start a new debug session for the spawned target while sharing the debugger instance. Tests:The refactoring maintains backward compatibility. All existing DAP test cases pass. Also added a few basic unit tests for DAPSessionManager Patch is 56.92 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/163653.diff 21 Files Affected:
diff --git a/lldb/include/lldb/API/SBTarget.h b/lldb/include/lldb/API/SBTarget.h
index 173fd05b54a13..1d251763a826a 100644
--- a/lldb/include/lldb/API/SBTarget.h
+++ b/lldb/include/lldb/API/SBTarget.h
@@ -44,6 +44,7 @@ class LLDB_API SBTarget {
eBroadcastBitWatchpointChanged = (1 << 3),
eBroadcastBitSymbolsLoaded = (1 << 4),
eBroadcastBitSymbolsChanged = (1 << 5),
+ eBroadcastBitNewTargetCreated = (1 << 6),
};
// Constructors
@@ -69,6 +70,8 @@ class LLDB_API SBTarget {
static lldb::SBModule GetModuleAtIndexFromEvent(const uint32_t idx,
const lldb::SBEvent &event);
+ static const char *GetSessionNameFromEvent(const SBEvent &event);
+
static const char *GetBroadcasterClassName();
lldb::SBProcess GetProcess();
diff --git a/lldb/include/lldb/Target/Target.h b/lldb/include/lldb/Target/Target.h
index f4a09237ce897..86349897762f4 100644
--- a/lldb/include/lldb/Target/Target.h
+++ b/lldb/include/lldb/Target/Target.h
@@ -537,6 +537,7 @@ class Target : public std::enable_shared_from_this<Target>,
eBroadcastBitWatchpointChanged = (1 << 3),
eBroadcastBitSymbolsLoaded = (1 << 4),
eBroadcastBitSymbolsChanged = (1 << 5),
+ eBroadcastBitNewTargetCreated = (1 << 6),
};
// These two functions fill out the Broadcaster interface:
@@ -556,6 +557,11 @@ class Target : public std::enable_shared_from_this<Target>,
TargetEventData(const lldb::TargetSP &target_sp,
const ModuleList &module_list);
+ TargetEventData(const lldb::TargetSP &target_sp, std::string session_name);
+
+ TargetEventData(const lldb::TargetSP &target_sp,
+ const ModuleList &module_list, std::string session_name);
+
~TargetEventData() override;
static llvm::StringRef GetFlavorString();
@@ -564,6 +570,8 @@ class Target : public std::enable_shared_from_this<Target>,
return TargetEventData::GetFlavorString();
}
+ static llvm::StringRef GetSessionNameFromEvent(const Event *event_ptr);
+
void Dump(Stream *s) const override;
static const TargetEventData *GetEventDataFromEvent(const Event *event_ptr);
@@ -579,6 +587,7 @@ class Target : public std::enable_shared_from_this<Target>,
private:
lldb::TargetSP m_target_sp;
ModuleList m_module_list;
+ std::string m_session_name;
TargetEventData(const TargetEventData &) = delete;
const TargetEventData &operator=(const TargetEventData &) = delete;
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
index 8eb64b4ab8b2b..b52aaf558aa24 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
@@ -785,6 +785,7 @@ def request_attach(
*,
program: Optional[str] = None,
pid: Optional[int] = None,
+ targetId: Optional[int] = None,
waitFor=False,
initCommands: Optional[list[str]] = None,
preRunCommands: Optional[list[str]] = None,
@@ -804,6 +805,8 @@ def request_attach(
args_dict["pid"] = pid
if program is not None:
args_dict["program"] = program
+ if targetId is not None:
+ args_dict["targetId"] = targetId
if waitFor:
args_dict["waitFor"] = waitFor
args_dict["initCommands"] = self.init_commands
diff --git a/lldb/source/API/SBTarget.cpp b/lldb/source/API/SBTarget.cpp
index 98d10aa07c53f..0f4508c772ee4 100644
--- a/lldb/source/API/SBTarget.cpp
+++ b/lldb/source/API/SBTarget.cpp
@@ -145,6 +145,14 @@ SBModule SBTarget::GetModuleAtIndexFromEvent(const uint32_t idx,
return SBModule(module_list.GetModuleAtIndex(idx));
}
+const char *SBTarget::GetSessionNameFromEvent(const SBEvent &event) {
+ LLDB_INSTRUMENT_VA(event);
+
+ return ConstString(
+ Target::TargetEventData::GetSessionNameFromEvent(event.get()))
+ .AsCString();
+}
+
const char *SBTarget::GetBroadcasterClassName() {
LLDB_INSTRUMENT();
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index e224a12e33463..a13e11c4b3a05 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -193,6 +193,7 @@ Target::Target(Debugger &debugger, const ArchSpec &target_arch,
SetEventName(eBroadcastBitModulesUnloaded, "modules-unloaded");
SetEventName(eBroadcastBitWatchpointChanged, "watchpoint-changed");
SetEventName(eBroadcastBitSymbolsLoaded, "symbols-loaded");
+ SetEventName(eBroadcastBitNewTargetCreated, "new-target-created");
CheckInWithManager();
@@ -5169,11 +5170,21 @@ void TargetProperties::SetDebugUtilityExpression(bool debug) {
// Target::TargetEventData
Target::TargetEventData::TargetEventData(const lldb::TargetSP &target_sp)
- : EventData(), m_target_sp(target_sp), m_module_list() {}
+ : TargetEventData(target_sp, ModuleList(), "") {}
Target::TargetEventData::TargetEventData(const lldb::TargetSP &target_sp,
const ModuleList &module_list)
- : EventData(), m_target_sp(target_sp), m_module_list(module_list) {}
+ : TargetEventData(target_sp, module_list, "") {}
+
+Target::TargetEventData::TargetEventData(const lldb::TargetSP &target_sp,
+ std::string session_name)
+ : TargetEventData(target_sp, ModuleList(), std::move(session_name)) {}
+
+Target::TargetEventData::TargetEventData(const lldb::TargetSP &target_sp,
+ const ModuleList &module_list,
+ std::string session_name)
+ : EventData(), m_target_sp(target_sp), m_module_list(module_list),
+ m_session_name(std::move(session_name)) {}
Target::TargetEventData::~TargetEventData() = default;
@@ -5209,6 +5220,25 @@ TargetSP Target::TargetEventData::GetTargetFromEvent(const Event *event_ptr) {
return target_sp;
}
+llvm::StringRef
+Target::TargetEventData::GetSessionNameFromEvent(const Event *event_ptr) {
+ const TargetEventData *event_data = GetEventDataFromEvent(event_ptr);
+ if (!event_data)
+ return llvm::StringRef();
+
+ if (!event_data->m_session_name.empty())
+ return event_data->m_session_name;
+
+ // Generate default session name if not provided
+ if (event_data->m_target_sp) {
+ lldb::user_id_t target_id = event_data->m_target_sp->GetGloballyUniqueID();
+ std::string default_name = llvm::formatv("Session {0}", target_id).str();
+ return ConstString(default_name).GetStringRef();
+ }
+
+ return llvm::StringRef();
+}
+
ModuleList
Target::TargetEventData::GetModuleListFromEvent(const Event *event_ptr) {
ModuleList module_list;
diff --git a/lldb/test/API/tools/lldb-dap/attach/TestDAP_attach.py b/lldb/test/API/tools/lldb-dap/attach/TestDAP_attach.py
index 5331a9f94ef1f..6ca286c12abd2 100644
--- a/lldb/test/API/tools/lldb-dap/attach/TestDAP_attach.py
+++ b/lldb/test/API/tools/lldb-dap/attach/TestDAP_attach.py
@@ -72,3 +72,16 @@ def test_by_name_waitFor(self):
self.spawn_thread.start()
self.attach(program=program, waitFor=True)
self.continue_and_verify_pid()
+
+ def test_attach_with_invalid_targetId(self):
+ """
+ Test that attaching with an invalid targetId fails with the expected
+ error message.
+ """
+ self.build_and_create_debug_adapter()
+
+ resp = self.attach(targetId=99999, expectFailure=True)
+ self.assertFalse(resp["success"])
+ self.assertIn(
+ "Unable to find existing debugger", resp["body"]["error"]["format"]
+ )
diff --git a/lldb/tools/lldb-dap/CMakeLists.txt b/lldb/tools/lldb-dap/CMakeLists.txt
index 7db334ca56bcf..2fbdc73a79978 100644
--- a/lldb/tools/lldb-dap/CMakeLists.txt
+++ b/lldb/tools/lldb-dap/CMakeLists.txt
@@ -12,6 +12,7 @@ add_lldb_library(lldbDAP
DAP.cpp
DAPError.cpp
DAPLog.cpp
+ DAPSessionManager.cpp
EventHelper.cpp
ExceptionBreakpoint.cpp
FifoFiles.cpp
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index f76656e98ca01..dc681336637e0 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//
#include "DAP.h"
+#include "CommandPlugins.h"
#include "DAPLog.h"
#include "EventHelper.h"
#include "ExceptionBreakpoint.h"
@@ -241,10 +242,12 @@ llvm::Error DAP::ConfigureIO(std::FILE *overrideOut, std::FILE *overrideErr) {
}
void DAP::StopEventHandlers() {
- if (event_thread.joinable()) {
- broadcaster.BroadcastEventByType(eBroadcastBitStopEventThread);
- event_thread.join();
- }
+ event_thread_sp.reset();
+
+ // Clean up expired event threads from the session manager.
+ DAPSessionManager::GetInstance().ReleaseExpiredEventThreads();
+
+ // Still handle the progress thread normally since it's per-DAP instance.
if (progress_event_thread.joinable()) {
broadcaster.BroadcastEventByType(eBroadcastBitStopProgressThread);
progress_event_thread.join();
@@ -804,7 +807,8 @@ void DAP::SetTarget(const lldb::SBTarget target) {
lldb::SBTarget::eBroadcastBitModulesLoaded |
lldb::SBTarget::eBroadcastBitModulesUnloaded |
lldb::SBTarget::eBroadcastBitSymbolsLoaded |
- lldb::SBTarget::eBroadcastBitSymbolsChanged);
+ lldb::SBTarget::eBroadcastBitSymbolsChanged |
+ lldb::SBTarget::eBroadcastBitNewTargetCreated);
listener.StartListeningForEvents(this->broadcaster,
eBroadcastBitStopEventThread);
}
@@ -1289,13 +1293,91 @@ protocol::Capabilities DAP::GetCustomCapabilities() {
}
void DAP::StartEventThread() {
- event_thread = std::thread(&DAP::EventThread, this);
+ // Get event thread for this debugger (creates it if it doesn't exist).
+ event_thread_sp = DAPSessionManager::GetInstance().GetEventThreadForDebugger(
+ debugger, this);
}
void DAP::StartProgressEventThread() {
progress_event_thread = std::thread(&DAP::ProgressEventThread, this);
}
+void DAP::StartEventThreads() {
+ if (clientFeatures.contains(eClientFeatureProgressReporting))
+ StartProgressEventThread();
+
+ StartEventThread();
+}
+
+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) {
+ return llvm::createStringError(
+ "Unable to find existing debugger for target ID");
+ }
+ debugger = shared_debugger.value();
+ StartEventThreads();
+ return llvm::Error::success();
+ }
+
+ debugger = lldb::SBDebugger::Create(/*argument_name=*/false);
+
+ // Configure input/output/error file descriptors.
+ debugger.SetInputFile(in);
+ target = debugger.GetDummyTarget();
+
+ llvm::Expected<int> out_fd = out.GetWriteFileDescriptor();
+ if (!out_fd)
+ return out_fd.takeError();
+ debugger.SetOutputFile(lldb::SBFile(*out_fd, "w", false));
+
+ llvm::Expected<int> err_fd = err.GetWriteFileDescriptor();
+ if (!err_fd)
+ return err_fd.takeError();
+ debugger.SetErrorFile(lldb::SBFile(*err_fd, "w", false));
+
+ // The sourceInitFile option is not part of the DAP specification. It is an
+ // extension used by the test suite to prevent sourcing `.lldbinit` and
+ // changing its behavior. The CLI flag --no-lldbinit takes precedence over
+ // the DAP parameter.
+ bool should_source_init_files = !no_lldbinit && sourceInitFile;
+ if (should_source_init_files) {
+ debugger.SkipLLDBInitFiles(false);
+ debugger.SkipAppInitFiles(false);
+ lldb::SBCommandReturnObject init;
+ auto interp = debugger.GetCommandInterpreter();
+ interp.SourceInitFileInGlobalDirectory(init);
+ interp.SourceInitFileInHomeDirectory(init);
+ }
+
+ // Run initialization commands.
+ if (llvm::Error err = RunPreInitCommands())
+ return err;
+
+ auto cmd = debugger.GetCommandInterpreter().AddMultiwordCommand(
+ "lldb-dap", "Commands for managing lldb-dap.");
+
+ if (clientFeatures.contains(eClientFeatureStartDebuggingRequest)) {
+ cmd.AddCommand(
+ "start-debugging", new StartDebuggingCommand(*this),
+ "Sends a startDebugging request from the debug adapter to the client "
+ "to start a child debug session of the same type as the caller.");
+ }
+
+ cmd.AddCommand(
+ "repl-mode", new ReplModeCommand(*this),
+ "Get or set the repl behavior of lldb-dap evaluation requests.");
+ cmd.AddCommand("send-event", new SendEventCommand(*this),
+ "Sends an DAP event to the client.");
+
+ StartEventThreads();
+ return llvm::Error::success();
+}
+
void DAP::ProgressEventThread() {
lldb::SBListener listener("lldb-dap.progress.listener");
debugger.GetBroadcaster().AddListener(
@@ -1374,6 +1456,14 @@ void DAP::EventThread() {
const auto event_mask = event.GetType();
if (lldb::SBProcess::EventIsProcessEvent(event)) {
lldb::SBProcess process = lldb::SBProcess::GetProcessFromEvent(event);
+ // Find the DAP instance that owns this process's target.
+ DAP *dap_instance = DAPSessionManager::FindDAP(process.GetTarget());
+ if (!dap_instance) {
+ DAP_LOG(log, "Unable to find DAP instance for process {0}",
+ process.GetProcessID());
+ continue;
+ }
+
if (event_mask & lldb::SBProcess::eBroadcastBitStateChanged) {
auto state = lldb::SBProcess::GetStateFromEvent(event);
switch (state) {
@@ -1390,89 +1480,138 @@ void DAP::EventThread() {
// Only report a stopped event if the process was not
// automatically restarted.
if (!lldb::SBProcess::GetRestartedFromEvent(event)) {
- SendStdOutStdErr(*this, process);
- if (llvm::Error err = SendThreadStoppedEvent(*this))
- DAP_LOG_ERROR(log, std::move(err),
+ SendStdOutStdErr(*dap_instance, process);
+ if (llvm::Error err = SendThreadStoppedEvent(*dap_instance))
+ DAP_LOG_ERROR(dap_instance->log, std::move(err),
"({1}) reporting thread stopped: {0}",
- m_client_name);
+ dap_instance->m_client_name);
}
break;
case lldb::eStateRunning:
case lldb::eStateStepping:
- WillContinue();
- SendContinuedEvent(*this);
+ dap_instance->WillContinue();
+ SendContinuedEvent(*dap_instance);
break;
case lldb::eStateExited:
lldb::SBStream stream;
process.GetStatus(stream);
- SendOutput(OutputType::Console, stream.GetData());
+ dap_instance->SendOutput(OutputType::Console, stream.GetData());
// When restarting, we can get an "exited" event for the process we
// just killed with the old PID, or even with no PID. In that case
// we don't have to terminate the session.
if (process.GetProcessID() == LLDB_INVALID_PROCESS_ID ||
- process.GetProcessID() == restarting_process_id) {
- restarting_process_id = LLDB_INVALID_PROCESS_ID;
+ process.GetProcessID() == dap_instance->restarting_process_id) {
+ dap_instance->restarting_process_id = LLDB_INVALID_PROCESS_ID;
} else {
// Run any exit LLDB commands the user specified in the
// launch.json
- RunExitCommands();
- SendProcessExitedEvent(*this, process);
- SendTerminatedEvent();
+ dap_instance->RunExitCommands();
+ SendProcessExitedEvent(*dap_instance, process);
+ dap_instance->SendTerminatedEvent();
done = true;
}
break;
}
} else if ((event_mask & lldb::SBProcess::eBroadcastBitSTDOUT) ||
(event_mask & lldb::SBProcess::eBroadcastBitSTDERR)) {
- SendStdOutStdErr(*this, process);
+ SendStdOutStdErr(*dap_instance, process);
}
} else if (lldb::SBTarget::EventIsTargetEvent(event)) {
if (event_mask & lldb::SBTarget::eBroadcastBitModulesLoaded ||
event_mask & lldb::SBTarget::eBroadcastBitModulesUnloaded ||
event_mask & lldb::SBTarget::eBroadcastBitSymbolsLoaded ||
event_mask & lldb::SBTarget::eBroadcastBitSymbolsChanged) {
+ lldb::SBTarget event_target =
+ lldb::SBTarget::GetTargetFromEvent(event);
+ // Find the DAP instance that owns this target.
+ DAP *dap_instance = DAPSessionManager::FindDAP(event_target);
+ if (!dap_instance)
+ continue;
+
const uint32_t num_modules =
lldb::SBTarget::GetNumModulesFromEvent(event);
const bool remove_module =
event_mask & lldb::SBTarget::eBroadcastBitModulesUnloaded;
- std::lock_guard<std::mutex> guard(modules_mutex);
+ std::lock_guard<std::mutex> guard(dap_instance->modules_mutex);
for (uint32_t i = 0; i < num_modules; ++i) {
lldb::SBModule module =
lldb::SBTarget::GetModuleAtIndexFromEvent(i, event);
std::optional<protocol::Module> p_module =
- CreateModule(target, module, remove_module);
+ CreateModule(dap_instance->target, module, remove_module);
if (!p_module)
continue;
llvm::StringRef module_id = p_module->id;
- const bool module_exists = modules.contains(module_id);
+ const bool module_exists =
+ dap_instance->modules.contains(module_id);
if (remove_module && module_exists) {
- modules.erase(module_id);
- Send(protocol::Event{
+ dap_instance->modules.erase(module_id);
+ dap_instance->Send(protocol::Event{
"module", ModuleEventBody{std::move(p_module).value(),
ModuleEventBody::eReasonRemoved}});
} else if (module_exists) {
- Send(protocol::Event{
+ dap_instance->Send(protocol::Event{
"module", ModuleEventBody{std::move(p_module).value(),
ModuleEventBody::eReasonChanged}});
} else if (!remove_module) {
- modules.insert(module_id);
- Send(protocol::Event{
+ dap_instance->modules.insert(module_id);
+ dap_instance->Send(protocol::Event{
"module", ModuleEventBody{std::move(p_module).value(),
ModuleEventBody::eReasonNew}});
}
}
+ } else if (event_mask & lldb::SBTarget::eBroadcastBitNewTargetCreated) {
+ auto target = lldb::SBTarget::GetTargetFromEvent(event);
+
+ // Generate unique target ID and set the shared debugger.
+ uint32_t target_id = target.GetGloballyUniqueID();
+ DAPSessionManager::GetInstance().SetSharedDebugger(target_id,
+ debugger);
+
+ // 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.
+ 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("targetId", target_id);
+ const char *session_name =
+ lldb::SBTarget::GetSessionNameFromEvent(event);
+ attach_config.try_emplace("name", session_name);
+
+ // 2. Construct the main 'startDebugging' request arguments.
+ llvm::json::Object start_debugging_args{
+ {"request", "attach"},
+ {"configuratio...
[truncated]
|
lldb/include/lldb/API/SBTarget.h
Outdated
| static lldb::SBModule GetModuleAtIndexFromEvent(const uint32_t idx, | ||
| const lldb::SBEvent &event); | ||
|
|
||
| static const char *GetSessionNameFromEvent(const SBEvent &event); |
There was a problem hiding this comment.
add documentation mentioning which kind of events carry a session name
lldb/include/lldb/Target/Target.h
Outdated
Comment on lines
560
to
563
| TargetEventData(const lldb::TargetSP &target_sp, std::string session_name); | ||
|
|
||
| TargetEventData(const lldb::TargetSP &target_sp, | ||
| const ModuleList &module_list, std::string session_name); |
lldb/include/lldb/Target/Target.h
Outdated
Comment on lines
560
to
563
| TargetEventData(const lldb::TargetSP &target_sp, std::string session_name); | ||
|
|
||
| TargetEventData(const lldb::TargetSP &target_sp, | ||
| const ModuleList &module_list, std::string session_name); |
There was a problem hiding this comment.
Something I'm curious about is this: you are adding a m_session_name string to the TargetEventData. It seems to me that adding it to the Target itself would be better. Then at any point in time you can ask the Target for its session name. What do you think?
There was a problem hiding this comment.
That makes sense, I'll move that into the Target.
Comment on lines
34
to
35
| // NOTE: intentional leak to avoid issues with C++ destructor chain | ||
| // Use std::call_once for thread-safe initialization. |
There was a problem hiding this comment.
just remove this comment
| // 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()) |
There was a problem hiding this comment.
don't use auto for simple types.
Comment on lines
128
to
135
| DAPSessionManager::GetSharedDebugger(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()) | ||
| return std::nullopt; | ||
| lldb::SBDebugger debugger = pos->second; | ||
| m_target_to_debugger_map.erase(pos); | ||
| return debugger; |
There was a problem hiding this comment.
the name of this method starts with Get but you are actually removing an entry in the map. What's wrong here?
| std::thread m_event_thread; | ||
| }; | ||
|
|
||
| /// Global DAP session manager. |
There was a problem hiding this comment.
State here what's the purpose of this object and the most relevant aspects of it that people should know about
|
|
||
| /// Wait for all sessions to finish disconnecting. | ||
| /// Returns an error if any client disconnection failed, otherwise success. | ||
| llvm::Error WaitForAllSessionsToDisconnect(); |
There was a problem hiding this comment.
Does this just wait or does it also trigger a disconnection?
There was a problem hiding this comment.
DisconnectAllSessions() should trigger the disconnection, while WaitForAllSessionsToDisconnect() blocks and waits for the sessions to disconnect and unregister.
There was a problem hiding this comment.
mention that in the documentation :)
|
|
||
| /// 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; |
There was a problem hiding this comment.
do you really need to store the debugger?
It seems more convenient to me to make the map [thread id -> SBThread]. You can use it for many more things and if you really need a debugger instance from a thread id, you can do m_target_map[thread_id].GetDebugger()
There was a problem hiding this comment.
I'm a bit confused what the thread id here is, do you mean target id -> SBTarget?
There was a problem hiding this comment.
it should have been target id -> SBTarget, sorry :)
|
|
||
| /// Map from debugger ID to its event thread used for when | ||
| /// multiple DAP sessions are using the same debugger instance. | ||
| std::map<lldb::user_id_t, std::weak_ptr<ManagedEventThread>> |
There was a problem hiding this comment.
To make this safer, make the map [Debugger* -> std::weak_ptr<ManagedEventThread>
then you don't need to handle ids, instead you handle debugger references or pointers
There was a problem hiding this comment.
The DAPSessionManager/DAP works w/ the SB API, but to use Debugger* as the key, that'd require accessing the private internal API via SBDebugger::get() - would we want to make this a friend class to expose that to it?
There was a problem hiding this comment.
nope, in this case user_id_t is fine
|
✅ With the latest revision this PR passed the Python code formatter. |
walter-erquinigo
approved these changes
Oct 20, 2025
|
I haven’t had a chance to take a look at this yet, but given that this is a significant change, please give me (and other contributors) a bit more time to review this before merging this. |
ashgti
reviewed
Oct 20, 2025
| std::string coreFile; | ||
|
|
||
| /// Unique ID of an existing target to attach to. | ||
| std::optional<uint32_t> targetId; |
There was a problem hiding this comment.
SBTarget::GetGloballyUniqueID() returns a lldb::user_id_t, should we use that here as well?
| auto target = lldb::SBTarget::GetTargetFromEvent(event); | ||
|
|
||
| // Generate unique target ID and store the target for handoff. | ||
| uint32_t target_id = target.GetGloballyUniqueID(); |
There was a problem hiding this comment.
Shouldn't this be a lldb::user_id_t?
Comment on lines
+74
to
+75
| /// Store a target for later retrieval by another session. | ||
| void StoreTargetById(uint32_t target_id, lldb::SBTarget target); |
There was a problem hiding this comment.
I'm not sure I understand this part, why do we need to store them separately and the TakeTargetById, why does that remove them from the list of targets?
Comment on lines
+1322
to
+1324
| // Get the debugger from the target and set it up. | ||
| debugger = shared_target->GetDebugger(); | ||
| StartEventThreads(); |
There was a problem hiding this comment.
Should we have a hook for running any commands when the debugger is reused? Reusing the debugger would any state would be persisted between DAP sessions and having a hook to perform any updates would be helpful, I think.
Comment on lines
+121
to
+125
| void DAPSessionManager::StoreTargetById(uint32_t target_id, | ||
| lldb::SBTarget target) { | ||
| std::lock_guard<std::mutex> lock(m_sessions_mutex); | ||
| m_target_map[target_id] = target; | ||
| } |
There was a problem hiding this comment.
Do we need to make any assumptions about the target? E.g. should we assert that its paused? Is it valid for the DAP to continue using the SBTarget? If not, we may want this to take a DAP &dap param and remove the target from the DAP instance.
Comment on lines
+1568
to
+1569
| } else if (event_mask & lldb::SBTarget::eBroadcastBitNewTargetCreated) { | ||
| auto target = lldb::SBTarget::GetTargetFromEvent(event); |
There was a problem hiding this comment.
What if you have a launchCommand/attachCommand that creates a target?
Should we ignore this event while performing the launch/attach commands?
ashgti
reviewed
Oct 20, 2025
| // "FILE *" to output packets back to VS Code and they have mutexes in them | ||
| // them prevent multiple threads from writing simultaneously so no locking | ||
| // is required. | ||
| void DAP::EventThread() { |
There was a problem hiding this comment.
I think we should refactor this to not be part of the class. The fact that this and dap_instance are different is confusing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
This change introduces a DAPSessionManager to enable multiple DAP sessions to share debugger instances when needed, for things like child process debugging and some scripting hooks that create dynamically new targets.
Changes include:
InitializeRequestHandlerand theLaunchorAttachrequests is during theTelemetryDispatcherdestruction call at the end of theDAP::HandleObjectcall, so this is safe.This enables scenarios when new targets are created dynamically so that the debug adapter can automatically start a new debug session for the spawned target while sharing the debugger instance.
Tests:
The refactoring maintains backward compatibility. All existing DAP test cases pass.
Also added a few basic unit tests for DAPSessionManager