[lldb-dap] Ensure the IO forwarding threads are managed by the DAP object lifecycle. #120457
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesThis moves the ownership of the threads that forward stdout/stderr to the DAP object itself to ensure that the threads are joined and that the forwarding is cleaned up when the DAP connection is disconnected. This is part of a larger refactor to allow lldb-dap to run in a listening mode and accept multiple connections. Patch is 24.00 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/120457.diff 8 Files Affected:
diff --git a/lldb/tools/lldb-dap/CMakeLists.txt b/lldb/tools/lldb-dap/CMakeLists.txt
index d68098bf7b3266..00906e8ac10904 100644
--- a/lldb/tools/lldb-dap/CMakeLists.txt
+++ b/lldb/tools/lldb-dap/CMakeLists.txt
@@ -1,7 +1,3 @@
-if ( CMAKE_SYSTEM_NAME MATCHES "Windows" OR CMAKE_SYSTEM_NAME MATCHES "NetBSD" )
- list(APPEND extra_libs lldbHost)
-endif ()
-
if (HAVE_LIBPTHREAD)
list(APPEND extra_libs pthread)
endif ()
@@ -32,7 +28,6 @@ add_lldb_tool(lldb-dap
IOStream.cpp
JSONUtils.cpp
LLDBUtils.cpp
- OutputRedirector.cpp
ProgressEvent.cpp
RunInTerminal.cpp
SourceBreakpoint.cpp
@@ -42,6 +37,7 @@ add_lldb_tool(lldb-dap
LINK_LIBS
liblldb
+ lldbHost
${extra_libs}
LINK_COMPONENTS
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 35250d9eef608a..4e0de6fa3a4e90 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -6,34 +6,53 @@
//
//===----------------------------------------------------------------------===//
-#include <chrono>
-#include <cstdarg>
-#include <fstream>
-#include <mutex>
-
#include "DAP.h"
#include "JSONUtils.h"
#include "LLDBUtils.h"
+#include "lldb/API/SBBreakpoint.h"
#include "lldb/API/SBCommandInterpreter.h"
#include "lldb/API/SBLanguageRuntime.h"
#include "lldb/API/SBListener.h"
#include "lldb/API/SBStream.h"
+#include "lldb/Host/FileSystem.h"
+#include "lldb/API/SBCommandReturnObject.h"
+#include "lldb/API/SBProcess.h"
+#include "lldb/Utility/Status.h"
+#include "lldb/lldb-defines.h"
+#include "lldb/lldb-enumerations.h"
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/Twine.h"
#include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/Error.h"
#include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/raw_ostream.h"
+#include <algorithm>
+#include <cassert>
+#include <chrono>
+#include <cstdarg>
+#include <cstdio>
+#include <fstream>
+#include <mutex>
+#include <utility>
#if defined(_WIN32)
#define NOMINMAX
#include <fcntl.h>
#include <io.h>
#include <windows.h>
+#else
+#include <unistd.h>
#endif
using namespace lldb_dap;
namespace lldb_dap {
-DAP::DAP(llvm::StringRef path, ReplMode repl_mode)
- : debug_adaptor_path(path), broadcaster("lldb-dap"),
+DAP::DAP(llvm::StringRef path, std::optional<std::ofstream> &log,
+ ReplMode repl_mode, StreamDescriptor input, StreamDescriptor output)
+ : debug_adaptor_path(path), log(log), input(std::move(input)),
+ output(std::move(output)), broadcaster("lldb-dap"),
exception_breakpoints(), focus_tid(LLDB_INVALID_THREAD_ID),
stop_at_entry(false), is_attach(false),
enable_auto_variable_summaries(false),
@@ -43,21 +62,7 @@ DAP::DAP(llvm::StringRef path, ReplMode repl_mode)
configuration_done_sent(false), waiting_for_run_in_terminal(false),
progress_event_reporter(
[&](const ProgressEvent &event) { SendJSON(event.ToJSON()); }),
- reverse_request_seq(0), repl_mode(repl_mode) {
- const char *log_file_path = getenv("LLDBDAP_LOG");
-#if defined(_WIN32)
- // Windows opens stdout and stdin in text mode which converts \n to 13,10
- // while the value is just 10 on Darwin/Linux. Setting the file mode to binary
- // fixes this.
- int result = _setmode(fileno(stdout), _O_BINARY);
- assert(result);
- result = _setmode(fileno(stdin), _O_BINARY);
- UNUSED_IF_ASSERT_DISABLED(result);
- assert(result);
-#endif
- if (log_file_path)
- log.reset(new std::ofstream(log_file_path));
-}
+ reverse_request_seq(0), repl_mode(repl_mode) {}
DAP::~DAP() = default;
@@ -173,6 +178,63 @@ ExceptionBreakpoint *DAP::GetExceptionBreakpoint(const lldb::break_id_t bp_id) {
return nullptr;
}
+llvm::Error DAP::ConfigureIO(std::optional<std::FILE *> overrideOut,
+ std::optional<std::FILE *> overrideErr) {
+ auto *inull = lldb_private::FileSystem::Instance().Fopen(
+ lldb_private::FileSystem::DEV_NULL, "w");
+ in = lldb::SBFile(inull, true);
+
+ lldb_private::Status status;
+ status = pout.CreateNew(/*child_process_inherit=*/false);
+ if (status.Fail())
+ return status.takeError();
+ status = perr.CreateNew(/*child_process_inherit=*/false);
+ if (status.Fail())
+ return status.takeError();
+
+ if (overrideOut) {
+ if (dup2(pout.GetWriteFileDescriptor(), fileno(*overrideOut)) == -1) {
+ return llvm::make_error<llvm::StringError>(
+ llvm::errnoAsErrorCode(),
+ llvm::formatv("override fd=%d failed", fileno(*overrideOut))
+ .str()
+ .c_str());
+ }
+ }
+
+ if (overrideErr) {
+ if (dup2(perr.GetWriteFileDescriptor(), fileno(*overrideErr)) == -1) {
+ return llvm::make_error<llvm::StringError>(
+ llvm::errnoAsErrorCode(),
+ llvm::formatv("override fd=%d failed", fileno(*overrideErr))
+ .str()
+ .c_str());
+ }
+ }
+
+ auto forwarder = [&](lldb_private::Pipe &pipe, OutputType outputType) {
+ char buffer[4098];
+ size_t bytes_read;
+ while (pipe.CanRead()) {
+ lldb_private::Status error = pipe.ReadWithTimeout(
+ &buffer, sizeof(buffer), std::chrono::seconds(1), bytes_read);
+ if (error.Success()) {
+ // zero bytes returned on EOF.
+ if (bytes_read == 0)
+ break;
+ SendOutput(outputType, llvm::StringRef(buffer, bytes_read));
+ }
+ }
+ };
+
+ stdout_forward_thread =
+ std::thread(forwarder, std::ref(pout), OutputType::Stdout);
+ stderr_forward_thread =
+ std::thread(forwarder, std::ref(perr), OutputType::Stderr);
+
+ return llvm::Error::success();
+}
+
// Send the JSON in "json_str" to the "out" stream. Correctly send the
// "Content-Length:" field followed by the length, followed by the raw
// JSON bytes.
@@ -208,19 +270,19 @@ std::string DAP::ReadJSON() {
std::string json_str;
int length;
- if (!input.read_expected(log.get(), "Content-Length: "))
+ if (!input.read_expected(log, "Content-Length: "))
return json_str;
- if (!input.read_line(log.get(), length_str))
+ if (!input.read_line(log, length_str))
return json_str;
if (!llvm::to_integer(length_str, length))
return json_str;
- if (!input.read_expected(log.get(), "\r\n"))
+ if (!input.read_expected(log, "\r\n"))
return json_str;
- if (!input.read_full(log.get(), length, json_str))
+ if (!input.read_full(log, length, json_str))
return json_str;
if (log) {
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index ae496236f13369..f9cedd32cd0bd3 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -9,36 +9,38 @@
#ifndef LLDB_TOOLS_LLDB_DAP_DAP_H
#define LLDB_TOOLS_LLDB_DAP_DAP_H
-#include <cstdio>
-#include <iosfwd>
-#include <map>
-#include <optional>
-#include <thread>
-
-#include "llvm/ADT/DenseMap.h"
-#include "llvm/ADT/DenseSet.h"
-#include "llvm/ADT/StringMap.h"
-#include "llvm/ADT/StringRef.h"
-#include "llvm/Support/JSON.h"
-#include "llvm/Support/Threading.h"
-#include "llvm/Support/raw_ostream.h"
-
-#include "lldb/API/SBAttachInfo.h"
-#include "lldb/API/SBCommandInterpreter.h"
-#include "lldb/API/SBCommandReturnObject.h"
-#include "lldb/API/SBDebugger.h"
-#include "lldb/API/SBEvent.h"
-#include "lldb/API/SBFormat.h"
-#include "lldb/API/SBLaunchInfo.h"
-#include "lldb/API/SBTarget.h"
-#include "lldb/API/SBThread.h"
-
+#include "DAPForward.h"
#include "ExceptionBreakpoint.h"
#include "FunctionBreakpoint.h"
#include "IOStream.h"
#include "InstructionBreakpoint.h"
#include "ProgressEvent.h"
#include "SourceBreakpoint.h"
+#include "lldb/API/SBBroadcaster.h"
+#include "lldb/API/SBCommandInterpreter.h"
+#include "lldb/API/SBDebugger.h"
+#include "lldb/API/SBError.h"
+#include "lldb/API/SBFile.h"
+#include "lldb/API/SBFormat.h"
+#include "lldb/API/SBFrame.h"
+#include "lldb/API/SBTarget.h"
+#include "lldb/API/SBThread.h"
+#include "lldb/API/SBValue.h"
+#include "lldb/API/SBValueList.h"
+#include "lldb/Host/Pipe.h"
+#include "lldb/lldb-types.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/JSON.h"
+#include "llvm/Support/Threading.h"
+#include <map>
+#include <mutex>
+#include <optional>
+#include <thread>
+#include <vector>
#define VARREF_LOCALS (int64_t)1
#define VARREF_GLOBALS (int64_t)2
@@ -138,15 +140,20 @@ struct SendEventRequestHandler : public lldb::SBCommandPluginInterface {
struct DAP {
llvm::StringRef debug_adaptor_path;
+ std::optional<std::ofstream> &log;
InputStream input;
OutputStream output;
+ lldb::SBFile in;
+ lldb_private::Pipe pout;
+ lldb_private::Pipe perr;
lldb::SBDebugger debugger;
lldb::SBTarget target;
Variables variables;
lldb::SBBroadcaster broadcaster;
std::thread event_thread;
std::thread progress_event_thread;
- std::unique_ptr<std::ofstream> log;
+ std::thread stdout_forward_thread;
+ std::thread stderr_forward_thread;
llvm::StringMap<SourceBreakpointMap> source_breakpoints;
FunctionBreakpointMap function_breakpoints;
InstructionBreakpointMap instruction_breakpoints;
@@ -198,13 +205,21 @@ struct DAP {
// will contain that expression.
std::string last_nonempty_var_expression;
- DAP(llvm::StringRef path, ReplMode repl_mode);
+ DAP(llvm::StringRef path, std::optional<std::ofstream> &log,
+ ReplMode repl_mode, StreamDescriptor input, StreamDescriptor output);
~DAP();
DAP(const DAP &rhs) = delete;
void operator=(const DAP &rhs) = delete;
ExceptionBreakpoint *GetExceptionBreakpoint(const std::string &filter);
ExceptionBreakpoint *GetExceptionBreakpoint(const lldb::break_id_t bp_id);
+ /// Redirect stdout and stderr fo the IDE's console output.
+ ///
+ /// Errors in this operation will be printed to the log file and the IDE's
+ /// console output as well.
+ llvm::Error ConfigureIO(std::optional<std::FILE *> overrideOut,
+ std::optional<std::FILE *> overrideErr);
+
// Serialize the JSON value into a string and send the JSON packet to
// the "out" stream.
void SendJSON(const llvm::json::Value &json);
diff --git a/lldb/tools/lldb-dap/IOStream.cpp b/lldb/tools/lldb-dap/IOStream.cpp
index d2e8ec40b0a7b8..a078104595037d 100644
--- a/lldb/tools/lldb-dap/IOStream.cpp
+++ b/lldb/tools/lldb-dap/IOStream.cpp
@@ -87,7 +87,7 @@ bool OutputStream::write_full(llvm::StringRef str) {
return true;
}
-bool InputStream::read_full(std::ofstream *log, size_t length,
+bool InputStream::read_full(std::optional<std::ofstream> &log, size_t length,
std::string &text) {
std::string data;
data.resize(length);
@@ -131,7 +131,8 @@ bool InputStream::read_full(std::ofstream *log, size_t length,
return true;
}
-bool InputStream::read_line(std::ofstream *log, std::string &line) {
+bool InputStream::read_line(std::optional<std::ofstream> &log,
+ std::string &line) {
line.clear();
while (true) {
if (!read_full(log, 1, line))
@@ -144,7 +145,8 @@ bool InputStream::read_line(std::ofstream *log, std::string &line) {
return true;
}
-bool InputStream::read_expected(std::ofstream *log, llvm::StringRef expected) {
+bool InputStream::read_expected(std::optional<std::ofstream> &log,
+ llvm::StringRef expected) {
std::string result;
if (!read_full(log, expected.size(), result))
return false;
diff --git a/lldb/tools/lldb-dap/IOStream.h b/lldb/tools/lldb-dap/IOStream.h
index 57d5fd458b7165..f4829473b7b0fb 100644
--- a/lldb/tools/lldb-dap/IOStream.h
+++ b/lldb/tools/lldb-dap/IOStream.h
@@ -52,16 +52,24 @@ struct StreamDescriptor {
struct InputStream {
StreamDescriptor descriptor;
- bool read_full(std::ofstream *log, size_t length, std::string &text);
+ explicit InputStream(StreamDescriptor descriptor)
+ : descriptor(std::move(descriptor)) {};
- bool read_line(std::ofstream *log, std::string &line);
+ bool read_full(std::optional<std::ofstream> &log, size_t length,
+ std::string &text);
- bool read_expected(std::ofstream *log, llvm::StringRef expected);
+ bool read_line(std::optional<std::ofstream> &log, std::string &line);
+
+ bool read_expected(std::optional<std::ofstream> &log,
+ llvm::StringRef expected);
};
struct OutputStream {
StreamDescriptor descriptor;
+ explicit OutputStream(StreamDescriptor descriptor)
+ : descriptor(std::move(descriptor)) {};
+
bool write_full(llvm::StringRef str);
};
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/OutputRedirector.cpp b/lldb/tools/lldb-dap/OutputRedirector.cpp
deleted file mode 100644
index 2c2f49569869b4..00000000000000
--- a/lldb/tools/lldb-dap/OutputRedirector.cpp
+++ /dev/null
@@ -1,63 +0,0 @@
-//===-- OutputRedirector.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
-//
-//===----------------------------------------------------------------------===/
-
-#if defined(_WIN32)
-#include <fcntl.h>
-#include <io.h>
-#else
-#include <unistd.h>
-#endif
-
-#include "DAP.h"
-#include "OutputRedirector.h"
-#include "llvm/ADT/StringRef.h"
-
-using namespace llvm;
-
-namespace lldb_dap {
-
-Error RedirectFd(int fd, std::function<void(llvm::StringRef)> callback) {
- int new_fd[2];
-#if defined(_WIN32)
- if (_pipe(new_fd, 4096, O_TEXT) == -1) {
-#else
- if (pipe(new_fd) == -1) {
-#endif
- int error = errno;
- return createStringError(inconvertibleErrorCode(),
- "Couldn't create new pipe for fd %d. %s", fd,
- strerror(error));
- }
-
- if (dup2(new_fd[1], fd) == -1) {
- int error = errno;
- return createStringError(inconvertibleErrorCode(),
- "Couldn't override the fd %d. %s", fd,
- strerror(error));
- }
-
- int read_fd = new_fd[0];
- std::thread t([read_fd, callback]() {
- char buffer[OutputBufferSize];
- while (true) {
- ssize_t bytes_count = read(read_fd, &buffer, sizeof(buffer));
- if (bytes_count == 0)
- return;
- if (bytes_count == -1) {
- if (errno == EAGAIN || errno == EINTR)
- continue;
- break;
- }
- callback(StringRef(buffer, bytes_count));
- }
- });
- t.detach();
- return Error::success();
-}
-
-} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/OutputRedirector.h b/lldb/tools/lldb-dap/OutputRedirector.h
deleted file mode 100644
index e26d1648b104f9..00000000000000
--- a/lldb/tools/lldb-dap/OutputRedirector.h
+++ /dev/null
@@ -1,26 +0,0 @@
-//===-- OutputRedirector.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_OUTPUT_REDIRECTOR_H
-#define LLDB_TOOLS_LLDB_DAP_OUTPUT_REDIRECTOR_H
-
-#include "llvm/ADT/StringRef.h"
-#include "llvm/Support/Error.h"
-
-namespace lldb_dap {
-
-/// Redirects the output of a given file descriptor to a callback.
-///
-/// \return
-/// \a Error::success if the redirection was set up correctly, or an error
-/// otherwise.
-llvm::Error RedirectFd(int fd, std::function<void(llvm::StringRef)> callback);
-
-} // namespace lldb_dap
-
-#endif // LLDB_TOOLS_LLDB_DAP_OUTPUT_REDIRECTOR_H
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index 3bfc578806021e..0d44530d48f92a 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -10,7 +10,6 @@
#include "FifoFiles.h"
#include "JSONUtils.h"
#include "LLDBUtils.h"
-#include "OutputRedirector.h"
#include "RunInTerminal.h"
#include "Watchpoint.h"
#include "lldb/API/SBDeclaration.h"
@@ -18,6 +17,7 @@
#include "lldb/API/SBListener.h"
#include "lldb/API/SBMemoryRegionInfo.h"
#include "lldb/API/SBStream.h"
+#include "lldb/API/SBEvent.h"
#include "lldb/API/SBStringList.h"
#include "lldb/Host/Config.h"
#include "llvm/ADT/ArrayRef.h"
@@ -137,15 +137,14 @@ lldb::SBValueList *GetTopLevelScope(DAP &dap, int64_t variablesReference) {
}
}
-SOCKET AcceptConnection(DAP &dap, int portno) {
+SOCKET AcceptConnection(std::optional<std::ofstream> &log, int portno) {
// Accept a socket connection from any host on "portno".
SOCKET newsockfd = -1;
struct sockaddr_in serv_addr, cli_addr;
SOCKET sockfd = socket(AF_INET, SOCK_STREAM, 0);
if (sockfd < 0) {
- if (dap.log)
- *dap.log << "error: opening socket (" << strerror(errno) << ")"
- << std::endl;
+ if (log)
+ *log << "error: opening socket (" << strerror(errno) << ")" << std::endl;
} else {
memset((char *)&serv_addr, 0, sizeof(serv_addr));
serv_addr.sin_family = AF_INET;
@@ -153,9 +152,9 @@ SOCKET AcceptConnection(DAP &dap, int portno) {
serv_addr.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
serv_addr.sin_port = htons(portno);
if (bind(sockfd, (struct sockaddr *)&serv_addr, sizeof(serv_addr)) < 0) {
- if (dap.log)
- *dap.log << "error: binding socket (" << strerror(errno) << ")"
- << std::endl;
+ if (log)
+ *log << "error: binding socket (" << strerror(errno) << ")"
+ << std::endl;
} else {
listen(sockfd, 5);
socklen_t clilen = sizeof(cli_addr);
@@ -163,8 +162,8 @@ SOCKET AcceptConnection(DAP &dap, int portno) {
llvm::sys::RetryAfterSignal(static_cast<SOCKET>(-1), accept, sockfd,
(struct sockaddr *)&cli_addr, &clilen);
if (newsockfd < 0)
- if (dap.log)
- *dap.log << "error: accept (" << strerror(errno) << ")" << std::endl;
+ if (log)
+ *log << "error: accept (" << strerror(errno) << ")" << std::endl;
}
#if defined(_WIN32)
closesocket(sockfd);
@@ -1099,6 +1098,14 @@ void request_disconnect(DAP &dap, const llvm::json::Object &request) {
dap.broadcaster.BroadcastEventByType(eBroadcastBitStopProgressThread);
dap.progress_event_thread.join();
}
+ if (dap.stdout_forward_thread.joinable()) {
+ dap.pout.Close();
+ dap.stdout_forward_thread.join();
+ }
+ if (dap.stderr_forward_thread.joinable()) {
+ dap.perr.Close();
+ dap.stderr_forward_thread.join();
+ }
dap.disconnecting = true;
}
@@ -1868,7 +1875,22 @@ void request_initialize(DAP &dap, const llvm::json::Object &request) {
// which may affect the outcome of tests.
bool source_init_file = GetBoolean(arguments, "sourceInitFile", true);
- dap.debugger = lldb::SBDebugger::Create(source_init_file);
+ // Do not source init files until in/out/err are configured.
+ dap.debugger = lldb::SBDebugger::Create(false);
+ dap.debugger.SetInputFile(dap.in);
+ dap.debugger.SetOutputFile(lldb::SBFile(dap.pout.GetWriteFileDescriptor(), "w", false));
+ dap.debugger.SetErrorFile(lldb::SBFile(dap.perr.GetWriteFileDescriptor(), "w", false));
+
+ auto interp = dap.debugger.GetCommandInterpreter();
+
+ if (source_init_file) {
+ dap.debugger.SkipLLDBInitFiles(false);
+ dap.debugger.SkipAppInitFiles(false);
+ lldb::SBCommandReturnObject init;
+ interp.SourceInitFileInGlobalDirectory(init);
+ interp.SourceInitFileInHomeDirectory(init);
+ }
+
if (llvm::Error err = dap.RunPreInitCommands()) {
response["success"] = false;
EmplaceSafeString(response, "message", llvm::toString(std::move(err)));
@@ -4907,38 +4929,6 @@ static void redirection_test() {
fflush(stderr);
}
-/// Redirect stdout and stderr fo the IDE's console output.
-///
-/// Errors in this operation will be printed to the log file and the IDE's
-/// console output as well.
-///
-/// \return
-/// A fd pointing to the original stdout.
-static int SetupStdoutStderrRedirection(DAP &dap) {
- int stdoutfd = fileno(stdout);
- int new_stdout_fd = dup(stdoutfd);
- auto output_callback_stderr = [&dap](llvm::StringRef data) {
- dap.SendOutput(OutputType::Stderr, data);
- };
- auto output_callback_stdout = [&dap](llvm::StringRef data) {...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
labath
reviewed
Dec 20, 2024
lldb/tools/lldb-dap/IOStream.h
Outdated
|
|
||
| bool read_line(std::ofstream *log, std::string &line); | ||
| bool read_full(std::optional<std::ofstream> &log, size_t length, |
There was a problem hiding this comment.
The same goes for all of these, I don't think reference-to-optional improves things here.
There was a problem hiding this comment.
Reverted these changes.
lldb/tools/lldb-dap/lldb-dap.cpp
Outdated
Comment on lines
5056
to
5058
| #endif | ||
|
|
||
| int stdout_fd = dup(fileno(stdout)); |
There was a problem hiding this comment.
Suggested change
| #endif | |
| int stdout_fd = dup(fileno(stdout)); | |
| int stdout_fd = dup(fileno(stdout)); | |
| #else | |
| int stdout_fd = fcntl(fileno(stdout), F_DUPFD_CLOEXEC, 3); | |
| #endif | |
I know this isn't your code, but I think it's a good idea to set this flag, particularly as we're going to be doing (a lot) more things within the same process.
There was a problem hiding this comment.
I created a help for duplicating the FD and added a #ifdef around this like I found in a few other places in lldb like
llvm-project/lldb/source/Host/posix/PipePosix.cpp
Lines 43 to 50 in 81831ef
There was a problem hiding this comment.
I would work (since the code is still single-threaded at this point), but I don't think it's better than F_DUPFD_CLOEXEC. The reason the code you quote is written this way is because non-linux systems don't have the equivalent of the pipe2. I've checked that macos and all supported BSD variants do have this option. The only one I couldn't check is AIX, but it's still too early to call that "supported".
…ject lifecycle. This moves the ownership of the threads that forward stdout/stderr to the DAP object itself to ensure that the threads are joined and that the forwarding is cleaned up when the DAP connection is disconnected. This is part of a larger refactor to allow lldb-dap to run in a listening mode and accept multiple connections.
…p file and making it into an object instead. This removes some of the duplicate code that was happening between stdout/stderr redirection.
ashgti
force-pushed
the
lldb-dap-redirect-io
branch
from
e81fd17 to
ce969ff
Compare
labath
reviewed
Dec 23, 2024
Comment on lines
45
to
52
| while (m_pipe.CanRead() && !m_stopped) { | ||
| size_t bytes_read; | ||
| Status status = m_pipe.Read(&buffer, sizeof(buffer), bytes_read); | ||
| if (status.Fail()) | ||
| continue; | ||
|
|
||
| // EOF detected | ||
| if (bytes_read == 0) |
There was a problem hiding this comment.
Suggested change
| while (m_pipe.CanRead() && !m_stopped) { | |
| size_t bytes_read; | |
| Status status = m_pipe.Read(&buffer, sizeof(buffer), bytes_read); | |
| if (status.Fail()) | |
| continue; | |
| // EOF detected | |
| if (bytes_read == 0) | |
| while (m_pipe.CanRead()) { | |
| size_t bytes_read; | |
| Status status = m_pipe.Read(&buffer, sizeof(buffer), bytes_read); | |
| if (status.Fail()) | |
| continue; | |
| // EOF detected | |
| if (bytes_read == 0 || m_stopped) |
.. motivation being to avoid sending the empty null byte. It's true that this can prevent us from sending other data as well, but I don't think anything can rely on that, as the thread can still exit before it manages to read that data from the pipe. If anything wanted to rely on some output being sent, it would have to implement to form of out-of-band synchronization to make sure the data is received before shutting things down.
lldb/tools/lldb-dap/lldb-dap.cpp
Outdated
Comment on lines
5056
to
5058
| #endif | ||
|
|
||
| int stdout_fd = dup(fileno(stdout)); |
There was a problem hiding this comment.
I would work (since the code is still single-threaded at this point), but I don't think it's better than F_DUPFD_CLOEXEC. The reason the code you quote is written this way is because non-linux systems don't have the equivalent of the pipe2. I've checked that macos and all supported BSD variants do have this option. The only one I couldn't check is AIX, but it's still too early to call that "supported".
|
I'm running into an issue on linux with this change that is resulting in 2 instances of the I think I've ended up with a second instance of FileSystem.h by referencing it in lldb-dap. I don't see errors like this on macOS, but that may be due to linker differences. I was using the I know in my other pull request we talked about linking against lldb/host for some utilities like the use of Pipe, but is this an issue with the current architecture? Should I avoid that Host/FileSystem API or will I run into this with other APIs potentially? |
|
I have the linux tests working by avoiding the |
labath
approved these changes
Dec 26, 2024
There was a problem hiding this comment.
I know in my other pull request we talked about linking against lldb/host for some utilities like the use of Pipe, but is this an issue with the current architecture? Should I avoid that Host/FileSystem API or will I run into this with other APIs potentially?
You're right. This is potential problem, one whose scope I did not fully realize when I made that suggestion. It happens because liblldb only exposes the public lldb API, which means that anything linking against it gets its own copy it. This can be a problem with classes that use singletons or some for of explicit initialization, etc.
Nonetheless, I still believe reimplementing any class that we might want to use on the other side of the API is not a solution. Overall, I think you've managed to strike a good balance with this version of the patch. The filesystem class is not particularly important here as the only thing we really get from it is the (os-specific) name of /dev/null. The socket and pipe classes on the other hand, are a lot more complicated, which makes it more worthwhile to reuse them. Fortunately, they are also the ones that don't suffer from these kinds of problems.
If you run into other problems with some changes like this, let me know, and I'll see what I can do about it.
Co-authored-by: Pavel Labath <[email protected]>
|
@walter-erquinigo Wanted to double check with you on this since this change now will always link |
|
@ashgti , this looks very nice! It's good overall that we are able to reuse some components and reduce the number of responsibilities in lldb-dap. |
walter-erquinigo
approved these changes
Jan 6, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/5258 Here is the relevant piece of the build log for the reference |
Hi. |
|
Reverted this in 81898ac. Apart from breaking windows, I'm also seeing some weirdness when debugging iOS apps with this change, don't know what's going on. |
|
I'll take a look |
ashgti
added a commit
to ashgti/llvm-project
that referenced
this pull request
Jan 13, 2025
…he DAP object lifecycle. (llvm#120457)" This reverts commit 81898ac.
labath
added a commit
to labath/llvm-project
that referenced
this pull request
Jan 22, 2025
Using a "random" name for an "anonymous" pipe seems to be the state of the art on windows (according to stack overflow, new windows versions may have something better, but it involves calling kernel APIs directly and generally a lot of dark magic). The problem with the current method was that is does not produce unique names if one has two copies of the pipe code in the same process, which is what happened with llvm#120457 (because liblldb only exposes the public api, and we've started using the pipe code in lldb-dap as well). This patch works around the problem by adding the address of the counter variable to the pipe name. Replicating the multiple-copies setup in a test would be very difficult, which is why I'm not adding a test for this scenario.
labath
added a commit
that referenced
this pull request
Jan 23, 2025
Using a "random" name for an "anonymous" pipe seems to be the state of the art on windows (according to stack overflow, new windows versions may have something better, but it involves calling kernel APIs directly and generally a lot of dark magic). The problem with the current method was that is does not produce unique names if one has two copies of the pipe code in the same process, which is what happened with #120457 (because liblldb only exposes the public api, and we've started using the pipe code in lldb-dap as well). This patch works around the problem by adding the address of the counter variable to the pipe name. Replicating the multiple-copies setup in a test would be very difficult, which is why I'm not adding a test for this scenario.
ashgti
added a commit
to ashgti/llvm-project
that referenced
this pull request
Jan 23, 2025
…he DAP object lifecycle. (llvm#120457)" This reverts commit 81898ac.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This moves the ownership of the threads that forward stdout/stderr to the DAP object itself to ensure that the threads are joined and that the forwarding is cleaned up when the DAP connection is disconnected.
This is part of a larger refactor to allow lldb-dap to run in a listening mode and accept multiple connections.