[lldb-server] Add unnamed pipe support to PipeWindows

Summary:
This adds unnamed pipe support in PipeWindows to support communication between a debug server and child process.
Modify PipeWindows::CreateNew to support the creation of an unnamed pipe.
Rename the previous method that created a named pipe to PipeWindows::CreateNewNamed.

Reviewers: zturner, llvm-commits

Reviewed By: zturner

Subscribers: Hui, labath, lldb-commits

Differential Revision: https://reviews.llvm.org/D56234

9 files changed, 107 insertions, 42 deletions

diff --git a/lldb/include/lldb/Host/PipeBase.h b/lldb/include/lldb/Host/PipeBase.h
index ad62072c7ba..88bd703960c 100644
--- a/lldb/include/lldb/Host/PipeBase.h
+++ b/lldb/include/lldb/Host/PipeBase.h
@@ -41,6 +41,9 @@ public:
   virtual bool CanRead() const = 0;
   virtual bool CanWrite() const = 0;
 
+  virtual lldb::pipe_t GetReadPipe() const = 0;
+  virtual lldb::pipe_t GetWritePipe() const = 0;
+
   virtual int GetReadFileDescriptor() const = 0;
   virtual int GetWriteFileDescriptor() const = 0;
   virtual int ReleaseReadFileDescriptor() = 0;
diff --git a/lldb/include/lldb/Host/posix/PipePosix.h b/lldb/include/lldb/Host/posix/PipePosix.h
index bb65a56abd0..30d19d97152 100644
--- a/lldb/include/lldb/Host/posix/PipePosix.h
+++ b/lldb/include/lldb/Host/posix/PipePosix.h
@@ -27,7 +27,7 @@ public:
   static int kInvalidDescriptor;
 
   PipePosix();
-  PipePosix(int read_fd, int write_fd);
+  PipePosix(lldb::pipe_t read, lldb::pipe_t write);
   PipePosix(const PipePosix &) = delete;
   PipePosix(PipePosix &&pipe_posix);
   PipePosix &operator=(const PipePosix &) = delete;
@@ -49,6 +49,13 @@ public:
   bool CanRead() const override;
   bool CanWrite() const override;
 
+  lldb::pipe_t GetReadPipe() const override {
+    return lldb::pipe_t(GetReadFileDescriptor());
+  }
+  lldb::pipe_t GetWritePipe() const override {
+    return lldb::pipe_t(GetWriteFileDescriptor());
+  }
+
   int GetReadFileDescriptor() const override;
   int GetWriteFileDescriptor() const override;
   int ReleaseReadFileDescriptor() override;
diff --git a/lldb/include/lldb/Host/windows/PipeWindows.h b/lldb/include/lldb/Host/windows/PipeWindows.h
index e309c421a71..1ae780ccc0b 100644
--- a/lldb/include/lldb/Host/windows/PipeWindows.h
+++ b/lldb/include/lldb/Host/windows/PipeWindows.h
@@ -24,10 +24,18 @@ namespace lldb_private {
 //----------------------------------------------------------------------
 class PipeWindows : public PipeBase {
 public:
+  static const int kInvalidDescriptor = -1;
+
+public:
   PipeWindows();
+  PipeWindows(lldb::pipe_t read, lldb::pipe_t write);
   ~PipeWindows() override;
 
+  // Create an unnamed pipe.
   Status CreateNew(bool child_process_inherit) override;
+
+  // Create a named pipe.
+  Status CreateNewNamed(bool child_process_inherit);
   Status CreateNew(llvm::StringRef name, bool child_process_inherit) override;
   Status CreateWithUniqueName(llvm::StringRef prefix,
                               bool child_process_inherit,
@@ -41,6 +49,9 @@ public:
   bool CanRead() const override;
   bool CanWrite() const override;
 
+  lldb::pipe_t GetReadPipe() const { return lldb::pipe_t(m_read); }
+  lldb::pipe_t GetWritePipe() const { return lldb::pipe_t(m_write); }
+
   int GetReadFileDescriptor() const override;
   int GetWriteFileDescriptor() const override;
   int ReleaseReadFileDescriptor() override;
diff --git a/lldb/include/lldb/lldb-types.h b/lldb/include/lldb/lldb-types.h
index 79f44156930..09dfc5c8a5e 100644
--- a/lldb/include/lldb/lldb-types.h
+++ b/lldb/include/lldb/lldb-types.h
@@ -47,7 +47,8 @@ typedef unsigned int __w64 socket_t; // Host socket type
 typedef void *thread_arg_t;                       // Host thread argument type
 typedef unsigned thread_result_t;                 // Host thread result type
 typedef thread_result_t (*thread_func_t)(void *); // Host thread function type
-}
+typedef void *pipe_t;                             // Host pipe type is HANDLE
+} // namespace lldb
 
 #else
 
@@ -65,6 +66,7 @@ typedef int socket_t;       // Host socket type
 typedef void *thread_arg_t;             // Host thread argument type
 typedef void *thread_result_t;          // Host thread result type
 typedef void *(*thread_func_t)(void *); // Host thread function type
+typedef int pipe_t;                     // Host pipe type
 } // namespace lldb
 
 #endif
@@ -76,10 +78,11 @@ typedef bool (*CommandOverrideCallbackWithResult)(
     void *baton, const char **argv, lldb_private::CommandReturnObject &result);
 typedef bool (*ExpressionCancelCallback)(ExpressionEvaluationPhase phase,
                                          void *baton);
-}
+} // namespace lldb
 
 #define LLDB_INVALID_PROCESS ((lldb::process_t)-1)
 #define LLDB_INVALID_HOST_THREAD ((lldb::thread_t)NULL)
+#define LLDB_INVALID_PIPE ((lldb::pipe_t)-1)
 
 namespace lldb {
 typedef uint64_t addr_t;
@@ -91,6 +94,6 @@ typedef int32_t break_id_t;
 typedef int32_t watch_id_t;
 typedef void *opaque_compiler_type_t;
 typedef uint64_t queue_id_t;
-}
+} // namespace lldb
 
 #endif // LLDB_lldb_types_h_
diff --git a/lldb/source/Host/posix/PipePosix.cpp b/lldb/source/Host/posix/PipePosix.cpp
index 1bf532cfd53..866a9897ee4 100644
--- a/lldb/source/Host/posix/PipePosix.cpp
+++ b/lldb/source/Host/posix/PipePosix.cpp
@@ -61,12 +61,13 @@ bool SetCloexecFlag(int fd) {
 std::chrono::time_point<std::chrono::steady_clock> Now() {
   return std::chrono::steady_clock::now();
 }
-}
+} // namespace
 
 PipePosix::PipePosix()
     : m_fds{PipePosix::kInvalidDescriptor, PipePosix::kInvalidDescriptor} {}
 
-PipePosix::PipePosix(int read_fd, int write_fd) : m_fds{read_fd, write_fd} {}
+PipePosix::PipePosix(lldb::pipe_t read, lldb::pipe_t write)
+    : m_fds{read, write} {}
 
 PipePosix::PipePosix(PipePosix &&pipe_posix)
     : PipeBase{std::move(pipe_posix)},
diff --git a/lldb/source/Host/windows/PipeWindows.cpp b/lldb/source/Host/windows/PipeWindows.cpp
index 1951c9ca193..57221a72899 100644
--- a/lldb/source/Host/windows/PipeWindows.cpp
+++ b/lldb/source/Host/windows/PipeWindows.cpp
@@ -25,14 +25,41 @@ using namespace lldb_private;
 
 namespace {
 std::atomic<uint32_t> g_pipe_serial(0);
+constexpr llvm::StringLiteral g_pipe_name_prefix = "\\\\.\\Pipe\\";
+} // namespace
+
+PipeWindows::PipeWindows()
+    : m_read(INVALID_HANDLE_VALUE), m_write(INVALID_HANDLE_VALUE),
+      m_read_fd(PipeWindows::kInvalidDescriptor),
+      m_write_fd(PipeWindows::kInvalidDescriptor) {
+  ZeroMemory(&m_read_overlapped, sizeof(m_read_overlapped));
+  ZeroMemory(&m_write_overlapped, sizeof(m_write_overlapped));
 }
 
-PipeWindows::PipeWindows() {
-  m_read = INVALID_HANDLE_VALUE;
-  m_write = INVALID_HANDLE_VALUE;
+PipeWindows::PipeWindows(pipe_t read, pipe_t write)
+    : m_read((HANDLE)read), m_write((HANDLE)write),
+      m_read_fd(PipeWindows::kInvalidDescriptor),
+      m_write_fd(PipeWindows::kInvalidDescriptor) {
+  assert(read != LLDB_INVALID_PIPE || write != LLDB_INVALID_PIPE);
+
+  // Don't risk in passing file descriptors and getting handles from them by
+  // _get_osfhandle since the retrieved handles are highly likely unrecognized
+  // in the current process and usually crashes the program.  Pass handles
+  // instead since the handle can be inherited.
+
+  if (read != LLDB_INVALID_PIPE) {
+    m_read_fd = _open_osfhandle((intptr_t)read, _O_RDONLY);
+    // Make sure the fd and native handle are consistent.
+    if (m_read_fd < 0)
+      m_read = INVALID_HANDLE_VALUE;
+  }
+
+  if (write != LLDB_INVALID_PIPE) {
+    m_write_fd = _open_osfhandle((intptr_t)write, _O_WRONLY);
+    if (m_write_fd < 0)
+      m_write = INVALID_HANDLE_VALUE;
+  }
 
-  m_read_fd = -1;
-  m_write_fd = -1;
   ZeroMemory(&m_read_overlapped, sizeof(m_read_overlapped));
   ZeroMemory(&m_write_overlapped, sizeof(m_write_overlapped));
 }
@@ -40,6 +67,24 @@ PipeWindows::PipeWindows() {
 PipeWindows::~PipeWindows() { Close(); }
 
 Status PipeWindows::CreateNew(bool child_process_inherit) {
+  // Create an anonymous pipe with the specified inheritance.
+  SECURITY_ATTRIBUTES sa{sizeof(SECURITY_ATTRIBUTES), 0,
+                         child_process_inherit ? TRUE : FALSE};
+  BOOL result = ::CreatePipe(&m_read, &m_write, &sa, 1024);
+  if (result == FALSE)
+    return Status(::GetLastError(), eErrorTypeWin32);
+
+  m_read_fd = _open_osfhandle((intptr_t)m_read, _O_RDONLY);
+  ZeroMemory(&m_read_overlapped, sizeof(m_read_overlapped));
+  m_read_overlapped.hEvent = ::CreateEventA(nullptr, TRUE, FALSE, nullptr);
+
+  m_write_fd = _open_osfhandle((intptr_t)m_write, _O_WRONLY);
+  ZeroMemory(&m_write_overlapped, sizeof(m_write_overlapped));
+
+  return Status();
+}
+
+Status PipeWindows::CreateNewNamed(bool child_process_inherit) {
   // Even for anonymous pipes, we open a named pipe.  This is because you
   // cannot get overlapped i/o on Windows without using a named pipe.  So we
   // synthesize a unique name.
@@ -60,7 +105,7 @@ Status PipeWindows::CreateNew(llvm::StringRef name,
   if (CanRead() || CanWrite())
     return Status(ERROR_ALREADY_EXISTS, eErrorTypeWin32);
 
-  std::string pipe_path = "\\\\.\\Pipe\\";
+  std::string pipe_path = g_pipe_name_prefix;
   pipe_path.append(name);
 
   // Always open for overlapped i/o.  We implement blocking manually in Read
@@ -75,7 +120,8 @@ Status PipeWindows::CreateNew(llvm::StringRef name,
   ZeroMemory(&m_read_overlapped, sizeof(m_read_overlapped));
   m_read_overlapped.hEvent = ::CreateEvent(nullptr, TRUE, FALSE, nullptr);
 
-  // Open the write end of the pipe.
+  // Open the write end of the pipe. Note that closing either the read or 
+  // write end of the pipe could directly close the pipe itself.
   Status result = OpenNamedPipe(name, child_process_inherit, false);
   if (!result.Success()) {
     CloseReadFileDescriptor();
@@ -111,7 +157,7 @@ Status PipeWindows::CreateWithUniqueName(llvm::StringRef prefix,
 
 Status PipeWindows::OpenAsReader(llvm::StringRef name,
                                  bool child_process_inherit) {
-  if (CanRead() || CanWrite())
+  if (CanRead())
     return Status(ERROR_ALREADY_EXISTS, eErrorTypeWin32);
 
   return OpenNamedPipe(name, child_process_inherit, true);
@@ -121,7 +167,7 @@ Status
 PipeWindows::OpenAsWriterWithTimeout(llvm::StringRef name,
                                      bool child_process_inherit,
                                      const std::chrono::microseconds &timeout) {
-  if (CanRead() || CanWrite())
+  if (CanWrite())
     return Status(ERROR_ALREADY_EXISTS, eErrorTypeWin32);
 
   return OpenNamedPipe(name, child_process_inherit, false);
@@ -137,7 +183,7 @@ Status PipeWindows::OpenNamedPipe(llvm::StringRef name,
   SECURITY_ATTRIBUTES attributes = {};
   attributes.bInheritHandle = child_process_inherit;
 
-  std::string pipe_path = "\\\\.\\Pipe\\";
+  std::string pipe_path = g_pipe_name_prefix;
   pipe_path.append(name);
 
   if (is_read) {
@@ -170,9 +216,9 @@ int PipeWindows::GetWriteFileDescriptor() const { return m_write_fd; }
 
 int PipeWindows::ReleaseReadFileDescriptor() {
   if (!CanRead())
-    return -1;
+    return PipeWindows::kInvalidDescriptor;
   int result = m_read_fd;
-  m_read_fd = -1;
+  m_read_fd = PipeWindows::kInvalidDescriptor;
   if (m_read_overlapped.hEvent)
     ::CloseHandle(m_read_overlapped.hEvent);
   m_read = INVALID_HANDLE_VALUE;
@@ -182,9 +228,9 @@ int PipeWindows::ReleaseReadFileDescriptor() {
 
 int PipeWindows::ReleaseWriteFileDescriptor() {
   if (!CanWrite())
-    return -1;
+    return PipeWindows::kInvalidDescriptor;
   int result = m_write_fd;
-  m_write_fd = -1;
+  m_write_fd = PipeWindows::kInvalidDescriptor;
   m_write = INVALID_HANDLE_VALUE;
   ZeroMemory(&m_write_overlapped, sizeof(m_write_overlapped));
   return result;
@@ -196,9 +242,10 @@ void PipeWindows::CloseReadFileDescriptor() {
 
   if (m_read_overlapped.hEvent)
     ::CloseHandle(m_read_overlapped.hEvent);
+
   _close(m_read_fd);
   m_read = INVALID_HANDLE_VALUE;
-  m_read_fd = -1;
+  m_read_fd = PipeWindows::kInvalidDescriptor;
   ZeroMemory(&m_read_overlapped, sizeof(m_read_overlapped));
 }
 
@@ -208,7 +255,7 @@ void PipeWindows::CloseWriteFileDescriptor() {
 
   _close(m_write_fd);
   m_write = INVALID_HANDLE_VALUE;
-  m_write_fd = -1;
+  m_write_fd = PipeWindows::kInvalidDescriptor;
   ZeroMemory(&m_write_overlapped, sizeof(m_write_overlapped));
 }
 
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
index b630fae87c3..72c1314a7c9 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp
@@ -1074,9 +1074,9 @@ Status GDBRemoteCommunication::StartDebugserverProcess(
                         __FUNCTION__, error.AsCString());
           return error;
         }
-        int write_fd = socket_pipe.GetWriteFileDescriptor();
+        pipe_t write = socket_pipe.GetWritePipe();
         debugserver_args.AppendArgument(llvm::StringRef("--pipe"));
-        debugserver_args.AppendArgument(llvm::to_string(write_fd));
+        debugserver_args.AppendArgument(llvm::to_string(write));
         launch_info.AppendCloseFileAction(socket_pipe.GetReadFileDescriptor());
 #endif
       } else {
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
index 60df07a0a3a..3521ddafbb1 100644
--- a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
+++ b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerPlatform.cpp
@@ -164,9 +164,6 @@ Status GDBRemoteCommunicationServerPlatform::LaunchGDBServer(
 GDBRemoteCommunication::PacketResult
 GDBRemoteCommunicationServerPlatform::Handle_qLaunchGDBServer(
     StringExtractorGDBRemote &packet) {
-#ifdef _WIN32
-  return SendErrorResponse(9);
-#else
   // Spawn a local debugserver as a platform so we can then attach or launch a
   // process...
 
@@ -217,10 +214,9 @@ GDBRemoteCommunicationServerPlatform::Handle_qLaunchGDBServer(
   PacketResult packet_result = SendPacketNoLock(response.GetString());
   if (packet_result != PacketResult::Success) {
     if (debugserver_pid != LLDB_INVALID_PROCESS_ID)
-      ::kill(debugserver_pid, SIGINT);
+      Host::Kill(debugserver_pid, SIGINT);
   }
   return packet_result;
-#endif
 }
 
 GDBRemoteCommunication::PacketResult
diff --git a/lldb/tools/lldb-server/lldb-gdbserver.cpp b/lldb/tools/lldb-server/lldb-gdbserver.cpp
index b30bea69941..062bbd0c3b6 100644
--- a/lldb/tools/lldb-server/lldb-gdbserver.cpp
+++ b/lldb/tools/lldb-server/lldb-gdbserver.cpp
@@ -218,20 +218,17 @@ Status writeSocketIdToPipe(const char *const named_pipe_path,
   return writeSocketIdToPipe(port_name_pipe, socket_id);
 }
 
-Status writeSocketIdToPipe(int unnamed_pipe_fd, const std::string &socket_id) {
-#if defined(_WIN32)
-  return Status("Unnamed pipes are not supported on Windows.");
-#else
-  Pipe port_pipe{Pipe::kInvalidDescriptor, unnamed_pipe_fd};
+Status writeSocketIdToPipe(lldb::pipe_t unnamed_pipe,
+                           const std::string &socket_id) {
+  Pipe port_pipe{LLDB_INVALID_PIPE, unnamed_pipe};
   return writeSocketIdToPipe(port_pipe, socket_id);
-#endif
 }
 
 void ConnectToRemote(MainLoop &mainloop,
                      GDBRemoteCommunicationServerLLGS &gdb_server,
                      bool reverse_connect, const char *const host_and_port,
                      const char *const progname, const char *const subcommand,
-                     const char *const named_pipe_path, int unnamed_pipe_fd,
+                     const char *const named_pipe_path, pipe_t unnamed_pipe,
                      int connection_fd) {
   Status error;
 
@@ -331,8 +328,8 @@ void ConnectToRemote(MainLoop &mainloop,
         }
         // If we have an unnamed pipe to write the socket id back to, do that
         // now.
-        else if (unnamed_pipe_fd >= 0) {
-          error = writeSocketIdToPipe(unnamed_pipe_fd, socket_id);
+        else if (unnamed_pipe != LLDB_INVALID_PIPE) {
+          error = writeSocketIdToPipe(unnamed_pipe, socket_id);
           if (error.Fail())
             fprintf(stderr, "failed to write to the unnamed pipe: %s\n",
                     error.AsCString());
@@ -384,7 +381,7 @@ int main_gdbserver(int argc, char *argv[]) {
   std::string log_file;
   StringRef
       log_channels; // e.g. "lldb process threads:gdb-remote default:linux all"
-  int unnamed_pipe_fd = -1;
+  lldb::pipe_t unnamed_pipe = LLDB_INVALID_PIPE;
   bool reverse_connect = false;
   int connection_fd = -1;
 
@@ -425,7 +422,7 @@ int main_gdbserver(int argc, char *argv[]) {
 
     case 'U': // unnamed pipe
       if (optarg && optarg[0])
-        unnamed_pipe_fd = StringConvert::ToUInt32(optarg, -1);
+        unnamed_pipe = (pipe_t)StringConvert::ToUInt64(optarg, -1);
       break;
 
     case 'r':
@@ -528,8 +525,8 @@ int main_gdbserver(int argc, char *argv[]) {
   printf("%s-%s", LLGS_PROGRAM_NAME, LLGS_VERSION_STR);
 
   ConnectToRemote(mainloop, gdb_server, reverse_connect, host_and_port,
-                  progname, subcommand, named_pipe_path.c_str(),
-                  unnamed_pipe_fd, connection_fd);
+                  progname, subcommand, named_pipe_path.c_str(), 
+                  unnamed_pipe, connection_fd);
 
   if (!gdb_server.IsConnected()) {
     fprintf(stderr, "no connection information provided, unable to run\n");