https://github.com/JDevlieghere created https://github.com/llvm/llvm-project/pull/138219
This PR changes how we treat the launch sequence in lldb-dap. - Send the initialized event after we finish handling the initialize request, rather than after we finish attaching or launching. - Delay handling the launch and attach request until we have received the configurationDone request. The latter is now largely a NO-OP and only exists to signal lldb-dap that it can handle the launch and attach requests. - Add synchronization for ignoring the first stop event. I originally wanted to do all the launching and attaching in asynchronous mode, but that's a little bit trickier than expected. On macOS, I'm getting an additional stop when we hit the dyld breakpoint. This might be something I come back to, but for now I'm keeping the old behavior with the proposed synchronization from #137920. Background: https://discourse.llvm.org/t/reliability-of-the-lldb-dap-tests/86125 >From 730fdebae55f38e2d1456101807836bd001aec0b Mon Sep 17 00:00:00 2001 From: Jonas Devlieghere <jo...@devlieghere.com> Date: Thu, 1 May 2025 15:11:48 -0700 Subject: [PATCH] [lldb-dap] Change the launch sequence This PR changes how we treat the launch sequence in lldb-dap. - Send the initialized event after we finish handling the initialize request, rather than after we finish attaching or launching. - Delay handling the launch and attach request until we have received the configurationDone request. The latter is now largely a NO-OP and only exists to signal lldb-dap that it can handle the launch and attach requests. - Add synchronization for ignoring the first stop event. I originally wanted to do all the launching and attaching in asynchronous mode, but that's a little bit trickier than expected. On macOS, I'm getting an additional stop when we hit the dyld breakpoint. This might be something I come back to, but for now I'm keeping the old behavior with the proposed synchronization from #137920. Background: https://discourse.llvm.org/t/reliability-of-the-lldb-dap-tests/86125 --- lldb/tools/lldb-dap/DAP.cpp | 37 +++++++++++++------ lldb/tools/lldb-dap/DAP.h | 14 ++++++- lldb/tools/lldb-dap/EventHelper.cpp | 2 +- .../lldb-dap/Handler/AttachRequestHandler.cpp | 28 +++++++++++--- .../ConfigurationDoneRequestHandler.cpp | 14 +------ .../Handler/InitializeRequestHandler.cpp | 29 +++++++++------ .../lldb-dap/Handler/LaunchRequestHandler.cpp | 2 - .../tools/lldb-dap/Handler/RequestHandler.cpp | 30 ++++++++++++--- lldb/tools/lldb-dap/Handler/RequestHandler.h | 1 + 9 files changed, 106 insertions(+), 51 deletions(-) diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp index 4cb0d8e49004c..237b16f8c45a9 100644 --- a/lldb/tools/lldb-dap/DAP.cpp +++ b/lldb/tools/lldb-dap/DAP.cpp @@ -84,8 +84,8 @@ DAP::DAP(Log *log, const ReplMode default_repl_mode, : log(log), transport(transport), broadcaster("lldb-dap"), exception_breakpoints(), focus_tid(LLDB_INVALID_THREAD_ID), stop_at_entry(false), is_attach(false), - restarting_process_id(LLDB_INVALID_PROCESS_ID), - configuration_done_sent(false), waiting_for_run_in_terminal(false), + restarting_process_id(LLDB_INVALID_PROCESS_ID), configuration_done(false), + waiting_for_run_in_terminal(false), progress_event_reporter( [&](const ProgressEvent &event) { SendJSON(event.ToJSON()); }), reverse_request_seq(0), repl_mode(default_repl_mode) { @@ -892,11 +892,14 @@ llvm::Error DAP::Loop() { errWrapper.SetErrorString(llvm::toString(std::move(err)).c_str()); return errWrapper; } + bool is_launch_attach = false; if (const protocol::Request *req = - std::get_if<protocol::Request>(&*next); - req && req->command == "disconnect") { - disconnecting = true; + std::get_if<protocol::Request>(&*next)) { + if (req->command == "disconnect") + disconnecting = true; + if (req->command == "launch" || req->command == "attach") + is_launch_attach = true; } const std::optional<CancelArguments> cancel_args = @@ -924,7 +927,10 @@ llvm::Error DAP::Loop() { { std::lock_guard<std::mutex> guard(m_queue_mutex); - m_queue.push_back(std::move(*next)); + if (is_launch_attach) + m_launch_attach_queue.push_back(std::move(*next)); + else + m_queue.push_back(std::move(*next)); } m_queue_cv.notify_one(); } @@ -938,15 +944,24 @@ llvm::Error DAP::Loop() { StopEventHandlers(); }); - while (!disconnecting || !m_queue.empty()) { + while (true) { std::unique_lock<std::mutex> lock(m_queue_mutex); - m_queue_cv.wait(lock, [&] { return disconnecting || !m_queue.empty(); }); + m_queue_cv.wait(lock, [&] { + return disconnecting || !m_queue.empty() || + (configuration_done && !m_launch_attach_queue.empty()); + }); - if (m_queue.empty()) + if (disconnecting) break; - Message next = m_queue.front(); - m_queue.pop_front(); + auto &active_queue = configuration_done && !m_launch_attach_queue.empty() + ? m_launch_attach_queue + : m_queue; + + assert(!active_queue.empty()); + + Message next = active_queue.front(); + active_queue.pop_front(); if (!HandleObject(next)) return llvm::createStringError(llvm::inconvertibleErrorCode(), diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h index 88eedb0860cf1..65c2426aafe40 100644 --- a/lldb/tools/lldb-dap/DAP.h +++ b/lldb/tools/lldb-dap/DAP.h @@ -188,7 +188,7 @@ struct DAP { // shutting down the entire adapter. When we're restarting, we keep the id of // the old process here so we can detect this case and keep running. lldb::pid_t restarting_process_id; - bool configuration_done_sent; + bool configuration_done; llvm::StringMap<std::unique_ptr<BaseRequestHandler>> request_handlers; bool waiting_for_run_in_terminal; ProgressEventReporter progress_event_reporter; @@ -214,6 +214,13 @@ struct DAP { /// The initial thread list upon attaching. std::optional<llvm::json::Array> initial_thread_list; + /// Synchronization of stop events between the event and request thread. + /// @{ + std::mutex ignore_stop_mutex; + std::condition_variable ignore_stop_cv; + bool ignore_next_stop; + /// @} + /// Creates a new DAP sessions. /// /// \param[in] log @@ -417,8 +424,11 @@ struct DAP { lldb::SBMutex GetAPIMutex() const { return target.GetAPIMutex(); } private: - std::mutex m_queue_mutex; + /// Queue for all incoming messages. std::deque<protocol::Message> m_queue; + /// Dedicated queue for launching and attaching. + std::deque<protocol::Message> m_launch_attach_queue; + std::mutex m_queue_mutex; std::condition_variable m_queue_cv; std::mutex m_cancelled_requests_mutex; diff --git a/lldb/tools/lldb-dap/EventHelper.cpp b/lldb/tools/lldb-dap/EventHelper.cpp index 2c659f39f4b66..ed2d8700c26b0 100644 --- a/lldb/tools/lldb-dap/EventHelper.cpp +++ b/lldb/tools/lldb-dap/EventHelper.cpp @@ -222,7 +222,7 @@ void SendContinuedEvent(DAP &dap) { // If the focus thread is not set then we haven't reported any thread status // to the client, so nothing to report. - if (!dap.configuration_done_sent || dap.focus_tid == LLDB_INVALID_THREAD_ID) { + if (!dap.configuration_done || dap.focus_tid == LLDB_INVALID_THREAD_ID) { return; } diff --git a/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp index 7084d30f2625b..8ded3c25f0a80 100644 --- a/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/AttachRequestHandler.cpp @@ -128,6 +128,9 @@ void AttachRequestHandler::operator()(const llvm::json::Object &request) const { return; } + std::unique_lock<std::mutex> lock(dap.ignore_stop_mutex); + dap.ignore_next_stop = !dap.stop_at_entry; + if ((pid == LLDB_INVALID_PROCESS_ID || gdb_remote_port == invalid_port) && wait_for) { char attach_msg[256]; @@ -181,9 +184,24 @@ void AttachRequestHandler::operator()(const llvm::json::Object &request) const { // selected target after these commands are run. dap.target = dap.debugger.GetSelectedTarget(); - // Make sure the process is attached and stopped before proceeding as the - // the launch commands are not run using the synchronous mode. - error = dap.WaitForProcessToStop(std::chrono::seconds(timeout_seconds)); + } + + // Notify the event thread that it should ignore the next stop event. + lock.unlock(); + dap.ignore_stop_cv.notify_one(); + + // Make sure the process is attached and stopped before proceeding. + error = dap.WaitForProcessToStop(std::chrono::seconds(timeout_seconds)); + + // FIXME: Unify this between launch & attach. + if (dap.stop_at_entry) { + // Client requests the baseline of currently existing threads after + // a successful launch or attach by sending a 'threads' request + // right after receiving the configurationDone response. + // Obtain the list of threads before we resume the process + dap.initial_thread_list = + GetThreads(dap.target.GetProcess(), dap.thread_format); + dap.target.GetProcess().Continue(); } if (error.Success() && core_file.empty()) { @@ -204,10 +222,8 @@ void AttachRequestHandler::operator()(const llvm::json::Object &request) const { } dap.SendJSON(llvm::json::Value(std::move(response))); - if (error.Success()) { + if (error.Success()) SendProcessEvent(dap, Attach); - dap.SendJSON(CreateEventObject("initialized")); - } } } // namespace lldb_dap diff --git a/lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp index f39bbdefdbb95..a10cd0c832782 100644 --- a/lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/ConfigurationDoneRequestHandler.cpp @@ -47,21 +47,11 @@ namespace lldb_dap { void ConfigurationDoneRequestHandler::operator()( const llvm::json::Object &request) const { + dap.configuration_done = true; + llvm::json::Object response; FillResponse(request, response); dap.SendJSON(llvm::json::Value(std::move(response))); - dap.configuration_done_sent = true; - if (dap.stop_at_entry) - SendThreadStoppedEvent(dap); - else { - // Client requests the baseline of currently existing threads after - // a successful launch or attach by sending a 'threads' request - // right after receiving the configurationDone response. - // Obtain the list of threads before we resume the process - dap.initial_thread_list = - GetThreads(dap.target.GetProcess(), dap.thread_format); - dap.target.GetProcess().Continue(); - } } } // namespace lldb_dap diff --git a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp index ce34c52bcc334..2214c7846217d 100644 --- a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp @@ -161,20 +161,22 @@ static void EventThreadFunction(DAP &dap) { case lldb::eStateSuspended: break; case lldb::eStateStopped: - // We launch and attach in synchronous mode then the first stop - // event will not be delivered. If we use "launchCommands" during a - // launch or "attachCommands" during an attach we might some process - // stop events which we do not want to send an event for. We will - // manually send a stopped event in request_configurationDone(...) - // so don't send any before then. - if (dap.configuration_done_sent) { - // Only report a stopped event if the process was not - // automatically restarted. - if (!lldb::SBProcess::GetRestartedFromEvent(event)) { - SendStdOutStdErr(dap, process); + { + std::unique_lock<std::mutex> lock(dap.ignore_stop_mutex); + if (dap.ignore_next_stop) { + DAP_LOG(dap.log, "Ignoring process stop event"); SendThreadStoppedEvent(dap); + dap.ignore_next_stop = false; + continue; } } + + // Only report a stopped event if the process was not + // automatically restarted. + if (!lldb::SBProcess::GetRestartedFromEvent(event)) { + SendStdOutStdErr(dap, process); + SendThreadStoppedEvent(dap); + } break; case lldb::eStateRunning: dap.WillContinue(); @@ -284,6 +286,7 @@ llvm::Expected<InitializeResponseBody> InitializeRequestHandler::Run( // Do not source init files until in/out/err are configured. dap.debugger = lldb::SBDebugger::Create(false); dap.debugger.SetInputFile(dap.in); + dap.target = dap.debugger.GetDummyTarget(); llvm::Expected<int> out_fd = dap.out.GetWriteFileDescriptor(); if (!out_fd) @@ -338,4 +341,8 @@ llvm::Expected<InitializeResponseBody> InitializeRequestHandler::Run( return dap.GetCapabilities(); } +void InitializeRequestHandler::PostRun() const { + dap.SendJSON(CreateEventObject("initialized")); +} + } // namespace lldb_dap diff --git a/lldb/tools/lldb-dap/Handler/LaunchRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/LaunchRequestHandler.cpp index 3e4532e754ec6..ffd61b040e55f 100644 --- a/lldb/tools/lldb-dap/Handler/LaunchRequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/LaunchRequestHandler.cpp @@ -72,8 +72,6 @@ void LaunchRequestHandler::PostRun() const { // Attach happens when launching with runInTerminal. SendProcessEvent(dap, dap.is_attach ? Attach : Launch); } - - dap.SendJSON(CreateEventObject("initialized")); } } // namespace lldb_dap diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp index 7a75cd93abc19..a3e75e35c7dc6 100644 --- a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp @@ -238,6 +238,9 @@ llvm::Error BaseRequestHandler::LaunchProcess( launch_info.SetLaunchFlags(flags | lldb::eLaunchFlagDebug | lldb::eLaunchFlagStopAtEntry); + std::unique_lock<std::mutex> lock(dap.ignore_stop_mutex); + dap.ignore_next_stop = !dap.stop_at_entry; + if (arguments.runInTerminal) { if (llvm::Error err = RunInTerminal(dap, arguments)) return err; @@ -259,12 +262,27 @@ 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(); - // Make sure the process is launched and stopped at the entry point before - // proceeding as the launch commands are not run using the synchronous - // mode. - lldb::SBError error = dap.WaitForProcessToStop(arguments.timeout); - if (error.Fail()) - return llvm::make_error<DAPError>(error.GetCString()); + } + + // Notify the event thread that it should ignore the next stop event. + lock.unlock(); + dap.ignore_stop_cv.notify_one(); + + // Make sure the process is launched and stopped at the entry point before + // proceeding. + lldb::SBError error = dap.WaitForProcessToStop(arguments.timeout); + if (error.Fail()) + return llvm::make_error<DAPError>(error.GetCString()); + + // FIXME: Unify this between launch & attach. + if (!dap.stop_at_entry) { + // Client requests the baseline of currently existing threads after + // a successful launch or attach by sending a 'threads' request + // right after receiving the configurationDone response. + // Obtain the list of threads before we resume the process + dap.initial_thread_list = + GetThreads(dap.target.GetProcess(), dap.thread_format); + dap.target.GetProcess().Continue(); } return llvm::Error::success(); diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h index fa3d76ed4a125..dec35c5dce1e5 100644 --- a/lldb/tools/lldb-dap/Handler/RequestHandler.h +++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h @@ -282,6 +282,7 @@ class InitializeRequestHandler static llvm::StringLiteral GetCommand() { return "initialize"; } llvm::Expected<protocol::InitializeResponseBody> Run(const protocol::InitializeRequestArguments &args) const override; + void PostRun() const override; }; class LaunchRequestHandler _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
