[Lldb-commits] [lldb] [lldb-dap] Reimplement `runInTerminal` with signals (PR #142374)
via lldb-commits
lldb-commits at lists.llvm.org
Mon Jun 2 05:30:15 PDT 2025
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
More information about the lldb-commits
mailing list