[lldb-dap] Implement runInTerminal for Windows

Currently, the named pipe is passed by name and a transient ofstream is
constructed at each I/O request. This assumes,
  - Blocking semantics: FIFO I/O waits for the other side to connect.
  - Buffered semantics: Closing one side does not discard existing data.

The former can be replaced by WaitNamedPipe/ConnectNamedPipe on Win32,
but the second cannot be easily worked around. It is also impossible to
have another "keep-alive" pipe server instance, as server-client pairs
are fixed on connection on Win32 and the client may get connected to it
instead of the real one.

Refactor FifoFile[IO] to use an open file handles rather than file name.

Author: SuibianP

lldb/tools/lldb-dap/FifoFiles.cpp

@@ -9,7 +9,13 @@
 #include "FifoFiles.h"
 #include "JSONUtils.h"
 
-#if !defined(_WIN32)
+#include "llvm/Support/FileSystem.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,26 +30,65 @@ using namespace llvm;
 
 namespace lldb_dap {
 
-FifoFile::FifoFile(StringRef path) : m_path(path) {}
+std::error_code EC;
 
+FifoFile::FifoFile(StringRef path)
+    : m_path(path), m_file(fopen(path.data(), "r+")) {
+  if (m_file == nullptr) {
+    EC = std::error_code(errno, std::generic_category());
+    llvm::errs() << "Failed to open fifo file: " << path << EC.message()
+                 << "\n";
+    return;
+  }
+  if (setvbuf(m_file, NULL, _IONBF, 0))
+    llvm::errs() << "Error setting unbuffered mode on C FILE\n";
+}
+FifoFile::FifoFile(StringRef path, FILE *f) : m_path(path), m_file(f) {}
 FifoFile::~FifoFile() {
+  fclose(m_file);
 #if !defined(_WIN32)
   unlink(m_path.c_str());
 #endif
+  // Unreferenced named pipes are deleted automatically on Win32
 }
 
-Expected<std::shared_ptr<FifoFile>> CreateFifoFile(StringRef path) {
-#if defined(_WIN32)
-  return createStringError(inconvertibleErrorCode(), "Unimplemented");
+// This probably belongs to llvm::sys::fs as another FSEntity type
+std::error_code createNamedPipe(const Twine &Prefix, StringRef Suffix,
+                                int &ResultFd,
+                                SmallVectorImpl<char> &ResultPath) {
+  const char *Middle = Suffix.empty() ? "-%%%%%%" : "-%%%%%%.";
+  auto EC = sys::fs::getPotentiallyUniqueFileName(
+#ifdef _WIN32
+      "\\\\.\\pipe\\LOCAL\\"
+#else
+      "/tmp/"
+#endif
+          + Prefix + Middle + Suffix,
+      ResultPath);
+  if (EC)
+    return EC;
+  ResultPath.push_back(0);
+  const char *path = ResultPath.data();
+#ifdef _WIN32
+  HANDLE h = ::CreateNamedPipeA(
+      path, PIPE_ACCESS_DUPLEX | FILE_FLAG_FIRST_PIPE_INSTANCE,
+      PIPE_TYPE_BYTE | PIPE_READMODE_BYTE | PIPE_WAIT, 1, 1024, 1024, 0, NULL);
+  if (h == INVALID_HANDLE_VALUE)
+    return std::error_code(::GetLastError(), std::system_category());
+  ResultFd = _open_osfhandle((intptr_t)h, _O_TEXT | _O_RDWR);
+  if (ResultFd == -1)
+    return std::error_code(::GetLastError(), std::system_category());
 #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);
+  if (mkfifo(path, 0600) == -1)
+    return std::error_code(errno, std::generic_category());
+  EC = openFileForWrite(ResultPath, ResultFd, sys::fs::CD_OpenExisting, sys::fs::OF_None, 0600);
+  if (EC)
+    return EC;
 #endif
+  return std::error_code();
 }
 
-FifoFileIO::FifoFileIO(StringRef fifo_file, StringRef other_endpoint_name)
+FifoFileIO::FifoFileIO(FifoFile 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) {
@@ -52,13 +97,20 @@ Expected<json::Value> FifoFileIO::ReadJSON(std::chrono::milliseconds timeout) {
   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;
+        rewind(m_fifo_file.m_file);
+        constexpr size_t buffer_size = 2048;
+        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;
       }));
-  if (future->wait_for(timeout) == std::future_status::timeout || !line)
+
+  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 +121,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 +135,12 @@ Error FifoFileIO::SendJSON(const json::Value &json,
   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;
+        rewind(m_fifo_file.m_file);
+        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 +159,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(
+        std::error_code(GetLastError(), std::system_category()),
+        "Failed to connect to the " + m_other_endpoint_name);
+  }
+#endif
+  return Error::success();
+}
+
 } // namespace lldb_dap

lldb/tools/lldb-dap/FifoFiles.h

@@ -11,8 +11,10 @@
 
 #include "llvm/Support/Error.h"
 #include "llvm/Support/JSON.h"
+#include "llvm/Support/raw_ostream.h"
 
 #include <chrono>
+#include <fstream>
 
 namespace lldb_dap {
 
@@ -21,21 +23,18 @@ namespace lldb_dap {
 /// The file is destroyed when the destructor is invoked.
 struct FifoFile {
   FifoFile(llvm::StringRef path);
+  FifoFile(llvm::StringRef path, FILE *f);
+  // FifoFile(llvm::StringRef path, FILE *f);
 
   ~FifoFile();
 
   std::string m_path;
+  FILE *m_file;
 };
 
-/// 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);
+std::error_code createNamedPipe(const llvm::Twine &Prefix,
+                                llvm::StringRef Suffix, int &ResultFd,
+                                llvm::SmallVectorImpl<char> &ResultPath);
 
 class FifoFileIO {
 public:
@@ -45,7 +44,7 @@ class FifoFileIO {
   /// \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);
+  FifoFileIO(FifoFile fifo_file, llvm::StringRef other_endpoint_name);
 
   /// Read the next JSON object from the underlying input fifo file.
   ///
@@ -71,12 +70,14 @@ class FifoFileIO {
   /// \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));
+  llvm::Error
+  SendJSON(const llvm::json::Value &json,
+           std::chrono::milliseconds timeout = std::chrono::milliseconds(2000));
+
+  llvm::Error WaitForPeer();
 
-private:
-  std::string m_fifo_file;
+  // private:
+  FifoFile m_fifo_file;
   std::string m_other_endpoint_name;
 };
 

lldb/tools/lldb-dap/RunInTerminal.cpp

@@ -97,7 +97,7 @@ static Error ToError(const RunInTerminalMessage &message) {
 
 RunInTerminalLauncherCommChannel::RunInTerminalLauncherCommChannel(
     StringRef comm_file)
-    : m_io(comm_file, "debug adaptor") {}
+    : m_io(FifoFile(comm_file), "debug adaptor") {}
 
 Error RunInTerminalLauncherCommChannel::WaitUntilDebugAdaptorAttaches(
     std::chrono::milliseconds timeout) {
@@ -122,9 +122,13 @@ void RunInTerminalLauncherCommChannel::NotifyError(StringRef error) {
 }
 
 RunInTerminalDebugAdapterCommChannel::RunInTerminalDebugAdapterCommChannel(
-    StringRef comm_file)
+    FifoFile comm_file)
     : m_io(comm_file, "runInTerminal launcher") {}
 
+Error RunInTerminalDebugAdapterCommChannel::WaitForLauncher() {
+  return m_io.WaitForPeer();
+}
+
 // Can't use \a std::future<llvm::Error> because it doesn't compile on Windows
 std::future<lldb::SBError>
 RunInTerminalDebugAdapterCommChannel::NotifyDidAttach() {
@@ -158,13 +162,24 @@ std::string RunInTerminalDebugAdapterCommChannel::GetLauncherError() {
 }
 
 Expected<std::shared_ptr<FifoFile>> CreateRunInTerminalCommFile() {
+  int comm_fd;
   SmallString<256> comm_file;
-  if (std::error_code EC = sys::fs::getPotentiallyUniqueTempFileName(
-          "lldb-dap-run-in-terminal-comm", "", comm_file))
+  if (std::error_code EC = createNamedPipe("lldb-dap-run-in-terminal-comm", "",
+                                           comm_fd, comm_file))
     return createStringError(EC, "Error making unique file name for "
                                  "runInTerminal communication files");
-
-  return CreateFifoFile(comm_file.str());
+  FILE *cf = fdopen(comm_fd, "r+");
+  if (setvbuf(cf, NULL, _IONBF, 0))
+    return createStringError(std::error_code(errno, std::generic_category()),
+                             "Error setting unbuffered mode on C FILE");
+  // There is no portable way to conjure an ofstream from HANDLE, so use FILE *
+  // llvm::raw_fd_stream does not support getline() and there is no
+  // llvm::buffer_istream
+
+  if (cf == NULL)
+    return createStringError(std::error_code(errno, std::generic_category()),
+                             "Error converting file descriptor to C FILE");
+  return std::make_shared<FifoFile>(comm_file, cf);
 }
 
 } // namespace lldb_dap

lldb/tools/lldb-dap/RunInTerminal.h

@@ -98,7 +98,7 @@ class RunInTerminalLauncherCommChannel {
 
 class RunInTerminalDebugAdapterCommChannel {
 public:
-  RunInTerminalDebugAdapterCommChannel(llvm::StringRef comm_file);
+  RunInTerminalDebugAdapterCommChannel(FifoFile comm_file);
 
   /// Notify the runInTerminal launcher that it was attached.
   ///
@@ -118,6 +118,8 @@ class RunInTerminalDebugAdapterCommChannel {
   /// default error message if a certain timeout if reached.
   std::string GetLauncherError();
 
+  llvm::Error WaitForLauncher();
+
 private:
   FifoFileIO m_io;
 };

lldb/tools/lldb-dap/lldb-dap.cpp

@@ -2007,7 +2007,7 @@ llvm::Error request_runInTerminal(DAP &dap,
     return comm_file_or_err.takeError();
   FifoFile &comm_file = *comm_file_or_err.get();
 
-  RunInTerminalDebugAdapterCommChannel comm_channel(comm_file.m_path);
+  RunInTerminalDebugAdapterCommChannel comm_channel(comm_file);
 
   lldb::pid_t debugger_pid = LLDB_INVALID_PROCESS_ID;
 #if !defined(_WIN32)
@@ -2025,14 +2025,19 @@ llvm::Error request_runInTerminal(DAP &dap,
                            }
                          });
 
+  auto err = comm_channel.WaitForLauncher();
+  if (err)
+    return err;
   if (llvm::Expected<lldb::pid_t> pid = comm_channel.GetLauncherPid())
     attach_info.SetProcessID(*pid);
   else
     return pid.takeError();
 
   dap.debugger.SetAsync(false);
   lldb::SBError error;
+  llvm::errs() << "Attaching to target\n";
   dap.target.Attach(attach_info, error);
+  llvm::errs() << "Attached to target\n";
 
   if (error.Fail())
     return llvm::createStringError(llvm::inconvertibleErrorCode(),
@@ -4860,11 +4865,6 @@ static void printHelp(LLDBDAPOptTable &table, llvm::StringRef tool_name) {
 static void LaunchRunInTerminalTarget(llvm::opt::Arg &target_arg,
                                       llvm::StringRef comm_file,
                                       lldb::pid_t debugger_pid, char *argv[]) {
-#if defined(_WIN32)
-  llvm::errs() << "runInTerminal is only supported on POSIX systems\n";
-  exit(EXIT_FAILURE);
-#else
-
   // On Linux with the Yama security module enabled, a process can only attach
   // to its descendants by default. In the runInTerminal case the target
   // process is launched by the client so we need to allow tracing explicitly.
@@ -4899,7 +4899,6 @@ static void LaunchRunInTerminalTarget(llvm::opt::Arg &target_arg,
   comm_channel.NotifyError(error);
   llvm::errs() << error << "\n";
   exit(EXIT_FAILURE);
-#endif
 }
 
 /// used only by TestVSCode_redirection_to_console.py