llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Hu Jialun (SuibianP) <details> <summary>Changes</summary> `runInTerminal` is currently implemented using JSON messages over FIFOs, which feels too clunky for the simple job of passing a PID. Instead, take advantage of `si_pid` available from `sa_sigaction` handler, and send a user signal instead. Both synchronisation and timeout are preserved with the protocol below, 1. Debugger waits for `SIGUSR1` under timeout with pselect 2. Launcher forks into child, sends `SIGUSR1` to debugger and suspends 3. Debugger receives `SIGUSR1`, captures the sender (launcher child) PID, attaches to and resumes it 4. Launcher child resumes and `exec`'s into target 5. Launcher parent `waitpid`'s and kills irresponsive child after timeout With this, the entirety of `FifoFiles` and `RunInTerminal` can be dropped. Refs: https://github.com/llvm/llvm-project/pull/121269#issuecomment-2901401482 --- Patch is 36.38 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/142374.diff 11 Files Affected: - (modified) lldb/test/API/tools/lldb-dap/runInTerminal/TestDAP_runInTerminal.py (-117) - (modified) lldb/tools/lldb-dap/CMakeLists.txt (-2) - (removed) lldb/tools/lldb-dap/FifoFiles.cpp (-101) - (removed) lldb/tools/lldb-dap/FifoFiles.h (-85) - (modified) lldb/tools/lldb-dap/Handler/RequestHandler.cpp (+72-27) - (modified) lldb/tools/lldb-dap/JSONUtils.cpp (+2-3) - (modified) lldb/tools/lldb-dap/JSONUtils.h (+1-4) - (modified) lldb/tools/lldb-dap/Options.td (+5-10) - (removed) lldb/tools/lldb-dap/RunInTerminal.cpp (-170) - (removed) lldb/tools/lldb-dap/RunInTerminal.h (-131) - (modified) lldb/tools/lldb-dap/tool/lldb-dap.cpp (+43-44) ``````````diff diff --git a/lldb/test/API/tools/lldb-dap/runInTerminal/TestDAP_runInTerminal.py b/lldb/test/API/tools/lldb-dap/runInTerminal/TestDAP_runInTerminal.py index 65c931210d400..3b6571621e541 100644 --- a/lldb/test/API/tools/lldb-dap/runInTerminal/TestDAP_runInTerminal.py +++ b/lldb/test/API/tools/lldb-dap/runInTerminal/TestDAP_runInTerminal.py @@ -131,120 +131,3 @@ def test_runInTerminalInvalidTarget(self): "'INVALIDPROGRAM' does not exist", response["body"]["error"]["format"], ) - - @skipIfWindows - @skipIf(oslist=["linux"], archs=no_match(["x86_64"])) - def test_missingArgInRunInTerminalLauncher(self): - if not self.isTestSupported(): - return - proc = subprocess.run( - [self.lldbDAPExec, "--launch-target", "INVALIDPROGRAM"], - capture_output=True, - universal_newlines=True, - ) - self.assertNotEqual(proc.returncode, 0) - self.assertIn( - '"--launch-target" requires "--comm-file" to be specified', proc.stderr - ) - - @skipIfWindows - @skipIf(oslist=["linux"], archs=no_match(["x86_64"])) - def test_FakeAttachedRunInTerminalLauncherWithInvalidProgram(self): - if not self.isTestSupported(): - return - comm_file = os.path.join(self.getBuildDir(), "comm-file") - os.mkfifo(comm_file) - - proc = subprocess.Popen( - [ - self.lldbDAPExec, - "--comm-file", - comm_file, - "--launch-target", - "INVALIDPROGRAM", - ], - universal_newlines=True, - stderr=subprocess.PIPE, - ) - - self.readPidMessage(comm_file) - self.sendDidAttachMessage(comm_file) - self.assertIn("No such file or directory", self.readErrorMessage(comm_file)) - - _, stderr = proc.communicate() - self.assertIn("No such file or directory", stderr) - - @skipIfWindows - @skipIf(oslist=["linux"], archs=no_match(["x86_64"])) - def test_FakeAttachedRunInTerminalLauncherWithValidProgram(self): - if not self.isTestSupported(): - return - comm_file = os.path.join(self.getBuildDir(), "comm-file") - os.mkfifo(comm_file) - - proc = subprocess.Popen( - [ - self.lldbDAPExec, - "--comm-file", - comm_file, - "--launch-target", - "echo", - "foo", - ], - universal_newlines=True, - stdout=subprocess.PIPE, - ) - - self.readPidMessage(comm_file) - self.sendDidAttachMessage(comm_file) - - stdout, _ = proc.communicate() - self.assertIn("foo", stdout) - - @skipIfWindows - @skipIf(oslist=["linux"], archs=no_match(["x86_64"])) - def test_FakeAttachedRunInTerminalLauncherAndCheckEnvironment(self): - if not self.isTestSupported(): - return - comm_file = os.path.join(self.getBuildDir(), "comm-file") - os.mkfifo(comm_file) - - proc = subprocess.Popen( - [self.lldbDAPExec, "--comm-file", comm_file, "--launch-target", "env"], - universal_newlines=True, - stdout=subprocess.PIPE, - env={**os.environ, "FOO": "BAR"}, - ) - - self.readPidMessage(comm_file) - self.sendDidAttachMessage(comm_file) - - stdout, _ = proc.communicate() - self.assertIn("FOO=BAR", stdout) - - @skipIfWindows - @skipIf(oslist=["linux"], archs=no_match(["x86_64"])) - def test_NonAttachedRunInTerminalLauncher(self): - if not self.isTestSupported(): - return - comm_file = os.path.join(self.getBuildDir(), "comm-file") - os.mkfifo(comm_file) - - proc = subprocess.Popen( - [ - self.lldbDAPExec, - "--comm-file", - comm_file, - "--launch-target", - "echo", - "foo", - ], - universal_newlines=True, - stderr=subprocess.PIPE, - env={**os.environ, "LLDB_DAP_RIT_TIMEOUT_IN_MS": "1000"}, - ) - - self.readPidMessage(comm_file) - - _, stderr = proc.communicate() - self.assertIn("Timed out trying to get messages from the debug adapter", stderr) diff --git a/lldb/tools/lldb-dap/CMakeLists.txt b/lldb/tools/lldb-dap/CMakeLists.txt index 40cba16c141f0..3e7e88afa9ced 100644 --- a/lldb/tools/lldb-dap/CMakeLists.txt +++ b/lldb/tools/lldb-dap/CMakeLists.txt @@ -14,7 +14,6 @@ add_lldb_library(lldbDAP DAPLog.cpp EventHelper.cpp ExceptionBreakpoint.cpp - FifoFiles.cpp FunctionBreakpoint.cpp InstructionBreakpoint.cpp JSONUtils.cpp @@ -22,7 +21,6 @@ add_lldb_library(lldbDAP OutputRedirector.cpp ProgressEvent.cpp ProtocolUtils.cpp - RunInTerminal.cpp SourceBreakpoint.cpp Transport.cpp Variables.cpp diff --git a/lldb/tools/lldb-dap/FifoFiles.cpp b/lldb/tools/lldb-dap/FifoFiles.cpp deleted file mode 100644 index 1f1bba80bd3b1..0000000000000 --- a/lldb/tools/lldb-dap/FifoFiles.cpp +++ /dev/null @@ -1,101 +0,0 @@ -//===-- FifoFiles.cpp -------------------------------------------*- C++ -*-===// -// -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -// -//===----------------------------------------------------------------------===// - -#include "FifoFiles.h" -#include "JSONUtils.h" - -#if !defined(_WIN32) -#include <sys/stat.h> -#include <sys/types.h> -#include <unistd.h> -#endif - -#include <chrono> -#include <fstream> -#include <future> -#include <optional> - -using namespace llvm; - -namespace lldb_dap { - -FifoFile::FifoFile(StringRef path) : m_path(path) {} - -FifoFile::~FifoFile() { -#if !defined(_WIN32) - unlink(m_path.c_str()); -#endif -} - -Expected<std::shared_ptr<FifoFile>> CreateFifoFile(StringRef path) { -#if defined(_WIN32) - return createStringError(inconvertibleErrorCode(), "Unimplemented"); -#else - if (int err = mkfifo(path.data(), 0600)) - return createStringError(std::error_code(err, std::generic_category()), - "Couldn't create fifo file: %s", path.data()); - return std::make_shared<FifoFile>(path); -#endif -} - -FifoFileIO::FifoFileIO(StringRef fifo_file, StringRef other_endpoint_name) - : m_fifo_file(fifo_file), m_other_endpoint_name(other_endpoint_name) {} - -Expected<json::Value> FifoFileIO::ReadJSON(std::chrono::milliseconds timeout) { - // We use a pointer for this future, because otherwise its normal destructor - // would wait for the getline to end, rendering the timeout useless. - std::optional<std::string> line; - std::future<void> *future = - new std::future<void>(std::async(std::launch::async, [&]() { - std::ifstream reader(m_fifo_file, std::ifstream::in); - std::string buffer; - std::getline(reader, buffer); - if (!buffer.empty()) - line = buffer; - })); - if (future->wait_for(timeout) == std::future_status::timeout || !line) - // Indeed this is a leak, but it's intentional. "future" obj destructor - // will block on waiting for the worker thread to join. And the worker - // thread might be stuck in blocking I/O. Intentionally leaking the obj - // as a hack to avoid blocking main thread, and adding annotation to - // supress static code inspection warnings - - // coverity[leaked_storage] - return createStringError(inconvertibleErrorCode(), - "Timed out trying to get messages from the " + - m_other_endpoint_name); - delete future; - return json::parse(*line); -} - -Error FifoFileIO::SendJSON(const json::Value &json, - std::chrono::milliseconds timeout) { - bool done = false; - std::future<void> *future = - new std::future<void>(std::async(std::launch::async, [&]() { - std::ofstream writer(m_fifo_file, std::ofstream::out); - writer << JSONToString(json) << std::endl; - done = true; - })); - if (future->wait_for(timeout) == std::future_status::timeout || !done) { - // Indeed this is a leak, but it's intentional. "future" obj destructor will - // block on waiting for the worker thread to join. And the worker thread - // might be stuck in blocking I/O. Intentionally leaking the obj as a hack - // to avoid blocking main thread, and adding annotation to supress static - // code inspection warnings" - - // coverity[leaked_storage] - return createStringError(inconvertibleErrorCode(), - "Timed out trying to send messages to the " + - m_other_endpoint_name); - } - delete future; - return Error::success(); -} - -} // namespace lldb_dap diff --git a/lldb/tools/lldb-dap/FifoFiles.h b/lldb/tools/lldb-dap/FifoFiles.h deleted file mode 100644 index 633ebeb2aedd4..0000000000000 --- a/lldb/tools/lldb-dap/FifoFiles.h +++ /dev/null @@ -1,85 +0,0 @@ -//===-- FifoFiles.h ---------------------------------------------*- C++ -*-===// -// -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -// -//===----------------------------------------------------------------------===// - -#ifndef LLDB_TOOLS_LLDB_DAP_FIFOFILES_H -#define LLDB_TOOLS_LLDB_DAP_FIFOFILES_H - -#include "llvm/Support/Error.h" -#include "llvm/Support/JSON.h" - -#include <chrono> - -namespace lldb_dap { - -/// Struct that controls the life of a fifo file in the filesystem. -/// -/// The file is destroyed when the destructor is invoked. -struct FifoFile { - FifoFile(llvm::StringRef path); - - ~FifoFile(); - - std::string m_path; -}; - -/// Create a fifo file in the filesystem. -/// -/// \param[in] path -/// The path for the fifo file. -/// -/// \return -/// A \a std::shared_ptr<FifoFile> if the file could be created, or an -/// \a llvm::Error in case of failures. -llvm::Expected<std::shared_ptr<FifoFile>> CreateFifoFile(llvm::StringRef path); - -class FifoFileIO { -public: - /// \param[in] fifo_file - /// The path to an input fifo file that exists in the file system. - /// - /// \param[in] other_endpoint_name - /// A human readable name for the other endpoint that will communicate - /// using this file. This is used for error messages. - FifoFileIO(llvm::StringRef fifo_file, llvm::StringRef other_endpoint_name); - - /// Read the next JSON object from the underlying input fifo file. - /// - /// The JSON object is expected to be a single line delimited with \a - /// std::endl. - /// - /// \return - /// An \a llvm::Error object indicating the success or failure of this - /// operation. Failures arise if the timeout is hit, the next line of text - /// from the fifo file is not a valid JSON object, or is it impossible to - /// read from the file. - llvm::Expected<llvm::json::Value> ReadJSON(std::chrono::milliseconds timeout); - - /// Serialize a JSON object and write it to the underlying output fifo file. - /// - /// \param[in] json - /// The JSON object to send. It will be printed as a single line delimited - /// with \a std::endl. - /// - /// \param[in] timeout - /// A timeout for how long we should until for the data to be consumed. - /// - /// \return - /// An \a llvm::Error object indicating whether the data was consumed by - /// a reader or not. - llvm::Error SendJSON( - const llvm::json::Value &json, - std::chrono::milliseconds timeout = std::chrono::milliseconds(20000)); - -private: - std::string m_fifo_file; - std::string m_other_endpoint_name; -}; - -} // namespace lldb_dap - -#endif // LLDB_TOOLS_LLDB_DAP_FIFOFILES_H diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp index 93bc80a38e29d..cfcbabbb16a93 100644 --- a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp +++ b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp @@ -14,10 +14,12 @@ #include "LLDBUtils.h" #include "Protocol/ProtocolBase.h" #include "Protocol/ProtocolRequests.h" -#include "RunInTerminal.h" #include "lldb/API/SBDefines.h" #include "lldb/API/SBEnvironment.h" #include "llvm/Support/Error.h" +#include <cerrno> +#include <csignal> +#include <limits> #include <mutex> #if !defined(_WIN32) @@ -51,6 +53,70 @@ static uint32_t SetLaunchFlag(uint32_t flags, bool flag, return flags; } +// Assume single thread +class PidReceiver { +private: + inline static volatile std::sig_atomic_t pid; + sigset_t oldmask; + struct sigaction oldaction; + + static_assert(std::numeric_limits<volatile sig_atomic_t>::max() >= + std::numeric_limits<pid_t>::max(), + "sig_atomic_t must be able to hold a pid_t value"); + + static void sig_handler(int, siginfo_t *info, void *) { + // Store the PID from the signal info + pid = info->si_pid; + } + + PidReceiver(const PidReceiver &) = delete; + PidReceiver &operator=(const PidReceiver &) = delete; + PidReceiver(PidReceiver &&) = delete; + +public: + PidReceiver() { + pid = LLDB_INVALID_PROCESS_ID; + // sigprocmask and sigaction can only fail by programmer error + // Defer SIGUSR1, otherwise we might receive it out of pselect and hang + sigset_t mask; + sigemptyset(&mask); + sigaddset(&mask, SIGUSR1); + assert(!sigprocmask(SIG_BLOCK, &mask, &oldmask)); + + // Set up sigaction for SIGUSR1 with SA_SIGINFO + struct sigaction sa; + std::memset(&sa, 0, sizeof(sa)); + sa.sa_sigaction = sig_handler; + sa.sa_flags = SA_SIGINFO; + sigemptyset(&sa.sa_mask); + assert(!sigaction(SIGUSR1, &sa, &oldaction)); + } + + ~PidReceiver() { + // Restore the old signal mask + assert(!sigprocmask(SIG_SETMASK, &oldmask, nullptr)); + assert(!sigaction(SIGUSR1, &oldaction, nullptr)); + } + + llvm::Expected<lldb::pid_t> WaitForPid() { + // Wait for the signal to be received + while (pid == LLDB_INVALID_PROCESS_ID) { + struct timespec timeout; + timeout.tv_sec = 5; + timeout.tv_nsec = 0; + auto ret = pselect(0, nullptr, nullptr, nullptr, &timeout, &oldmask); + if (ret == -1 && errno != EINTR) { + return llvm::createStringError( + std::error_code(errno, std::generic_category()), "pselect failed"); + } else if (ret == 0) { + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "Timed out waiting for signal"); + } + } + return pid; + } +}; + static llvm::Error RunInTerminal(DAP &dap, const protocol::LaunchRequestArguments &arguments) { if (!dap.clientFeatures.contains( @@ -65,26 +131,19 @@ RunInTerminal(DAP &dap, const protocol::LaunchRequestArguments &arguments) { dap.is_attach = true; lldb::SBAttachInfo attach_info; - llvm::Expected<std::shared_ptr<FifoFile>> comm_file_or_err = - CreateRunInTerminalCommFile(); - if (!comm_file_or_err) - return comm_file_or_err.takeError(); - FifoFile &comm_file = *comm_file_or_err.get(); - - RunInTerminalDebugAdapterCommChannel comm_channel(comm_file.m_path); - lldb::pid_t debugger_pid = LLDB_INVALID_PROCESS_ID; #if !defined(_WIN32) debugger_pid = getpid(); #endif + auto pid_receiver = PidReceiver(); llvm::json::Object reverse_request = CreateRunInTerminalReverseRequest( arguments.configuration.program, arguments.args, arguments.env, - arguments.cwd, comm_file.m_path, debugger_pid); + arguments.cwd, debugger_pid); dap.SendReverseRequest<LogFailureResponseHandler>("runInTerminal", std::move(reverse_request)); - if (llvm::Expected<lldb::pid_t> pid = comm_channel.GetLauncherPid()) + if (llvm::Expected<lldb::pid_t> pid = pid_receiver.WaitForPid()) attach_info.SetProcessID(*pid); else return pid.takeError(); @@ -95,13 +154,7 @@ RunInTerminal(DAP &dap, const protocol::LaunchRequestArguments &arguments) { if (error.Fail()) return llvm::createStringError(llvm::inconvertibleErrorCode(), - "Failed to attach to the target process. %s", - comm_channel.GetLauncherError().c_str()); - // This will notify the runInTerminal launcher that we attached. - // We have to make this async, as the function won't return until the launcher - // resumes and reads the data. - std::future<lldb::SBError> did_attach_message_success = - comm_channel.NotifyDidAttach(); + "Failed to attach to the target process."); // We just attached to the runInTerminal launcher, which was waiting to be // attached. We now resume it, so it can receive the didAttach notification @@ -115,15 +168,7 @@ RunInTerminal(DAP &dap, const protocol::LaunchRequestArguments &arguments) { // we return the debugger to its sync state. scope_sync_mode.reset(); - // If sending the notification failed, the launcher should be dead by now and - // the async didAttach notification should have an error message, so we - // return it. Otherwise, everything was a success. - did_attach_message_success.wait(); - error = did_attach_message_success.get(); - if (error.Success()) - return llvm::Error::success(); - return llvm::createStringError(llvm::inconvertibleErrorCode(), - error.GetCString()); + return llvm::Error::success(); } void BaseRequestHandler::Run(const Request &request) { diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp index 573f3eba00f62..bf10742e18bb8 100644 --- a/lldb/tools/lldb-dap/JSONUtils.cpp +++ b/lldb/tools/lldb-dap/JSONUtils.cpp @@ -1238,15 +1238,14 @@ llvm::json::Value CreateCompileUnit(lldb::SBCompileUnit &unit) { llvm::json::Object CreateRunInTerminalReverseRequest( llvm::StringRef program, const std::vector<std::string> &args, const llvm::StringMap<std::string> &env, llvm::StringRef cwd, - llvm::StringRef comm_file, lldb::pid_t debugger_pid) { + lldb::pid_t debugger_pid) { llvm::json::Object run_in_terminal_args; // This indicates the IDE to open an embedded terminal, instead of opening // the terminal in a new window. run_in_terminal_args.try_emplace("kind", "integrated"); // The program path must be the first entry in the "args" field - std::vector<std::string> req_args = {DAP::debug_adapter_path.str(), - "--comm-file", comm_file.str()}; + std::vector<std::string> req_args = {DAP::debug_adapter_path.str()}; if (debugger_pid != LLDB_INVALID_PROCESS_ID) { req_args.push_back("--debugger-pid"); req_args.push_back(std::to_string(debugger_pid)); diff --git a/lldb/tools/lldb-dap/JSONUtils.h b/lldb/tools/lldb-dap/JSONUtils.h index 08699a94bbd87..4ed66e6992be2 100644 --- a/lldb/tools/lldb-dap/JSONUtils.h +++ b/lldb/tools/lldb-dap/JSONUtils.h @@ -466,9 +466,6 @@ llvm::json::Value CreateCompileUnit(lldb::SBCompileUnit &unit); /// \param[in] cwd /// The working directory for the run in terminal request. /// -/// \param[in] comm_file -/// The fifo file used to communicate the with the target launcher. -/// /// \param[in] debugger_pid /// The PID of the lldb-dap instance that will attach to the target. The /// launcher uses it on Linux tell the kernel that it should allow the @@ -480,7 +477,7 @@ llvm::json::Value CreateCompileUnit(lldb::SBCompileUnit &unit); llvm::json::Object CreateRunInTerminalReverseRequest( llvm::StringRef program, const std::vector<std::string> &args, const llvm::StringMap<std::string> &env, llvm::StringRef cwd, - llvm::StringRef comm_file, lldb::pid_t debugger_pid); + lldb::pid_t debugger_pid); /// Create a "Terminated" JSON object that contains statistics /// diff --git a/lldb/tools/lldb-dap/Options.td b/lldb/tools/lldb-dap/Options.td index aecf91797ac70..669d6a7c0df05 100644 --- a/lldb/tools/lldb-dap/Options.td +++ b/lldb/tools/lldb-dap/Options.td @@ -31,16 +31,11 @@ def connection "Connections are specified like 'tcp://[host]:port' or " "'unix:///path'.">; -def launch_target: S<"laun... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/142374 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
