https://github.com/ashgti created https://github.com/llvm/llvm-project/pull/140142
The recent change to the startup flow has highlighted a problem with breakpoint events being missed. Previously, we would handle `attach`/`launch` commands immediately, leaving the process in a suspended until the `configurationDone` command was sent. If a breakpoint was set between the `attach`/`launch` and the `configurationDone` request, it may have been resolved immediately, but there was a chance the module was not yet loaded. With the new startup flow, the breakpoint is always set before the target is loaded. For some targets, this is fine and the breakpoint event fires eventually and marks the breakpoint as verified. However, if a `attach`/`launch` request has `attachCommands`/`launchCommands` that create a new target then we will miss the breakpoint events associated with that target because we haven't configured the target broadcaster for the debugger. We only configure that in the `DAP::SetTarget` call, which happens after the events would have occurred. To address this, I adjusted the `DAP::EventThread` to listen for the broadcast class instead of an individual broadcaster. I think this may also fix the unstable `TestDAP_breakpointEvents` tests. We're not using the `Debugger::DefaultEventHandler` https://github.com/llvm/llvm-project/blob/090f46d8d246762401c41c5486dde299382d6c90/lldb/source/Core/Debugger.cpp#L2048 which is why these broadcast classes are not registered on our debugger instance in lldb-dap. >From ea5ee538a1ca384725f8166ec9289883683b52ce Mon Sep 17 00:00:00 2001 From: John Harrison <harj...@google.com> Date: Thu, 15 May 2025 14:13:48 -0700 Subject: [PATCH] [lldb-dap] Listen for broadcast classes. The recent change to the startup flow has highlighted a problem with breakpoint events being missed. Previously, we would handle `attach`/`launch` commands immediately, leaving the process in a suspended until the `configurationDone` command was sent. If a breakpoint was set between the `attach`/`launch` and the `configurationDone` request, it may have been resolved immediately, but there was a chance the module was not yet loaded. With the new startup flow, the breakpoint is always set before the target is loaded. For some targets, this is fine and the breakpoint event fires eventually and marks the breakpoint as verified. However, if a `attach`/`launch` request has `attachCommands`/`launchCommands` that create a new target then we will miss the breakpoint events associated with that target because we haven't configured the target broadcaster for the debugger. We only configure that in the `DAP::SetTarget` call, which happens after the events would have occurred. To address this, I adjusted the `DAP::EventThread` to listen for the broadcast class instead of an individual broadcaster. I think this may also fix the unstable `TestDAP_breakpointEvents` tests. --- lldb/tools/lldb-dap/DAP.cpp | 53 +++++++++---------- lldb/tools/lldb-dap/DAP.h | 3 +- .../lldb-dap/Handler/AttachRequestHandler.cpp | 4 +- .../tools/lldb-dap/Handler/RequestHandler.cpp | 2 +- 4 files changed, 31 insertions(+), 31 deletions(-) diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp index 51f9da854f4b6..827ebb1465938 100644 --- a/lldb/tools/lldb-dap/DAP.cpp +++ b/lldb/tools/lldb-dap/DAP.cpp @@ -26,6 +26,7 @@ #include "lldb/API/SBListener.h" #include "lldb/API/SBProcess.h" #include "lldb/API/SBStream.h" +#include "lldb/API/SBTarget.h" #include "lldb/Utility/IOObject.h" #include "lldb/Utility/Status.h" #include "lldb/lldb-defines.h" @@ -719,23 +720,7 @@ lldb::SBTarget DAP::CreateTarget(lldb::SBError &error) { return target; } -void DAP::SetTarget(const lldb::SBTarget target) { - this->target = target; - - if (target.IsValid()) { - // Configure breakpoint event listeners for the target. - lldb::SBListener listener = this->debugger.GetListener(); - listener.StartListeningForEvents( - this->target.GetBroadcaster(), - lldb::SBTarget::eBroadcastBitBreakpointChanged | - lldb::SBTarget::eBroadcastBitModulesLoaded | - lldb::SBTarget::eBroadcastBitModulesUnloaded | - lldb::SBTarget::eBroadcastBitSymbolsLoaded | - lldb::SBTarget::eBroadcastBitSymbolsChanged); - listener.StartListeningForEvents(this->broadcaster, - eBroadcastBitStopEventThread); - } -} +void DAP::SetTarget(const lldb::SBTarget target) { this->target = target; } bool DAP::HandleObject(const Message &M) { TelemetryDispatcher dispatcher(&debugger); @@ -1489,17 +1474,31 @@ void DAP::ProgressEventThread() { } // All events from the debugger, target, process, thread and frames are -// received in this function that runs in its own thread. We are using a -// "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. +// received in this function that runs in its own thread. void DAP::EventThread() { llvm::set_thread_name(transport.GetClientName() + ".event_handler"); - lldb::SBEvent event; + + // Configure the debugger listener for all events lldb-dap is interested in. lldb::SBListener listener = debugger.GetListener(); - broadcaster.AddListener(listener, eBroadcastBitStopEventThread); - debugger.GetBroadcaster().AddListener( - listener, lldb::eBroadcastBitError | lldb::eBroadcastBitWarning); + listener.StartListeningForEventClass( + debugger, lldb::SBTarget::GetBroadcasterClassName(), + lldb::SBTarget::eBroadcastBitBreakpointChanged | + lldb::SBTarget::eBroadcastBitModulesLoaded | + lldb::SBTarget::eBroadcastBitModulesUnloaded | + lldb::SBTarget::eBroadcastBitSymbolsLoaded | + lldb::SBTarget::eBroadcastBitSymbolsChanged); + listener.StartListeningForEventClass( + debugger, lldb::SBProcess::GetBroadcasterClassName(), + lldb::SBProcess::eBroadcastBitStateChanged | + lldb::SBProcess::eBroadcastBitSTDOUT | + lldb::SBProcess::eBroadcastBitSTDERR); + listener.StartListeningForEvents(debugger.GetBroadcaster(), + lldb::SBDebugger::eBroadcastBitError | + lldb::SBDebugger::eBroadcastBitWarning); + // Listen for the lldb-dap stop event. + listener.StartListeningForEvents(broadcaster, eBroadcastBitStopEventThread); + + lldb::SBEvent event; bool done = false; while (!done) { if (listener.WaitForEvent(1, event)) { @@ -1633,8 +1632,8 @@ void DAP::EventThread() { SendJSON(llvm::json::Value(std::move(bp_event))); } } - } else if (event_mask & lldb::eBroadcastBitError || - event_mask & lldb::eBroadcastBitWarning) { + } else if (event_mask & lldb::SBDebugger::eBroadcastBitError || + event_mask & lldb::SBDebugger::eBroadcastBitWarning) { lldb::SBStructuredData data = lldb::SBDebugger::GetDiagnosticFromEvent(event); if (!data.IsValid()) diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h index c2e4c2dea582e..c76f6d2ba1ffc 100644 --- a/lldb/tools/lldb-dap/DAP.h +++ b/lldb/tools/lldb-dap/DAP.h @@ -334,8 +334,7 @@ struct DAP { /// An SBTarget object. lldb::SBTarget CreateTarget(lldb::SBError &error); - /// Set given target object as a current target for lldb-dap and start - /// listeing for its breakpoint events. + /// Set given target object as a current target for lldb-dap. void SetTarget(const lldb::SBTarget target); bool HandleObject(const protocol::Message &M); diff --git a/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp index 0293ffbd0c922..167ddb9eb73ed 100644 --- a/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp @@ -96,7 +96,9 @@ Error AttachRequestHandler::Run(const AttachRequestArguments &args) const { if (llvm::Error err = dap.RunAttachCommands(args.attachCommands)) return err; - dap.target = dap.debugger.GetSelectedTarget(); + // The custom commands might have created a new target so we should use + // the selected target after these commands are run. + dap.SetTarget(dap.debugger.GetSelectedTarget()); // Validate the attachCommand results. if (!dap.target.GetProcess().IsValid()) diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp index 93bc80a38e29d..7de582ee94320 100644 --- a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp @@ -209,7 +209,7 @@ llvm::Error BaseRequestHandler::LaunchProcess( // The custom commands might have created a new target so we should use // the selected target after these commands are run. - dap.target = dap.debugger.GetSelectedTarget(); + dap.SetTarget(dap.debugger.GetSelectedTarget()); } } _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
