[lldb-dap] Use existing lldb::IOObjectSP for DAP IO (NFC). #128750
Conversation
This simplifies the IOStream.cpp implementation by building on top of the existing lldb::IOObjectSP. Additionally, this should help ensure clients connected of a `--connection` specifier properly detect shutdown requests when the Socket is closed. Previously, the StreamDescriptor was just accessing the underyling native handle and was not aware of the `Close()` call to the Socket itself.
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesThis simplifies the IOStream.cpp implementation by building on top of the existing lldb::IOObjectSP. Additionally, this should help ensure clients connected of a This is both nice to have for simplifying the existing code and this unblocks an upcoming refactor to support the cancel request. Full diff: https://github.com/llvm/llvm-project/pull/128750.diff 5 Files Affected:
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index 01f294e14de6a..95f2e5c6521c2 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -17,6 +17,7 @@
#include "lldb/API/SBListener.h"
#include "lldb/API/SBProcess.h"
#include "lldb/API/SBStream.h"
+#include "lldb/Utility/IOObject.h" // IWYU pragma: keep
#include "lldb/Utility/Status.h"
#include "lldb/lldb-defines.h"
#include "lldb/lldb-enumerations.h"
@@ -59,7 +60,7 @@ const char DEV_NULL[] = "/dev/null";
namespace lldb_dap {
DAP::DAP(std::string name, llvm::StringRef path, std::ofstream *log,
- StreamDescriptor input, StreamDescriptor output, ReplMode repl_mode,
+ lldb::IOObjectSP input, lldb::IOObjectSP output, ReplMode repl_mode,
std::vector<std::string> pre_init_commands)
: name(std::move(name)), debug_adaptor_path(path), log(log),
input(std::move(input)), output(std::move(output)),
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index f87841a56f4d3..b4e77b78c5273 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -29,6 +29,7 @@
#include "lldb/API/SBThread.h"
#include "lldb/API/SBValue.h"
#include "lldb/API/SBValueList.h"
+#include "lldb/lldb-forward.h"
#include "lldb/lldb-types.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/DenseSet.h"
@@ -125,21 +126,21 @@ struct Variables {
struct StartDebuggingRequestHandler : public lldb::SBCommandPluginInterface {
DAP &dap;
- explicit StartDebuggingRequestHandler(DAP &d) : dap(d) {};
+ explicit StartDebuggingRequestHandler(DAP &d) : dap(d){};
bool DoExecute(lldb::SBDebugger debugger, char **command,
lldb::SBCommandReturnObject &result) override;
};
struct ReplModeRequestHandler : public lldb::SBCommandPluginInterface {
DAP &dap;
- explicit ReplModeRequestHandler(DAP &d) : dap(d) {};
+ explicit ReplModeRequestHandler(DAP &d) : dap(d){};
bool DoExecute(lldb::SBDebugger debugger, char **command,
lldb::SBCommandReturnObject &result) override;
};
struct SendEventRequestHandler : public lldb::SBCommandPluginInterface {
DAP &dap;
- explicit SendEventRequestHandler(DAP &d) : dap(d) {};
+ explicit SendEventRequestHandler(DAP &d) : dap(d){};
bool DoExecute(lldb::SBDebugger debugger, char **command,
lldb::SBCommandReturnObject &result) override;
};
@@ -211,7 +212,7 @@ struct DAP {
std::string last_nonempty_var_expression;
DAP(std::string name, llvm::StringRef path, std::ofstream *log,
- StreamDescriptor input, StreamDescriptor output, ReplMode repl_mode,
+ lldb::IOObjectSP input, lldb::IOObjectSP output, ReplMode repl_mode,
std::vector<std::string> pre_init_commands);
~DAP();
DAP(const DAP &rhs) = delete;
diff --git a/lldb/tools/lldb-dap/IOStream.cpp b/lldb/tools/lldb-dap/IOStream.cpp
index 7d0f363440f53..d8543b560c050 100644
--- a/lldb/tools/lldb-dap/IOStream.cpp
+++ b/lldb/tools/lldb-dap/IOStream.cpp
@@ -7,125 +7,34 @@
//===----------------------------------------------------------------------===//
#include "IOStream.h"
+#include "lldb/Utility/IOObject.h" // IWYU pragma: keep
+#include "lldb/Utility/Status.h"
#include <fstream>
#include <string>
-#if defined(_WIN32)
-#include <io.h>
-#else
-#include <netinet/in.h>
-#include <sys/socket.h>
-#include <unistd.h>
-#endif
-
using namespace lldb_dap;
-StreamDescriptor::StreamDescriptor() = default;
-
-StreamDescriptor::StreamDescriptor(StreamDescriptor &&other) {
- *this = std::move(other);
-}
-
-StreamDescriptor::~StreamDescriptor() {
- if (!m_close)
- return;
-
- if (m_is_socket)
-#if defined(_WIN32)
- ::closesocket(m_socket);
-#else
- ::close(m_socket);
-#endif
- else
- ::close(m_fd);
-}
-
-StreamDescriptor &StreamDescriptor::operator=(StreamDescriptor &&other) {
- m_close = other.m_close;
- other.m_close = false;
- m_is_socket = other.m_is_socket;
- if (m_is_socket)
- m_socket = other.m_socket;
- else
- m_fd = other.m_fd;
- return *this;
-}
-
-StreamDescriptor StreamDescriptor::from_socket(SOCKET s, bool close) {
- StreamDescriptor sd;
- sd.m_is_socket = true;
- sd.m_socket = s;
- sd.m_close = close;
- return sd;
-}
-
-StreamDescriptor StreamDescriptor::from_file(int fd, bool close) {
- StreamDescriptor sd;
- sd.m_is_socket = false;
- sd.m_fd = fd;
- sd.m_close = close;
- return sd;
-}
-
bool OutputStream::write_full(llvm::StringRef str) {
- while (!str.empty()) {
- int bytes_written = 0;
- if (descriptor.m_is_socket)
- bytes_written = ::send(descriptor.m_socket, str.data(), str.size(), 0);
- else
- bytes_written = ::write(descriptor.m_fd, str.data(), str.size());
-
- if (bytes_written < 0) {
- if (errno == EINTR || errno == EAGAIN)
- continue;
- return false;
- }
- str = str.drop_front(bytes_written);
- }
+ if (!descriptor)
+ return false;
- return true;
+ size_t num_bytes = str.size();
+ auto status = descriptor->Write(str.data(), num_bytes);
+ return status.Success();
}
bool InputStream::read_full(std::ofstream *log, size_t length,
std::string &text) {
+ if (!descriptor)
+ return false;
+
std::string data;
data.resize(length);
- char *ptr = &data[0];
- while (length != 0) {
- int bytes_read = 0;
- if (descriptor.m_is_socket)
- bytes_read = ::recv(descriptor.m_socket, ptr, length, 0);
- else
- bytes_read = ::read(descriptor.m_fd, ptr, length);
-
- if (bytes_read == 0) {
- if (log)
- *log << "End of file (EOF) reading from input file.\n";
- return false;
- }
- if (bytes_read < 0) {
- int reason = 0;
-#if defined(_WIN32)
- if (descriptor.m_is_socket)
- reason = WSAGetLastError();
- else
- reason = errno;
-#else
- reason = errno;
- if (reason == EINTR || reason == EAGAIN)
- continue;
-#endif
-
- if (log)
- *log << "Error " << reason << " reading from input file.\n";
- return false;
- }
+ auto status = descriptor->Read(data.data(), length);
+ if (status.Fail())
+ return false;
- assert(bytes_read >= 0 && (size_t)bytes_read <= length);
- ptr += bytes_read;
- length -= bytes_read;
- }
text += data;
return true;
}
diff --git a/lldb/tools/lldb-dap/IOStream.h b/lldb/tools/lldb-dap/IOStream.h
index c91b2f717893c..e9fb8e11c92da 100644
--- a/lldb/tools/lldb-dap/IOStream.h
+++ b/lldb/tools/lldb-dap/IOStream.h
@@ -9,45 +9,17 @@
#ifndef LLDB_TOOLS_LLDB_DAP_IOSTREAM_H
#define LLDB_TOOLS_LLDB_DAP_IOSTREAM_H
-#if defined(_WIN32)
-#include "lldb/Host/windows/windows.h"
-#include <winsock2.h>
-#else
-typedef int SOCKET;
-#endif
-
+#include "lldb/lldb-forward.h"
#include "llvm/ADT/StringRef.h"
-
#include <fstream>
#include <string>
-// Windows requires different system calls for dealing with sockets and other
-// types of files, so we can't simply have one code path that just uses read
-// and write everywhere. So we need an abstraction in order to allow us to
-// treat them identically.
namespace lldb_dap {
-struct StreamDescriptor {
- StreamDescriptor();
- ~StreamDescriptor();
- StreamDescriptor(StreamDescriptor &&other);
-
- StreamDescriptor &operator=(StreamDescriptor &&other);
-
- static StreamDescriptor from_socket(SOCKET s, bool close);
- static StreamDescriptor from_file(int fd, bool close);
-
- bool m_is_socket = false;
- bool m_close = false;
- union {
- int m_fd;
- SOCKET m_socket;
- };
-};
struct InputStream {
- StreamDescriptor descriptor;
+ lldb::IOObjectSP descriptor;
- explicit InputStream(StreamDescriptor descriptor)
+ explicit InputStream(lldb::IOObjectSP descriptor)
: descriptor(std::move(descriptor)) {}
bool read_full(std::ofstream *log, size_t length, std::string &text);
@@ -58,9 +30,9 @@ struct InputStream {
};
struct OutputStream {
- StreamDescriptor descriptor;
+ lldb::IOObjectSP descriptor;
- explicit OutputStream(StreamDescriptor descriptor)
+ explicit OutputStream(lldb::IOObjectSP descriptor)
: descriptor(std::move(descriptor)) {}
bool write_full(llvm::StringRef str);
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index 3b029b2ef047a..c94faa4958039 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -15,6 +15,7 @@
#include "RunInTerminal.h"
#include "lldb/API/SBStream.h"
#include "lldb/Host/Config.h"
+#include "lldb/Host/File.h"
#include "lldb/Host/MainLoop.h"
#include "lldb/Host/MainLoopBase.h"
#include "lldb/Host/Socket.h"
@@ -80,7 +81,11 @@ typedef int socklen_t;
#endif
using namespace lldb_dap;
-using lldb_private::NativeSocket;
+using lldb_private::File;
+using lldb_private::IOObject;
+using lldb_private::MainLoop;
+using lldb_private::MainLoopBase;
+using lldb_private::NativeFile;
using lldb_private::Socket;
using lldb_private::Status;
@@ -309,14 +314,14 @@ serveConnection(const Socket::SocketProtocol &protocol, const std::string &name,
// address.
llvm::outs().flush();
- static lldb_private::MainLoop g_loop;
+ static MainLoop g_loop;
llvm::sys::SetInterruptFunction([]() {
g_loop.AddPendingCallback(
- [](lldb_private::MainLoopBase &loop) { loop.RequestTermination(); });
+ [](MainLoopBase &loop) { loop.RequestTermination(); });
});
std::condition_variable dap_sessions_condition;
std::mutex dap_sessions_mutex;
- std::map<Socket *, DAP *> dap_sessions;
+ std::map<IOObject *, DAP *> dap_sessions;
unsigned int clientCount = 0;
auto handle = listener->Accept(g_loop, [=, &dap_sessions_condition,
&dap_sessions_mutex, &dap_sessions,
@@ -330,18 +335,15 @@ serveConnection(const Socket::SocketProtocol &protocol, const std::string &name,
<< " client connected: " << name << "\n";
}
+ lldb::IOObjectSP io(std::move(sock));
+
// Move the client into a background thread to unblock accepting the next
// client.
std::thread client([=, &dap_sessions_condition, &dap_sessions_mutex,
- &dap_sessions, sock = std::move(sock)]() {
+ &dap_sessions]() {
llvm::set_thread_name(name + ".runloop");
- StreamDescriptor input =
- StreamDescriptor::from_socket(sock->GetNativeSocket(), false);
- // Close the output last for the best chance at error reporting.
- StreamDescriptor output =
- StreamDescriptor::from_socket(sock->GetNativeSocket(), false);
- DAP dap = DAP(name, program_path, log, std::move(input),
- std::move(output), default_repl_mode, pre_init_commands);
+ DAP dap = DAP(name, program_path, log, io, io, default_repl_mode,
+ pre_init_commands);
if (auto Err = dap.ConfigureIO()) {
llvm::logAllUnhandledErrors(std::move(Err), llvm::errs(),
@@ -353,7 +355,7 @@ serveConnection(const Socket::SocketProtocol &protocol, const std::string &name,
{
std::scoped_lock<std::mutex> lock(dap_sessions_mutex);
- dap_sessions[sock.get()] = &dap;
+ dap_sessions[io.get()] = &dap;
}
if (auto Err = dap.Loop()) {
@@ -369,7 +371,7 @@ serveConnection(const Socket::SocketProtocol &protocol, const std::string &name,
}
std::unique_lock<std::mutex> lock(dap_sessions_mutex);
- dap_sessions.erase(sock.get());
+ dap_sessions.erase(io.get());
std::notify_all_at_thread_exit(dap_sessions_condition, std::move(lock));
});
client.detach();
@@ -561,8 +563,10 @@ int main(int argc, char *argv[]) {
return EXIT_FAILURE;
}
- StreamDescriptor input = StreamDescriptor::from_file(fileno(stdin), false);
- StreamDescriptor output = StreamDescriptor::from_file(stdout_fd, false);
+ lldb::IOObjectSP input = std::make_shared<NativeFile>(
+ fileno(stdin), File::eOpenOptionReadOnly, true);
+ lldb::IOObjectSP output = std::make_shared<NativeFile>(
+ stdout_fd, File::eOpenOptionWriteOnly, false);
DAP dap = DAP("stdin/stdout", program_path, log.get(), std::move(input),
std::move(output), default_repl_mode, pre_init_commands);
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
JDevlieghere
left a comment
JDevlieghere
left a comment
There was a problem hiding this comment.
Is it worth keeping InputStream and OutputStream around? Can we use a (Stream)File directly?
Co-authored-by: Jonas Devlieghere <[email protected]>
labath
left a comment
labath
left a comment
There was a problem hiding this comment.
This looks really nice. I also think the Input/OutputStreams could go away but that can be a separate patch.
However, this part
properly detect shutdown requests when the Socket is closed
really scares me. I must have missed this during the review, but there is no way to reliably terminate thread by closing a file descriptor from under it. There are at least two bad scenarios here:
- the socket FD gets reassigned before the thread manages to notice that, and it will start reading from a random socket. (This is more likely than it may seem because posix systems always assign the lowest available FD number)
- On linux at least, if the reading thread is blocked (at least in a select syscall, but probably read as well) on that fd, then closing the fd will not unblock the thread.
To reliably force termination, I'd suggest changing the read thread to use a MainLoop object reading from the socket, and then terminating it via loop.AddPendingCallback([&](MainLoopBase &loop) { loop.RequestTermination(); });. It sounds a bit like overkill, but the main loop class is currently our multiplexing primitive with the widest platform support. It also goes along nicely with the IOObject classes you're introducing here. And when/if we have a way (see the Pipe patch) for it to handle pipes as well, we could have it handle stdout/err forwarding as well, and ditch the forwarding threads.
(I can also imagine a setup where we reuse the top-level MainLoop object (the one that currently accepts connections). In this world the main thread would pull json data from (all) connections, and enqueue them to connection-specific request queues. The nice part about this approach is that there are fewer threads flying around, so it's easier to understand what's happening and easier to shut things down. The disadvantage is that the main thread becomes a bit of a choke point -- though I don't expect this to matter much in practice since we will usually only have one connection and I doubt that json (de)serialization is going to be the bottleneck)
The one issue with this is that MainLoop on Windows only supports sockets, not Files. At the moment we primarily work over stdin/stdout for communication, although the
I do think we could merge some of these threads if MainLoop supported Files on Windows. That may be worth tackling specifically to unblock this behavior. |
Co-authored-by: Jonas Devlieghere <[email protected]>
I'll take a look at removing them as part of the refactoring to support 'cancel' requests. I think it makes sense. |
Okay, I think I understand what you're saying. With the connection flag, we're communicating over sockets, and we only need to force termination in the socket case. However, the code that actually reads from the socket is generic and also needs to work in the "regular" mode -- which communicates over stdin/out.
That would be very nice, but I think that would have do be on you mostly. I should be able to support you with e.g. windows bits, but I don't think I'd be able to do everything. I'm actually sitting on a Socket refactor patch for a couple of months now that I haven't had time to clean up and submit. I do think we should do something about the close-in-another-thread thingy (it's an anti pattern that tends to show up a lot, and I'm very getting sensitive to it :/). Would it be possible to refactor the reading code so that it goes through the main loop if we're communicating over a socket, and reads directly from the IOObject otherwise? I don't expect the result to be pretty, but it feels like it should be possible to make it not-completely-ugly. |
This simplifies the IOStream.cpp implementation by building on top of the existing lldb::IOObjectSP. Additionally, this should help ensure clients connected of a `--connection` specifier properly detect shutdown requests when the Socket is closed. Previously, the StreamDescriptor was just accessing the underlying native handle and was not aware of the `Close()` call to the Socket itself. This is both nice to have for simplifying the existing code and this unblocks an upcoming refactor to support the cancel request. --------- Co-authored-by: Jonas Devlieghere <[email protected]>
This simplifies the IOStream.cpp implementation by building on top of the existing lldb::IOObjectSP. Additionally, this should help ensure clients connected of a `--connection` specifier properly detect shutdown requests when the Socket is closed. Previously, the StreamDescriptor was just accessing the underlying native handle and was not aware of the `Close()` call to the Socket itself. This is both nice to have for simplifying the existing code and this unblocks an upcoming refactor to support the cancel request. --------- Co-authored-by: Jonas Devlieghere <[email protected]>
4 participants
This simplifies the IOStream.cpp implementation by building on top of the existing lldb::IOObjectSP.
Additionally, this should help ensure clients connected of a
--connectionspecifier properly detect shutdown requests when the Socket is closed. Previously, the StreamDescriptor was just accessing the underlying native handle and was not aware of theClose()call to the Socket itself.This is both nice to have for simplifying the existing code and this unblocks an upcoming refactor to support the cancel request.