[lldb-dap] Add multi-session support with shared debugger instances #163653
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
Member
|
@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); |
Member
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); |
Member
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?
Contributor
Author
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. |
Member
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()) |
Member
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; |
Member
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. |
Member
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(); |
Member
There was a problem hiding this comment.
Does this just wait or does it also trigger a disconnection?
Contributor
Author
There was a problem hiding this comment.
DisconnectAllSessions() should trigger the disconnection, while WaitForAllSessionsToDisconnect() blocks and waits for the sessions to disconnect and unregister.
Member
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; |
Member
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()
Contributor
Author
There was a problem hiding this comment.
I'm a bit confused what the thread id here is, do you mean target id -> SBTarget?
Member
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>> |
Member
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
Contributor
Author
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?
Member
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
Member
|
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
Contributor
ashgti
left a comment
ashgti
left a comment
There was a problem hiding this comment.
Can we also add a test in test/API/tools/lldb-dap for this? Maybe in the test/API/tools/lldb-dap/startDebugging folder?
| std::string coreFile; | ||
|
|
||
| /// Unique ID of an existing target to attach to. | ||
| std::optional<uint32_t> targetId; |
Contributor
There was a problem hiding this comment.
SBTarget::GetGloballyUniqueID() returns a lldb::user_id_t, should we use that here as well?
lldb/tools/lldb-dap/DAP.cpp
Outdated
| auto target = lldb::SBTarget::GetTargetFromEvent(event); | ||
|
|
||
| // Generate unique target ID and store the target for handoff. | ||
| uint32_t target_id = target.GetGloballyUniqueID(); |
Contributor
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); |
Contributor
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?
Contributor
Author
There was a problem hiding this comment.
When a startDebugging request is called and creates a child DAP session, there's currently no way to attach to some existing debugger instance/target ID. StoreTargetById is used as a way to temporarily store the newly created target so that the new DAP session can attach to it and keep a strong mapping from target id to Target. The TakeTargetById removes it from the list of targets so only one DAP session can take ownership of the new target.
Member
There was a problem hiding this comment.
Now that I think of it, you don't need to this mapping.
Assuming that a DAP debug session has a single debugger instance, which I think is true 99.999% of the cases, whenever you want to attach to a child target by ID, you can get the Debugger instance of any existing target, then you can ask its debugger for its target list, and then you you do a linear scan on it finding the ID you want.
Then you can avoid doing all this tracking.
Member
There was a problem hiding this comment.
And if you were to track something, you can track all Debugger instances that were created. But hopefully you don't need this
Collaborator
There was a problem hiding this comment.
There is a function in the lldb::SBDebugger that can already find you a target by and a debugger by ID (and debugger IDs start with 1 and increment, so it would be easy to iterate if needed:
static lldb::SBDebugger lldb::SBDebugger::FindDebuggerWithID(int id);
lldb::SBTarget lldb::SBDebugger::FindTargetByGloballyUniqueID(lldb::user_id_t id);
So you should be able to find what you need without tracking this in StoreTargetById as Walter suggested.
You can also get the debugger ID easily if needed:
(lldb) script lldb.debugger.GetID()
1
And this API is availabe from C++.
lldb/tools/lldb-dap/DAP.cpp
Outdated
Comment on lines
1322
to
1324
| // Get the debugger from the target and set it up. | ||
| debugger = shared_target->GetDebugger(); | ||
| StartEventThreads(); |
Contributor
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.
Contributor
Author
There was a problem hiding this comment.
Would we want some sort of callback mechanism here? Otherwise, it seems like using initCommands/preRunCommands from the attach config seems to also be able to provide the ability to serve as a hook for running commands when it's reused.
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; | ||
| } |
Contributor
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.
lldb/tools/lldb-dap/DAP.cpp
Outdated
Comment on lines
1568
to
1569
| } else if (event_mask & lldb::SBTarget::eBroadcastBitNewTargetCreated) { | ||
| auto target = lldb::SBTarget::GetTargetFromEvent(event); |
Contributor
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?
Contributor
Author
There was a problem hiding this comment.
Thanks for the catch, we should probably ignore this launch/attach commands, and only use it for when there's some runtime dynamic target creation. I'll work on fixing this
Contributor
Author
There was a problem hiding this comment.
On second thought, if you were to just have an attach/launch command create a target, it by default doesn't broadcast eBroadcastBitNewTargetCreated so I don't think this should be an issue?
ashgti
reviewed
Oct 20, 2025
lldb/tools/lldb-dap/DAP.cpp
Outdated
| // "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() { |
Contributor
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.
lldb/include/lldb/Target/Target.h
Outdated
| /// | ||
| /// \return | ||
| /// The target session name for this target. | ||
| const std::string &GetTargetSessionName() { return m_target_session_name; } |
Member
There was a problem hiding this comment.
Suggested change
| const std::string &GetTargetSessionName() { return m_target_session_name; } | |
| llvm::StringRef GetTargetSessionName() { return m_target_session_name; } |
You should use StringRef. It can represent different kinds of strings automatically.
lldb/tools/lldb-dap/DAP.cpp
Outdated
Comment on lines
1312
to
1328
| 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 (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"); | ||
| } |
Member
There was a problem hiding this comment.
this doesn't look very clean.
What about creating two InitializeDebugger functions? One doesn't take parameters and the other one takes forcefully those two ids?
| @@ -0,0 +1,142 @@ | |||
| //===-- DAPSessionManager.cpp ----------------------------------*- C++ -*-===// | |||
Member
There was a problem hiding this comment.
new files should use a simple
//===----------------------------------------------------------------------===//
no need to repeat the name of the file
| dap.Send(protocol::Event{"memory", std::move(body)}); | ||
| } | ||
|
|
||
| // Note: EventThread() is architecturally different from the other functions in |
Member
There was a problem hiding this comment.
thanks for this comment
ashgti
reviewed
Oct 23, 2025
| /// 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 { |
Contributor
There was a problem hiding this comment.
Small nit, but we could just call this SessionManager, its already in the lldb_dap namespace. But not that big of a deal either way.
Comment on lines
52
to
60
| /// Get the singleton instance of the DAP session manager. | ||
| static DAPSessionManager &GetInstance(); |
Contributor
There was a problem hiding this comment.
Most of the time in lldb, components have an Initialize / Terminate function to setup / teardown an instance.
Contributor
Author
There was a problem hiding this comment.
@ashgti Thanks for the suggestion! Do you have recommendations for what to include if we were to implement Initialize/Terminate? I was trying to think of what we'd do, but DAPSessionManager's just a coordinator/registry that keeps some maps that have safe defaults so it doesn't seem to need much initialization, and it seems like it'd have a pretty empty Terminate as well since it doesn't technically own anything.
|
|
||
| DAPSessionManager &DAPSessionManager::GetInstance() { | ||
| static std::once_flag initialized; | ||
| static DAPSessionManager *instance = |
Contributor
There was a problem hiding this comment.
Should this be a llvm::ManagedStatic?
Contributor
Author
There was a problem hiding this comment.
This didn't end up working here and we may need to keep this intentional leak due to issues w/ the destructor chain. It seemed to cause DAP to hang since the ManagedStatic seemed to try to destroy the DAPSessionManager and some resources may have already been cleaned up by LLDB termination.
lldb/tools/lldb-dap/EventHelper.cpp
Outdated
| // 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; |
Contributor
There was a problem hiding this comment.
You should be able to use lldb_dap::protocol::AttachRequestArguments for this type.
Contributor
Author
There was a problem hiding this comment.
@ashgti I was trying to use this type, but it didn't end up working automatically here, since SendReverseRequest expects an llvm::json::Value argument, so if we were to try to use lldb_dap::protocol::AttachRequestArguments, we'd have to create an AttachRequestArguments object and populate its values, then convert it back to JSON anyways. It also doesn't seem like any of the other Request types in DAP have custom ToJSON type functions serialization.
lldb/tools/lldb-dap/EventHelper.cpp
Outdated
| dap_instance = active_instances[0]; | ||
| } | ||
|
|
||
| if (dap_instance) { |
Contributor
There was a problem hiding this comment.
Should we report an error if we made it here without a dap_instance? That seems like an internal logic issue, right?
lldb/tools/lldb-dap/EventHelper.cpp
Outdated
Comment on lines
455
to
457
| // Find the DAP instance that owns this target to check if we should | ||
| // ignore this event. | ||
| DAP *dap_instance = DAPSessionManager::FindDAP(target); |
Contributor
There was a problem hiding this comment.
I'm not seeing where we ignore this if the dap_instance isn't at some point, like checking dap.configuration_done
qxy11
force-pushed
the
dap-shared-debugger
branch
from
7d596d7 to
ded699a
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Contributor
Author
|
Just following up on this PR - thanks for the feedback so far! If there’s anything else I should address, please let me know. With the ongoing changes to DAP, rebasing has been a bit tricky, so I’d love to get this merged soon if possible. |
clayborg
approved these changes
Nov 20, 2025
Collaborator
clayborg
left a comment
clayborg
left a comment
There was a problem hiding this comment.
Looks like feedback was followed up on, lets start with this and iterate on it!
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.
Summary: - Changed targetId type from uint32_t to lldb::user_id_t - Added comprehensive documentation for StoreTargetById/TakeTargetById
qxy11
force-pushed
the
dap-shared-debugger
branch
from
c601582 to
4fb482e
Compare
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/181/builds/32436 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/163/builds/30578 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/19518 Here is the relevant piece of the build log for the reference |
Collaborator
|
@Michael137 any ideas on how to track down what is going wrong here? How do we run these tests? |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/110/builds/6518 Here is the relevant piece of the build log for the reference |
Collaborator
|
If there is not a quick fix coming, can we revert the change to get the bots back to green and before it becomes trickier to revert? |
Contributor
Author
|
I think #169744 might be able to fix the cross-project-test errors |
tanji-dg
pushed a commit
to tanji-dg/llvm-project
that referenced
this pull request
Nov 27, 2025
…lvm#163653) ## 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: - Add `DAPSessionManager` singleton to track and coordinate all active DAP sessions - Support attaching to an existing target via its globally unique target ID (targetId parameter) - Share debugger instances across sessions when new targets are created dynamically - Refactor event thread management to allow sharing event threads between sessions and move event thread and event thread handlers to `EventHelpers` - Add `eBroadcastBitNewTargetCreated` event to notify when new targets are created - Extract session names from target creation events - Defer debugger initialization from 'initialize' request to 'launch'/'attach' requests. The only time the debugger is used currently in between its creation in `InitializeRequestHandler` and the `Launch` or `Attach` requests is during the `TelemetryDispatcher` destruction call at the end of the `DAP::HandleObject` call, 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 ``` >> ninja DAPTests >> ./tools/lldb/unittests/DAP/DAPTests >>./bin/llvm-lit -v ../llvm-project/lldb/test/API/tools/lldb-dap/ ```
qxy11
added a commit
that referenced
this pull request
Nov 27, 2025
…169744) # Summary This is a forward fix for test errors from #163653. The PR moved debugger initialization outside of InitializeRequestHandler, and into Launch/AttachRequestHandlers to support DAP sessions sharing debugger instances for dynamically created targets. However, DExTer's DAP class seemed to set breakpoints before the debugger was initialized, which caused the tests to hang waiting for a breakpoint to hit due to none of the breakpoints getting resolved. # Tests ``` bin/llvm-lit -v /home/qxy11/llvm/llvm-project/cross-project-tests/debuginfo-tests/dexter-tests/ ```
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Nov 27, 2025
…n DExTer (#169744) # Summary This is a forward fix for test errors from llvm/llvm-project#163653. The PR moved debugger initialization outside of InitializeRequestHandler, and into Launch/AttachRequestHandlers to support DAP sessions sharing debugger instances for dynamically created targets. However, DExTer's DAP class seemed to set breakpoints before the debugger was initialized, which caused the tests to hang waiting for a breakpoint to hit due to none of the breakpoints getting resolved. # Tests ``` bin/llvm-lit -v /home/qxy11/llvm/llvm-project/cross-project-tests/debuginfo-tests/dexter-tests/ ```
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/64/builds/6635 Here is the relevant piece of the build log for the reference |
GeneraluseAI
pushed a commit
to GeneraluseAI/llvm-project
that referenced
this pull request
Nov 27, 2025
…lvm#163653) ## 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: - Add `DAPSessionManager` singleton to track and coordinate all active DAP sessions - Support attaching to an existing target via its globally unique target ID (targetId parameter) - Share debugger instances across sessions when new targets are created dynamically - Refactor event thread management to allow sharing event threads between sessions and move event thread and event thread handlers to `EventHelpers` - Add `eBroadcastBitNewTargetCreated` event to notify when new targets are created - Extract session names from target creation events - Defer debugger initialization from 'initialize' request to 'launch'/'attach' requests. The only time the debugger is used currently in between its creation in `InitializeRequestHandler` and the `Launch` or `Attach` requests is during the `TelemetryDispatcher` destruction call at the end of the `DAP::HandleObject` call, 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 ``` >> ninja DAPTests >> ./tools/lldb/unittests/DAP/DAPTests >>./bin/llvm-lit -v ../llvm-project/lldb/test/API/tools/lldb-dap/ ```
GeneraluseAI
pushed a commit
to GeneraluseAI/llvm-project
that referenced
this pull request
Nov 27, 2025
…lvm#169744) # Summary This is a forward fix for test errors from llvm#163653. The PR moved debugger initialization outside of InitializeRequestHandler, and into Launch/AttachRequestHandlers to support DAP sessions sharing debugger instances for dynamically created targets. However, DExTer's DAP class seemed to set breakpoints before the debugger was initialized, which caused the tests to hang waiting for a breakpoint to hit due to none of the breakpoints getting resolved. # Tests ``` bin/llvm-lit -v /home/qxy11/llvm/llvm-project/cross-project-tests/debuginfo-tests/dexter-tests/ ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 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