Integrate dual connection w/ LLDB-DAP using shared event thread #26

qxy11 · 2025-08-06T21:57:36Z

Summary

This PR adds support for dual connection debugging with LLDB-DAP using a shared debugger and event thread between DAP instances.

Adds a new Target event eBroadcastBitNewTargetSpawned. This is sent from ProcessGDBRemote when it creates and connects a new GPU target. In DAP::EventThread, we create a reverse startDebugging request, and send it an attach configuration with attachCommands to target select {x} where x is the GPU target ID, to set the GPU DAP instance's target correctly.
Adds a shared event thread field to DAP std::shared_ptr<std::thread> event_thread_sp;
Refactors lldb-dap.cpp to use a static DapSessionManager. This holds and manages the lifetimes of the shared debugger and active DAP instances, rather than creating and storing local instances in lldb-dap.cpp. Instead of creating a new EventThread per DAP instance, we set the event_thread_sp to the shared pointer to a single running EventThread.
- This holds an optional pointer std::optional<lldb::SBDebugger> shared_debugger_ to a shared debugger. When a target is spawned for the GPU case, we will set this shared debugger, and set the debugger to the shared debugger instance in the AttachRequestHandler. Otherwise, the shared debugger is not set, so we maintain backwards compatibility.
- This also holds std::map<lldb::user_id_t, std::shared_ptr<std::thread>> debugger_event_threads_. This maps debugger IDs to their shared event threads. If two DAP instances share a debugger, this allows us to return the correct shared event thread in GetEventThreadForDebugger. BC is still maintained, since if there's no shared debugger, we will create a new event thread like before.
Inside the EventThread(), we get the active DAP sessions from the DapSessionManager and coordinate event handling using to the correct DAP session based on the Target associated with the event with DAPSessionManager::FindDAPForTarget.
Once the reverse request is sent, VSCode creates a new debug child session under our main session, and LLDB-DAP creates a new DAP instance which passes through a InitializeRequestHandler and AttachRequestHandler. Rather than creating a new debugger in InitializeRequestHandler, we use the global shared debugger, and rather than creating a new target during the AttachRequestHandler, we already have the GPU target, so we just use these instances instead, and set the attributes of the DAP instance accordingly.
After this, the shared event thread should take care of forwarding events to the correct DAP instances, and we should be able to debug on VSCode by toggling between the native and gpu processes. Breakpoints are shared and automatically handled by VSCode.

Testing

Cherry-picked the changes on top of upstream LLVM, and DAP unit tests still pass.

Wrote a couple unit tests to test that

The reverse request is sent automatically, and has the expected parameters.
Modified the testing dap-server to simulate behavior from VSCode to initialize and launch a new debug session upon receiving "startDebugging". Tested that in server mode, this works.

bin/lldb-dotest -p TestDAP_gpu_reverse_request

Did some manual testing on VSCode. We can hit breakpoints, continue, switch between the target sessions and send commands correctly.

clayborg · 2025-08-06T23:07:35Z

lldb/tools/lldb-dap/DAP.cpp

This can go inside DAPSessionManager::GetInstance(). Also add a commnt before it:

// NOTE: intentional leak to avoid issues with C++ destructor chain

clayborg · 2025-08-06T23:20:27Z

lldb/tools/lldb-dap/DAP.cpp

This solutions implements a solution where once we have a GPU process that gets spawned, now all DAP instances after this will share the same debugger and the same event thread. It would be nice to be able to only do this for the spawned process. So if we did something like:

1 - a.out with GPU linkage gets launched
2 - GPU process is spawned and we add a new process to VS code
3 - CPU child process gets spawned

It would be nice if 1 and 2 share the same debugger and event thread, but 3 would have its own debugger and event thread.

Since we know the new SBTarget (in target above), maybe we can do something like:

DAPSessionManager::GetInstance().SetSharedDebuggerForTarget(debugger, target_index);

This is making a strong mapping that says that our new target wants to use this debugger. But then the question is how to make sure when we start the new debug session for the GPU to get this since the debugger gets created in the initialize packet. Is there a way to pass additional info to the "initialize" packet somehow so we can specify the target index?

clayborg · 2025-08-06T23:24:01Z

lldb/tools/lldb-dap/DAP.cpp

Add a comment here explaining what is going on:

// We create "attachCommands" that will select the target that already // exists in LLDB. The DAP instance will attach to this already existing // target and the debug session will be ready to go.

clayborg · 2025-08-06T23:38:34Z

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

This code makes me nervous. If the debugger already has a target, there is no guarantee that the selected target is the right one for the one we want to attach to.

One fix would be to add a new target index to the AttachRequestArguments and into the package.json:

struct AttachRequestArguments { ... uint32_t target_idx = UINT32_MAX; };

Then we can add a "target_idx" as an argument to the "attach" configuration in the package.json.

Since we control the "attach" config that we send here, we can easily add it. Then this code would be something like:

lldb::SBTarget target; if (args.target_idx != UINT32_MAX) { lldb::SBTarget target = dap.debugger.GetTargetAtIndex(args.target_idx); if (!target.IsValid()) error.SetErrorStringWithFormat("invalid target_idx %u in attach config", args.target_idx); } else { target = dap.CreateTarget(error); } if (error.Fail()) return ToError(error); dap.SetTarget(target);

clayborg · 2025-08-06T23:40:17Z

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

This is where I don't know how to solve the issue of getting the right debugger from our shared instance. The issue is once anyone sets the shared debugger, we will always get one back here. So if we add functionality later that allows us to follow all child processes of a parent process, they will all always share the same debugger...

qxy11 · 2025-08-08T20:56:59Z

The only time the debugger is used currently in between its creation in InitializeRequestHandler and the Launch or Attach requests is during during the TelemetryDispatcher destruction call at the end of the DAP::HandleObject call. Thus, it seemed to be that it should be pretty safe to move the debugger initialization into Launch and Attach - that way we also know whether or not to use the shared debugger based on the attach config.

In this case, since the debugger isn't created after the InitializeRequestHandler, no telemetry data is dispatched to the debugger to forward. The first time this client telemetry gets dispatched to the debugger would be after Launch now.

I did some refactoring to move some initialization code from InitializeRequestHandler into DAP API calls, and then moved the debugger initialization into Attach/LaunchRequestHandler. I also added the targetIdx config option as suggested to the attach config.

We can now launch multiple debug sessions without it sharing one debugger instance:

Currently, we need to manually stop both the native and GPU processes during debugging, but intuitively it makes sense to just disconnect the debugger once either one gets disconnected - maybe something to follow up on? For child process debugging, it makes sense to keep the debugger alive for a forked process.

qxy11 · 2025-08-08T23:57:50Z

lldb/tools/lldb-server/Plugins/AMDGPU/LLDBServerPluginAMDGPU.cpp

Will revert this once other changes are approved. Needed to set this for testing since lldb wasn't breaking at "_ZN4rocr19_loader_debug_stateEv" and properly loading dbg modules, so I couldn't test setting breakpoints, etc.

clayborg

We should make a strong mapping between target index and the debugger to use. See inlined comments for details

clayborg · 2025-08-14T18:39:25Z

lldb/tools/lldb-dap/DAP.h

+  DAPSessionManager(DAPSessionManager &&) = delete;
+  DAPSessionManager &operator=(DAPSessionManager &&) = delete;
+
+  std::mutex sessions_mutex_;


All instance variables in LLDB code will start with m_ instead of suffixing them with _. So this should be:

std::mutex m_sessions_mutex;

Ditto for all ivars below.

clayborg · 2025-08-14T18:48:43Z

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

+  // Initialize DAP debugger and related components if not sharing previously
+  // launched debugger.
+  bool use_shared_debugger = args.targetIdx != UINT32_MAX;
+  if (Error err = dap.InitializeDebugger(use_shared_debugger)) {


If we have a target index, we should just pass this into dap.InitializeDebugger(args.targetIdx). Since if we have a target, we must use its debugger.

clayborg · 2025-08-14T18:50:15Z

lldb/tools/lldb-dap/DAP.cpp

+  return llvm::Error::success();
+}
+
+llvm::Error DAP::InitializeDebugger(bool use_shared_debugger) {


We should change this to pass in the target index as an optional:

llvm::Error DAP::InitializeDebugger(std::optional<uint32_t> target_idx)

Then we can use it below.

clayborg · 2025-08-14T18:52:00Z

lldb/tools/lldb-dap/DAP.cpp

+  if (use_shared_debugger) {
+    auto shared_debugger = DAPSessionManager::GetInstance().GetSharedDebugger();
+    if (!shared_debugger) {
+      return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                     "unable to get shared debugger");
+    }
+    debugger = shared_debugger.value();
+    return StartEventThreads();
+  }


This code can then become:

if (target_idx.has_value()) auto shared_debugger = DAPSessionManager::GetInstance().GetSharedDebugger(*target_idx); if (shared_debugger) { debugger = shared_debugger.value(); return StartEventThreads(); } }

This allows us to be clear about which target must use which debugger. Also, do we need to call StartEventThreads() above? Don't we already have a thread for the old debugger that we can use?

StartEventThreads() here was just for consistency since StartEventThread() will just use the shared thread from the old debugger (so it'll be a no-op and not start up a new thread), but each DAP instance still has its own ProgressEventThread() as well that's not shared from the old debugger.

clayborg · 2025-08-14T18:54:54Z

lldb/tools/lldb-dap/DAP.cpp

+          auto target_index = debugger.GetIndexOfTarget(target);
+
+          // Set the shared debugger for GPU processes
+          DAPSessionManager::GetInstance().SetSharedDebugger(debugger);


Do we want to use the target index in this call to make sure we always get the right debugger? like:

DAPSessionManager::GetInstance().SetSharedDebugger(debugger, target_index);

This then clearly specifies that a this debugger should be used for "target_index".

clayborg · 2025-08-14T18:57:29Z

lldb/tools/lldb-dap/DAP.h

+
+  /// Optional shared debugger instance set when the native process
+  /// spawns a new GPU target
+  std::optional<lldb::SBDebugger> shared_debugger_;


This could be a map that maps target index to which debugger to use. That would make sure we get the right debugger for the target. I am thinking of if we get two GPU connections at the same time, this could fail. So if we use a map:

std::map<uint32_t, lldb::SBDebugger> m_target_to_debugger_map;

clayborg · 2025-08-14T18:59:32Z

lldb/tools/lldb-dap/DAP.h

+  void WaitForAllSessionsToDisconnect();
+
+  /// Set the shared debugger instance (only for GPU processes)
+  void SetSharedDebugger(lldb::SBDebugger debugger);


Change to make a map between target index and the right debugger to use:

void SetSharedDebugger(uint32_t target_idx, lldb::SBDebugger debugger) { m_target_to_debugger_map[target_idx] = debugger; }

I have a comment to creating m_target_to_debugger_map below

clayborg · 2025-08-14T19:00:53Z

lldb/tools/lldb-dap/DAP.h

+  void SetSharedDebugger(lldb::SBDebugger debugger);
+
+  /// Get the shared debugger instance if it exists
+  std::optional<lldb::SBDebugger> GetSharedDebugger();


Change to include the target index:

std::optional<lldb::SBDebugger> GetSharedDebugger(uint32_t target_idx) { auto pos = m_target_to_debugger_map.find(target_idx); 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; }

clayborg

I think we have some use count issues with the current design. See inlined comments. We want to verify that our event thread is being joined using some logging temporarily to make sure this design is working as expected. Full details and suggestions in inlined comments.

clayborg · 2025-08-14T20:33:29Z

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

  std::string coreFile;

+  /// Index of an existing target to attach to.
+  uint32_t targetIdx = UINT32_MAX;


Can we make this a std::optional<uint32_t> targetIdx;?

clayborg · 2025-08-15T01:16:56Z

lldb/tools/lldb-dap/DAP.h

+  std::map<lldb::user_id_t, std::shared_ptr<std::thread>>
+      m_debugger_event_threads;


We might need to make this a map where the value is a std::weak_ptr<std::thread>> otherwise our use count will always be 2 in DAP::StopEventHandlers() when there is just one DAP using the event thread right?

Thanks for the catch! The original design didn't have the manager holding references to the event thread shared_ptr, and just had one shared_ptr per DAP, but I never updated the StopEventHandlers() accordingly to account for the new changes. weak_ptr makes sense in this context!

I've updated and logged to verify that the thread is joined at the end now when disconnecting the last dap instance.

lldb/tools/lldb-dap/DAP.cpp

clayborg · 2025-08-15T01:51:26Z

lldb/tools/lldb-dap/DAP.cpp

+  }
+
+  // Create new thread and store it
+  auto new_thread =


we use the "_sp" suffix for shared pointers and "_wp" for weak pointers to make sure people know what they have. So rename new_thread to thread_sp.

clayborg · 2025-08-15T01:51:58Z

lldb/tools/lldb-dap/DAP.cpp

+  // Create new thread and store it
+  auto new_thread =
+      std::make_shared<std::thread>(&DAP::EventThread, requesting_dap);
+  m_debugger_event_threads[debugger_id] = new_thread;


This assignment will work if the value is a weak pointer, no changes needed.

clayborg · 2025-08-15T01:54:30Z

lldb/tools/lldb-dap/DAP.cpp

+  // Check if we already have a thread (most common case)
+  auto it = m_debugger_event_threads.find(debugger_id);
+  if (it != m_debugger_event_threads.end() && it->second) {
+    return it->second;


If the value is a weak pointer, we will want to call lock() on it to convert it to a shared pointer. So the code for using a weak pointer needs to be:

if (it != m_debugger_event_threads.end()) { auto thread_sp = it->second.lock(); if (thread_sp) return thread_sp; // Our weak pointer has expired m_debugger_event_threads.erase(it); }

jeffreytan81 · 2025-08-15T19:01:29Z

lldb/tools/lldb-dap/tool/lldb-dap.cpp

-  bool client_failed = false;
-  {
-    std::scoped_lock<std::mutex> lock(dap_sessions_mutex);
-    for (auto [sock, dap] : dap_sessions) {
-      if (llvm::Error error = dap->Disconnect()) {
-        client_failed = true;
-        llvm::errs() << "DAP client " << dap->transport.GetClientName()
-                     << " disconnected failed: "
-                     << llvm::toString(std::move(error)) << "\n";
-      }
-    }
-  }


I know you are doing a lot of refactoring to existing code into DAPSessionManager class. I would suggest you create a separate PR into upstream main to upstream the change. That would avoid potential merge conflicts in future considering several folks are actively refactoring/changing this in upstream.

jeffreytan81 · 2025-08-15T19:02:05Z

lldb/tools/lldb-dap/DAP.h

 using ClientFeature = protocol::ClientFeature;

+/// Global DAP session manager
+class DAPSessionManager {


Can you move this class into its own DAPSessionManager.h/DAPSessionManager.cpp?

jeffreytan81 · 2025-08-15T19:05:29Z

lldb/tools/lldb-dap/DAP.cpp

+  static DAPSessionManager *instance = nullptr;
+  if (!instance) {
+    instance = new DAPSessionManager();
+  }
+  return *instance;


Is this thread safe? I think you need mutex protection or std::call_once for it (or Double-checked locking with manual mutex lock).
Since C++11, static local variable initialization is guaranteed to be thread-safe but since you are using pointer here, I do not think C++ compiler is providing you thread safe guarantee.

Good point, it's not thread-safe since I"m initializing it to nullptr. I'll address this with a std::call_once call.

jeffreytan81 · 2025-08-15T19:13:40Z

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

+  // Initialize DAP debugger and related components if not sharing previously
+  // launched debugger.
+  std::optional<uint32_t> target_idx = args.targetIdx;
+  if (Error err = dap.InitializeDebugger(target_idx)) {


This delays InitializeDebugger from InitializeRequestHandler to Launch/Attach time. Is this timing/semantics changing required? How do we know this is safe to do without causing issues?

This change was required there's not any way to pass additional info to the "initialize" packet from the startDebugging request since we aren't the ones sending it, so we can only know whether to use the shared debugger instance at attach time. Otherwise, we'd end up always using a shared debugger, and run into the issue where all subsequent debugging sessions launched would still be under the same shared debugger instance and run the same event thread, when we only want this for the GPU case.

I did some manual testing by attaching lldb to the lldb-dap process, and the initialized debugger isn't used for anything (other than telemetry that requires a debugger to exist) until the Launch/Attach time (more details here #26 (comment)), so the change seemed safe.

The other option for solving this issue would've been having some sort of counter to decrement once the shared debugger instance was used for a GPU child session, but that option would've been racy.

jeffreytan81 · 2025-08-18T18:50:16Z

lldb/tools/lldb-dap/DAP.cpp

+  if (event_thread_sp && event_thread_sp.use_count() == 1 &&
+      event_thread_sp->joinable()) {


This is not thread-safe -- when the last two clients both are calling DAP::StopEventHandlers at the same time, there are a race condition that both will find use_count > 1 and miss the broadcast to stop the event thread.

We need to either add the locking or probably can use std::shared_ptr constructor's deleter to do the broadcaster and join code which will be called when target object is deleted.

Summary: This should allow the shared_ptr destruction to take care of broadcasting events and joining the event thread() on the last reference to it

walter-erquinigo · 2025-08-28T15:36:12Z

@qxy11 , please let us know when this is ready for review again :)

qxy11 · 2025-08-28T16:04:00Z

@qxy11 , please let us know when this is ready for review again :)

@walter-erquinigo - it should be ready for another review, all previous comments should've been addressed.

walter-erquinigo · 2025-09-03T15:02:47Z

I'm about to review this, but I have the overall sense that these changes are big and non-trivial, and keeping them in this fork will be a burden when doing rebases.
So, why don't we take this upstream after we finish reviewing it here? We don't need to mention GPU debugging, but we can mention that we have some workflows that spawn programmatically new targets, and that plan to use these changes for debugging child processes. Greg and I are the "owners" of lldb-dap according to the llvm docs, so we can just approve it without further justifications.

walter-erquinigo · 2025-09-03T15:08:41Z

lldb/include/lldb/API/SBTarget.h

    eBroadcastBitWatchpointChanged = (1 << 3),
    eBroadcastBitSymbolsLoaded = (1 << 4),
    eBroadcastBitSymbolsChanged = (1 << 5),
+    eBroadcastBitNewTargetSpawned = (1 << 6),


I think NewTargetCreated would be a better name

walter-erquinigo · 2025-09-03T15:09:39Z

lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py

+    def _handle_startDebugging_request(self, packet):
+        response = {
+            "type": "response",
+            "request_seq": packet.get("seq", 0),


i don't think you should have a default seq, as you should always get one. packet["seq"] should be fine in this case

walter-erquinigo · 2025-09-03T15:19:58Z

lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py

        sourceMap: Optional[Union[list[tuple[str, str]], dict[str, str]]] = None,
        gdbRemotePort: Optional[int] = None,
        gdbRemoteHostname: Optional[str] = None,
+        targetIdx: Optional[int] = None,


We shouldn't use target index for this. It's not guaranteed to be unique per target for an entire session. For example, see this sequence:

you create two targets. They will have index 0 and 1

you delete target 1

then you create a new target. It will have index 1 again

This might result in race conditions.

I think that should use create an actual unique id for each target as part of its creation, then you can use it to select the target you want as part of the reverse request.

walter-erquinigo · 2025-09-03T15:26:18Z

lldb/test/API/tools/lldb-dap/gpu/TestDAP_gpu_reverse_request.py

+def _detect_rocm():
+    """Detects rocm target."""
+    try:
+        proc = Popen(["rocminfo"], stdout=PIPE, stderr=PIPE)
+        return "amd"
+    except Exception:
+        return None
+
+def skipUnlessHasROCm(func):
+    """Decorate the item to skip test unless ROCm is available."""
+
+    def has_rocm():
+        if _detect_rocm() is None:
+            return "ROCm not available (rocm-smi not found)"
+        return None
+
+    return skipTestIfFn(has_rocm)(func)
+
+


move this file into a new folder called amd within lldb-dap/gpu

walter-erquinigo · 2025-09-03T17:38:53Z

lldb/test/API/tools/lldb-dap/gpu/TestDAP_gpu_reverse_request.py

+    return skipTestIfFn(has_rocm)(func)
+
+
+class TestDAP_gpu_reverse_request(lldbdap_testcase.DAPTestCaseBase):


add AMD to the name of the class, also don't use underscores

walter-erquinigo · 2025-09-03T17:50:30Z

lldb/tools/lldb-dap/DAP.cpp

+          // process name
+
+          attach_config.try_emplace("type", "lldb");
+          attach_config.try_emplace("name", "GPU Session");


This name has to be configurable by the creator of the target. It might be useful to encode a name in the event or in the target itself. For example, in the GPUAction handler where we create the GPU target, we should be able to specify a custom name for the target that the session will pick up. A default name like Session <target id> might be okay as a fallback.
This functionality should be not just for GPUs, as mentioned above.

walter-erquinigo · 2025-09-03T17:51:51Z

lldb/tools/lldb-dap/DAP.cpp

+          llvm::json::Object start_debugging_args;
+          start_debugging_args.try_emplace("request", "attach");
+          start_debugging_args.try_emplace("configuration",
+                                           std::move(attach_config));


This might simplify this code pattern

Suggested change

llvm::json::Object start_debugging_args;

start_debugging_args.try_emplace("request", "attach");

start_debugging_args.try_emplace("configuration",

std::move(attach_config));

llvm::json::Object start_debugging_args {

{"request", "attach"},

{"configuration", std::move(attach_config)}

};

walter-erquinigo · 2025-09-03T17:52:40Z

lldb/tools/lldb-dap/DAP.cpp

+        DAP *dap_instance =
+            DAPSessionManager::GetInstance().FindDAPForTarget(event_target);


FindDAPForTarget is used very often, you might as well create a static override that will under the hood invoke GetInstance(), just to simplify the callers

walter-erquinigo · 2025-09-03T17:53:05Z

lldb/tools/lldb-dap/DAP.cpp

-        SendOutput(OutputType::Important,
-                   llvm::formatv("{0}: {1}", type, message).str());
+        // Global debugger events - send to all DAP instances
+        std::vector<DAP *> active_instances =


walter-erquinigo · 2025-09-03T17:56:01Z

lldb/tools/lldb-dap/DAP.cpp

+          auto target_index = debugger.GetIndexOfTarget(target);
+
+          // Set the shared debugger for GPU processes
+          DAPSessionManager::GetInstance().SetSharedDebugger(target_index,


I think you can avoid storing a debugger per target index if you just store the Target object, and you can query its underlying debugger whenever you initialize the new debug session.

I was looking into switching to this, but it seems like it might be a less clear API pairing since we'd still have to store a map from target ID -> target in some sort of StoreTarget function, rather than target ID -> debugger, but now we'd have to query the underlying debugger whenever we call GetSharedDebugger rather than directly getting it, and it doesn't seem like there's much of an overhead difference between storing just the pointer to the target vs. the debugger?

…d encode the name into the target event

… broadcast bit to eBroadcastBitNewTargetCreated and p

qxy11 · 2025-09-11T06:22:25Z

I'm about to review this, but I have the overall sense that these changes are big and non-trivial, and keeping them in this fork will be a burden when doing rebases. So, why don't we take this upstream after we finish reviewing it here? We don't need to mention GPU debugging, but we can mention that we have some workflows that spawn programmatically new targets, and that plan to use these changes for debugging child processes. Greg and I are the "owners" of lldb-dap according to the llvm docs, so we can just approve it without further justifications.

Sounds good! Once changes here look good, I'll put up an upstream diff with the non-gpu specific changes.

walter-erquinigo

Overall this looks pretty good. There might be a few things to improve, but please start upstreaming the non-gpu parts and i'll provide specific feedback to those patches.
Thanks for all of this :)

walter-erquinigo · 2025-09-15T15:58:08Z

lldb/source/Target/TargetList.cpp

You can't really do that because you can have a target without a process. It would be better to just assign a proper unique ID to the target class

walter-erquinigo · 2025-09-15T15:58:41Z

lldb/source/Utility/GPUGDBRemotePackets.cpp

just call it session name. This should be somewhat generic

- Resolved conflicts in GPUGDBRemotePackets.h and .cpp to support both session_name and stop_id fields - Resolved conflicts in LLDBServerPluginAMDGPU.cpp to maintain dual connection functionality - Combined latest GPU plugin improvements with DAP dual connection features

clayborg requested changes Aug 6, 2025

View reviewed changes

qxy11 commented Aug 8, 2025

View reviewed changes

clayborg force-pushed the llvm-server-plugins branch from 5dd859e to db4c6e6 Compare August 10, 2025 23:23

qxy11 force-pushed the dap-dual-connection branch from 928542d to 1170435 Compare August 11, 2025 22:54

qxy11 added 7 commits August 14, 2025 11:03

Integration w/ LLDB-DAP using shared event thread

eb22c2f

Add targetIdx to attach config

412cf86

Move debugger construction to Launch/AttachRequest

dfefa98

Add comment, move static variable

bf3fb54

Undo temp fix for LLDBServerPluginAMDGPU

2a3b3b0

Add lldb-dap unit tests for gpu

58c7548

Format

770558b

qxy11 force-pushed the dap-dual-connection branch from 1170435 to 770558b Compare August 14, 2025 18:15

Fix the hip file compilation

32f791d

clayborg requested changes Aug 14, 2025

View reviewed changes

qxy11 added 2 commits August 14, 2025 12:36

Rename instance variables w/ lldb convention

c41920b

Stronger mapping from targetIdx to debugger

e9dc33b

clayborg requested changes Aug 15, 2025

View reviewed changes

Use weak reference to event handler in DAPSessionManager

2b5436a

jeffreytan81 reviewed Aug 15, 2025

View reviewed changes

Move DAPSessionManager into its own file and std::once in GetInstance()

028eddd

jeffreytan81 reviewed Aug 18, 2025

View reviewed changes

Address thread safety issue w/ RAII ManagedEventThread

9cc4af8

Summary: This should allow the shared_ptr destruction to take care of broadcasting events and joining the event thread() on the last reference to it

walter-erquinigo self-requested a review August 28, 2025 15:35

walter-erquinigo requested changes Sep 3, 2025

View reviewed changes

qxy11 added 2 commits September 10, 2025 22:47

Add session name to GPUActions to make it configurable per plugin, an…

afd3fe1

…d encode the name into the target event

Pass configured session names to reverse request DAP instance, rename…

aae9921

… broadcast bit to eBroadcastBitNewTargetCreated and p

qxy11 added 3 commits September 10, 2025 22:47

Assign unique target IDs to reverse attach to

69d841c

Move and fix tests for unique target ids

f18bd67

Add static convenience method for FindDAP + lint

1868fe2

qxy11 requested a review from walter-erquinigo September 11, 2025 06:32

walter-erquinigo reviewed Sep 15, 2025

View reviewed changes

qxy11 and others added 3 commits September 19, 2025 10:35

Unique target ids + dap_session_name -> session_name

ffb3a53

Fix attach request handling

01d14b1

qxy11 force-pushed the dap-dual-connection branch from 2c5f647 to 53b2074 Compare September 19, 2025 17:36

		std::map<lldb::user_id_t, std::shared_ptr<std::thread>>
		m_debugger_event_threads;

		if (event_thread_sp && event_thread_sp.use_count() == 1 &&
		event_thread_sp->joinable()) {

		return skipTestIfFn(has_rocm)(func)


		class TestDAP_gpu_reverse_request(lldbdap_testcase.DAPTestCaseBase):

		DAP *dap_instance =
		DAPSessionManager::GetInstance().FindDAPForTarget(event_target);

Integrate dual connection w/ LLDB-DAP using shared event thread #26

Are you sure you want to change the base?

Integrate dual connection w/ LLDB-DAP using shared event thread #26

Uh oh!

Conversation

qxy11 commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qxy11 commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qxy11 Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clayborg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clayborg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qxy11 Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qxy11 commented Aug 6, 2025 •

edited

Loading

qxy11 commented Aug 8, 2025 •

edited

Loading

qxy11 Aug 8, 2025 •

edited

Loading

qxy11 Aug 15, 2025 •

edited

Loading

qxy11 commented Aug 28, 2025 •

edited

Loading