[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.

---

Win32 provides no way to replace the process image. Under the hood exec*
actually creates a new process with a new PID. DebugActiveProcess also
cannot get notified of process creations.

Create the new process in a suspended state and resume it after attach.

Author: SuibianP

lldb/packages/Python/lldbsuite/test/dotest.py

@@ -545,7 +545,7 @@ def setupSysPath():
 
     lldbDir = os.path.dirname(lldbtest_config.lldbExec)
 
-    lldbDAPExec = os.path.join(lldbDir, "lldb-dap")
+    lldbDAPExec = os.path.join(lldbDir, "lldb-dap.exe" if os.name == "nt" else "lldb-dap")
     if is_exe(lldbDAPExec):
         os.environ["LLDBDAP_EXEC"] = lldbDAPExec
 

lldb/test/API/tools/lldb-dap/runInTerminal/TestDAP_runInTerminal.py

@@ -43,7 +43,6 @@ def isTestSupported(self):
         except:
             return False
 
-    @skipIfWindows
     @skipIf(archs=no_match(["x86_64"]))
     def test_runInTerminal(self):
         if not self.isTestSupported():
@@ -90,7 +89,6 @@ def test_runInTerminal(self):
         env = self.dap_server.request_evaluate("foo")["body"]["result"]
         self.assertIn("bar", env)
 
-    @skipIf(archs=no_match(["x86_64"]))
     def test_runInTerminalWithObjectEnv(self):
         if not self.isTestSupported():
             return
@@ -113,7 +111,6 @@ def test_runInTerminalWithObjectEnv(self):
         self.assertIn("FOO", request_envs)
         self.assertEqual("BAR", request_envs["FOO"])
 
-    @skipIfWindows
     @skipIf(archs=no_match(["x86_64"]))
     def test_runInTerminalInvalidTarget(self):
         if not self.isTestSupported():
@@ -132,7 +129,6 @@ def test_runInTerminalInvalidTarget(self):
             response["message"],
         )
 
-    @skipIfWindows
     @skipIf(archs=no_match(["x86_64"]))
     def test_missingArgInRunInTerminalLauncher(self):
         if not self.isTestSupported():

lldb/test/API/tools/lldb-dap/runInTerminal/main.c

@@ -1,11 +1,13 @@
 #include <stdio.h>
 #include <stdlib.h>
-#include <unistd.h>
+
+#include <threads.h>
+#include <time.h>
 
 int main(int argc, char *argv[]) {
   const char *foo = getenv("FOO");
   for (int counter = 1;; counter++) {
-    sleep(1); // breakpoint
+    thrd_sleep(&(struct timespec){.tv_sec = 1}, NULL); // breakpoint
   }
   return 0;
 }

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,41 +30,95 @@ 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";
+    std::terminate();
+  }
+  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(FifoFile &&other)
+    : m_path(other.m_path), m_file(other.m_file) {
+  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
 }
 
-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
+  ResultPath.pop_back();
+  return std::error_code();
 }
 
-FifoFileIO::FifoFileIO(StringRef fifo_file, StringRef other_endpoint_name)
-    : m_fifo_file(fifo_file), m_other_endpoint_name(other_endpoint_name) {}
+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, [&]() {
-        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 +129,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 +143,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 +167,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,22 @@ 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(FifoFile &&other);
+
+  FifoFile(const FifoFile &) = delete;
+  FifoFile &operator=(const FifoFile &) = delete;
 
   ~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 +48,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.
   ///
@@ -75,8 +78,10 @@ class FifoFileIO {
       const llvm::json::Value &json,
       std::chrono::milliseconds timeout = std::chrono::milliseconds(20000));
 
+  llvm::Error WaitForPeer();
+
 private:
-  std::string m_fifo_file;
+  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) {
@@ -111,8 +111,10 @@ Error RunInTerminalLauncherCommChannel::WaitUntilDebugAdaptorAttaches(
     return message.takeError();
 }
 
-Error RunInTerminalLauncherCommChannel::NotifyPid() {
-  return m_io.SendJSON(RunInTerminalMessagePid(getpid()).ToJSON());
+Error RunInTerminalLauncherCommChannel::NotifyPid(lldb::pid_t pid) {
+  if (pid == 0)
+    pid = getpid();
+  return m_io.SendJSON(RunInTerminalMessagePid(pid).ToJSON());
 }
 
 void RunInTerminalLauncherCommChannel::NotifyError(StringRef error) {
@@ -122,8 +124,12 @@ void RunInTerminalLauncherCommChannel::NotifyError(StringRef error) {
 }
 
 RunInTerminalDebugAdapterCommChannel::RunInTerminalDebugAdapterCommChannel(
-    StringRef comm_file)
-    : m_io(comm_file, "runInTerminal launcher") {}
+    FifoFile &comm_file)
+    : m_io(std::move(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>
@@ -158,13 +164,25 @@ 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 for "
+                             "runInTerminal comm-file");
+  return std::make_shared<FifoFile>(comm_file, cf);
 }
 
 } // namespace lldb_dap

lldb/tools/lldb-dap/RunInTerminal.h

@@ -87,7 +87,7 @@ class RunInTerminalLauncherCommChannel {
   /// \return
   ///     An \a llvm::Error object in case of errors or if this operation times
   ///     out.
-  llvm::Error NotifyPid();
+  llvm::Error NotifyPid(lldb::pid_t pid = 0);
 
   /// Notify the debug adaptor that there's been an error.
   void NotifyError(llvm::StringRef error);
@@ -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;
 };