[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/Makefile

@@ -1,3 +1,3 @@
-C_SOURCES := main.c
+CXX_SOURCES := main.cpp
 
 include Makefile.rules

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():
@@ -53,7 +52,7 @@ def test_runInTerminal(self):
             launch the inferior with the correct environment variables and arguments.
         """
         program = self.getBuildArtifact("a.out")
-        source = "main.c"
+        source = "main.cpp"
         self.build_and_launch(
             program, runInTerminal=True, args=["foobar"], env=["FOO=bar"]
         )
@@ -113,7 +112,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 +130,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

@@ -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;
+}

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

@@ -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)
+#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(StringRef path) : m_path(path) {}
+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
 }
 
-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
+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,
+      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 (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 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();
+  return Error::success();
 }
 
-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);
+        /*
+         * 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;
       }));
-  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 +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, [&]() {
-        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 +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

lldb/tools/lldb-dap/FifoFiles.cpp

@@ -11,31 +11,44 @@
 
 #include "llvm/Support/Error.h"
 #include "llvm/Support/JSON.h"
+#include "llvm/Support/raw_ostream.h"
 
 #include <chrono>
+#include <fstream>
 
 namespace lldb_dap {
 
 /// Struct that controls the life of a fifo file in the filesystem.
 ///
 /// The file is destroyed when the destructor is invoked.
 struct FifoFile {
-  FifoFile(llvm::StringRef path);
+  FifoFile(FifoFile &&other);
+
+  FifoFile(const FifoFile &) = delete;
+  FifoFile &operator=(const FifoFile &) = delete;
 
   ~FifoFile();
 
+  static llvm::Expected<FifoFile> create(llvm::StringRef path);
+  FifoFile(llvm::StringRef path, FILE *f);
+
   std::string m_path;
+  FILE *m_file;
+
+private:
+  FifoFile(std::string path, FILE *f);
 };
 
-/// Create a fifo file in the filesystem.
+/// Create and open a named pipe with a unique name.
 ///
-/// \param[in] path
-///     The path for the fifo file.
+/// The arguments have identical meanings to those of
+/// llvm::sys::fs::createTemporaryFile.
 ///
-/// \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);
+/// Note that the resulting filename is further prepended by \c \\.\pipe\\LOCAL\
+/// on Windows and the native temp directory on other platforms.
+llvm::Error createUniqueNamedPipe(const llvm::Twine &prefix,
+                                  llvm::StringRef suffix, int &result_fd,
+                                  llvm::SmallVectorImpl<char> &result_path);
 
 class FifoFileIO {
 public:
@@ -45,7 +58,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 +88,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/FifoFiles.h

@@ -104,18 +104,22 @@ static llvm::Error RunInTerminal(DAP &dap,
   if (!comm_file_or_err)
     return comm_file_or_err.takeError();
   FifoFile &comm_file = *comm_file_or_err.get();
+  std::string comm_filename = comm_file.m_path;
 
-  RunInTerminalDebugAdapterCommChannel comm_channel(comm_file.m_path);
+  RunInTerminalDebugAdapterCommChannel comm_channel(comm_file);
 
   lldb::pid_t debugger_pid = LLDB_INVALID_PROCESS_ID;
 #if !defined(_WIN32)
   debugger_pid = getpid();
 #endif
   llvm::json::Object reverse_request = CreateRunInTerminalReverseRequest(
-      launch_request, dap.debug_adapter_path, comm_file.m_path, debugger_pid);
+      launch_request, dap.debug_adapter_path, comm_filename, debugger_pid);
   dap.SendReverseRequest<LogFailureResponseHandler>("runInTerminal",
                                                     std::move(reverse_request));
 
+  auto err = comm_channel.WaitForLauncher();
+  if (err)
+    return err;
   if (llvm::Expected<lldb::pid_t> pid = comm_channel.GetLauncherPid())
     attach_info.SetProcessID(*pid);
   else