[lldb-dap] Implement runInTerminal for Windows
#121269
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| CXX_SOURCES := main.cpp | ||
|
|
||
| include Makefile.rules |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| #include <chrono> | ||
| #include <cstdlib> | ||
| #include <thread> | ||
|
|
||
| using namespace std; | ||
|
|
||
| int main(int argc, char *argv[]) { | ||
| const char *foo = getenv("FOO"); | ||
| for (int counter = 1;; counter++) { | ||
| this_thread::sleep_for(chrono::seconds(1)); // breakpoint | ||
| } | ||
| return 0; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,8 +8,15 @@ | |
|
|
||
| #include "FifoFiles.h" | ||
| #include "JSONUtils.h" | ||
| #include "llvm/Support/FileSystem.h" | ||
| #include "llvm/Support/Path.h" | ||
| #include "llvm/Support/WindowsError.h" | ||
|
|
||
| #if defined(_WIN32) | ||
| #include <Windows.h> | ||
| #include <fcntl.h> | ||
| #include <io.h> | ||
| #else | ||
| #include <sys/stat.h> | ||
| #include <sys/types.h> | ||
| #include <unistd.h> | ||
|
|
@@ -24,41 +31,105 @@ using namespace llvm; | |
|
|
||
| namespace lldb_dap { | ||
|
|
||
| FifoFile::FifoFile(std::string path, FILE *f) : m_path(path), m_file(f) {} | ||
|
|
||
| Expected<FifoFile> FifoFile::create(StringRef path) { | ||
| auto file = fopen(path.data(), "r+"); | ||
| if (file == nullptr) | ||
| return createStringError(inconvertibleErrorCode(), | ||
| "Failed to open fifo file " + path); | ||
| if (setvbuf(file, NULL, _IONBF, 0)) | ||
| return createStringError(inconvertibleErrorCode(), | ||
| "Failed to setvbuf on fifo file " + path); | ||
| return FifoFile(path, file); | ||
| } | ||
|
|
||
| FifoFile::FifoFile(StringRef path, FILE *f) : m_path(path), m_file(f) {} | ||
|
|
||
| FifoFile::FifoFile(FifoFile &&other) | ||
| : m_path(other.m_path), m_file(other.m_file) { | ||
| other.m_path.clear(); | ||
| other.m_file = nullptr; | ||
| } | ||
| FifoFile::~FifoFile() { | ||
| if (m_file) | ||
| fclose(m_file); | ||
| #if !defined(_WIN32) | ||
| // Unreferenced named pipes are deleted automatically on Win32 | ||
| unlink(m_path.c_str()); | ||
| #endif | ||
| } | ||
|
|
||
| // This probably belongs to llvm::sys::fs as another FSEntity type | ||
| Error createUniqueNamedPipe(const Twine &prefix, StringRef suffix, | ||
| int &result_fd, | ||
| SmallVectorImpl<char> &result_path) { | ||
| std::error_code EC; | ||
| #ifdef _WIN32 | ||
| const char *middle = suffix.empty() ? "-%%%%%%" : "-%%%%%%."; | ||
| EC = sys::fs::getPotentiallyUniqueFileName( | ||
| "\\\\.\\pipe\\LOCAL\\" + prefix + middle + suffix, result_path); | ||
| #else | ||
| EC = sys::fs::getPotentiallyUniqueTempFileName(prefix, suffix, result_path); | ||
| #endif | ||
| if (EC) | ||
| return errorCodeToError(EC); | ||
| result_path.push_back(0); | ||
| const char *path = result_path.data(); | ||
| #ifdef _WIN32 | ||
| HANDLE h = ::CreateNamedPipeA( | ||
| path, PIPE_ACCESS_DUPLEX | FILE_FLAG_FIRST_PIPE_INSTANCE, | ||
|
There was a problem hiding this comment.
I looked up how pipes work on Windows a bit more and it looks like the pipe is being opened in duplex mode, which allows it to read and write. A fifo on linux or macOS is unidirectional, when its opened for reading it blocks until the file is also opened for writing and vice versa. This causes lldb-dap to have some synchronization points between the two processes as they're reading or writing to the fifo file. However, with this change it looks like we're attempt to open the file in Should we instead switch to a communication channel that supports reading and writing for all platforms? For example, we could open a socket for local communication and set the port of the socket instead of using a fifo. Sockets do support both reading and writing on all platforms and we have some existing helpers for supporting sockets already. Is that something you might have a chance to look into? If not, I could take some time to look at converting this to use sockets in the near future. There was a problem hiding this comment.
This is implemented with
Intra-process deadlock (waiting on its own pending output) should not be possible, since on Windows a process cannot even read back what it wrote into a named pipe from the same handle. The data flow is bidirectional, but can only be consumed by the other end. Inter-process deadlock, with the exception of buffering, is also rather unlikely. Current protocol over the pipe is strictly synchronous and sequential, or "half-duplex" in some sense -- there are no overlapping transmissions; in other words, the process will not transmit before expected response is fully received, and will not block on read before it sends out the message. Buffering can still cause deadlocks. Without the
Using sockets feels a bit like overkill, but would definitely make code more uniform across platforms I suppose. I can give it a try if you deem it more maintainable.
As an aside, I remember I did also attempt to retrofit the use case into
I am not too familiar with that so unable to give guarantees, but let me give it a try. |
||
| PIPE_TYPE_BYTE | PIPE_READMODE_BYTE | PIPE_WAIT, 1, 512, 512, 0, NULL); | ||
| if (h == INVALID_HANDLE_VALUE) | ||
| return createStringError(mapLastWindowsError(), "CreateNamedPipe"); | ||
| result_fd = _open_osfhandle((intptr_t)h, _O_TEXT | _O_RDWR); | ||
| if (result_fd == -1) | ||
| return createStringError(mapLastWindowsError(), "_open_osfhandle"); | ||
| #else | ||
| if (mkfifo(path, 0600) == -1) | ||
| return createStringError(std::error_code(errno, std::generic_category()), | ||
| "mkfifo"); | ||
| EC = openFileForWrite(result_path, result_fd, sys::fs::CD_OpenExisting, | ||
| sys::fs::OF_None, 0600); | ||
| if (EC) | ||
| return errorCodeToError(EC); | ||
| #endif | ||
| result_path.pop_back(); | ||
|
There was a problem hiding this comment. Any reason why this is being removed from the end of the path? I think thats the There was a problem hiding this comment. Yes, it is the NUL. It would be encoded into JSON if not popped. I think it might have always been buggy and just happened to work, as NUL termination from |
||
| return Error::success(); | ||
| } | ||
|
|
||
| FifoFileIO::FifoFileIO(FifoFile &&fifo_file, StringRef other_endpoint_name) | ||
| : m_fifo_file(std::move(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, [&]() { | ||
| rewind(m_fifo_file.m_file); | ||
|
There was a problem hiding this comment. Do we need to rewind the file? Shouldn't the position be at the beginning of the file already? There was a problem hiding this comment. It seemed to help on Windows, but I did not investigate further, as it is technically also required by ISO C standard (C23 draft N3220 §7.23.5.3.7) which C++ delegate to for C libraries (
There was a problem hiding this comment. I think the flow of the logic here is:
This should mean that the parent side only reads from the file and the child side only writes to the file. Additionally, I think they're only each performing 1 write/read on each side so their respective file positions should still be at the beginning of the file after opening. There was a problem hiding this comment. The DA side ("PID 1") sends a There was a problem hiding this comment. I think seeking isn't support on a fifo ( There was a problem hiding this comment. Practically it should not be needed, but I am tempted to leave the Citing C89 rationale §4.9.5.3
FIFO can also possibly have buffering, and the requirement makes sense. I suppose the timeout I observed on Windows with |
||
| /* | ||
| * The types of messages passing through the fifo are: | ||
| * {"kind": "pid", "pid": 9223372036854775807} | ||
| * {"kind": "didAttach"} | ||
| * {"kind": "error", "error": "error message"} | ||
| * | ||
| * 512 characters should be more than enough. | ||
| */ | ||
| constexpr size_t buffer_size = 512; | ||
| char buffer[buffer_size]; | ||
| char *ptr = fgets(buffer, buffer_size, m_fifo_file.m_file); | ||
| if (ptr == nullptr || *ptr == 0) | ||
| return; | ||
| size_t len = strlen(buffer); | ||
| if (len <= 1) | ||
| return; | ||
| buffer[len - 1] = '\0'; // remove newline | ||
| line = buffer; | ||
|
Comment on lines
+121
to
+129
There was a problem hiding this comment. The previous version would get a full line, however this new version will read until the buffer is full or EOF. The JSON used is smaller than the buffer size, so this should be okay, but if the JSON value was changed then this could be an issue. Should we read until EOF? Or Should we read until we find a newline specifically? There was a problem hiding this comment. We can reinvent POSIX |
||
| })); | ||
|
|
||
| if (future->wait_for(timeout) == std::future_status::timeout) | ||
| // 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 | ||
|
|
@@ -69,6 +140,11 @@ Expected<json::Value> FifoFileIO::ReadJSON(std::chrono::milliseconds timeout) { | |
| return createStringError(inconvertibleErrorCode(), | ||
| "Timed out trying to get messages from the " + | ||
| m_other_endpoint_name); | ||
| if (!line) { | ||
| return createStringError(inconvertibleErrorCode(), | ||
| "Failed to get messages from the " + | ||
| m_other_endpoint_name); | ||
| } | ||
| delete future; | ||
| return json::parse(*line); | ||
| } | ||
|
|
@@ -78,8 +154,12 @@ Error FifoFileIO::SendJSON(const json::Value &json, | |
| bool done = false; | ||
| std::future<void> *future = | ||
| new std::future<void>(std::async(std::launch::async, [&]() { | ||
| rewind(m_fifo_file.m_file); | ||
SuibianP marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| auto payload = JSONToString(json); | ||
| if (fputs(payload.c_str(), m_fifo_file.m_file) == EOF || | ||
| fputc('\n', m_fifo_file.m_file) == EOF) { | ||
| return; | ||
| } | ||
| done = true; | ||
| })); | ||
| if (future->wait_for(timeout) == std::future_status::timeout || !done) { | ||
|
|
@@ -98,4 +178,17 @@ Error FifoFileIO::SendJSON(const json::Value &json, | |
| return Error::success(); | ||
| } | ||
|
|
||
| Error FifoFileIO::WaitForPeer() { | ||
| #ifdef _WIN32 | ||
| if (!::ConnectNamedPipe((HANDLE)_get_osfhandle(fileno(m_fifo_file.m_file)), | ||
| NULL) && | ||
| GetLastError() != ERROR_PIPE_CONNECTED) { | ||
| return createStringError(mapLastWindowsError(), | ||
| "Failed to connect to the " + | ||
| m_other_endpoint_name); | ||
| } | ||
| #endif | ||
| return Error::success(); | ||
| } | ||
|
|
||
| } // namespace lldb_dap | ||
Uh oh!
There was an error while loading. Please reload this page.