Fix deadlock in Python one-line execution.

Python one-line execution was using ConnectionFileDescriptor to do
a non-blocking read against a pipe.  This won't work on Windows,
as CFD is implemented using select(), and select() only works with
sockets on Windows.

The solution is to use ConnectionGenericFile on Windows, which uses
the native API to do overlapped I/O on the pipe.  This in turn
requires re-implementing Host::Pipe on Windows using native OS
handles instead of the more portable _pipe CRT api.

Reviewed by: Greg Clayton
Differential Revision: http://reviews.llvm.org/D5679

llvm-svn: 219339

8 files changed, 431 insertions, 105 deletions

diff --git a/lldb/include/lldb/Host/Pipe.h b/lldb/include/lldb/Host/Pipe.h
index b36c85cfbe8..b748788c5ee 100644
--- a/lldb/include/lldb/Host/Pipe.h
+++ b/lldb/include/lldb/Host/Pipe.h
@@ -7,77 +7,13 @@
 //
 //===----------------------------------------------------------------------===//
 
-#ifndef liblldb_Pipe_h_
-#define liblldb_Pipe_h_
-#if defined(__cplusplus)
+#ifndef liblldb_Host_Pipe_h_
+#define liblldb_Host_Pipe_h_
 
-#include <stdarg.h>
-#include <stdio.h>
-#include <sys/types.h>
+#if defined(_WIN32)
+#include "lldb/Host/windows/PipeWindows.h"
+#else
+#include "lldb/Host/windows/PipePosix.h"
+#endif
 
-#include "lldb/lldb-private.h"
-
-namespace lldb_private {
-
-//----------------------------------------------------------------------
-/// @class Pipe Pipe.h "lldb/Host/Pipe.h"
-/// @brief A class that abtracts unix style pipes.
-///
-/// A class that abstracts the LLDB core from host pipe functionality.
-//----------------------------------------------------------------------
-class Pipe
-{
-public:
-    static int kInvalidDescriptor;
-    
-    Pipe();
-    
-    ~Pipe();
-    
-    bool
-    Open();
-
-    bool
-    IsValid() const;
-    
-    bool
-    ReadDescriptorIsValid() const;
-
-    bool
-    WriteDescriptorIsValid() const;
-
-    int
-    GetReadFileDescriptor() const;
-    
-    int
-    GetWriteFileDescriptor() const;
-    
-    // Close both descriptors
-    void
-    Close();
-
-    bool
-    CloseReadFileDescriptor();
-    
-    bool
-    CloseWriteFileDescriptor();
-
-    int
-    ReleaseReadFileDescriptor();
-    
-    int
-    ReleaseWriteFileDescriptor();
-
-    size_t
-    Read (void *buf, size_t size);
-
-    size_t
-    Write (const void *buf, size_t size);
-private:
-    int m_fds[2];
-};
-
-} // namespace lldb_private
-
-#endif  // #if defined(__cplusplus)
-#endif  // liblldb_Pipe_h_
+#endif // liblldb_Host_Pipe_h_
diff --git a/lldb/include/lldb/Host/posix/PipePosix.h b/lldb/include/lldb/Host/posix/PipePosix.h
new file mode 100644
index 00000000000..0102124d947
--- /dev/null
+++ b/lldb/include/lldb/Host/posix/PipePosix.h
@@ -0,0 +1,84 @@
+//===-- PipePosix.h ---------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef liblldb_Host_posix_PipePosix_h_
+#define liblldb_Host_posix_PipePosix_h_
+#if defined(__cplusplus)
+
+#include <stdarg.h>
+#include <stdio.h>
+#include <sys/types.h>
+
+#include "lldb/lldb-private.h"
+
+namespace lldb_private {
+
+//----------------------------------------------------------------------
+/// @class Pipe Pipe.h "lldb/Host/posix/PipePosix.h"
+/// @brief A posix-based implementation of Pipe, a class that abtracts
+///        unix style pipes.
+///
+/// A class that abstracts the LLDB core from host pipe functionality.
+//----------------------------------------------------------------------
+class Pipe
+{
+public:
+    static int kInvalidDescriptor;
+    
+    Pipe();
+    
+    ~Pipe();
+    
+    bool
+    Open();
+
+    bool
+    IsValid() const;
+    
+    bool
+    ReadDescriptorIsValid() const;
+
+    bool
+    WriteDescriptorIsValid() const;
+
+    int
+    GetReadFileDescriptor() const;
+    
+    int
+    GetWriteFileDescriptor() const;
+    
+    // Close both descriptors
+    void
+    Close();
+
+    bool
+    CloseReadFileDescriptor();
+    
+    bool
+    CloseWriteFileDescriptor();
+
+    int
+    ReleaseReadFileDescriptor();
+    
+    int
+    ReleaseWriteFileDescriptor();
+
+    size_t
+    Read (void *buf, size_t size);
+
+    size_t
+    Write (const void *buf, size_t size);
+private:
+    int m_fds[2];
+};
+
+} // namespace lldb_private
+
+#endif  // #if defined(__cplusplus)
+#endif  // liblldb_Host_posix_PipePosix_h_
diff --git a/lldb/include/lldb/Host/windows/PipeWindows.h b/lldb/include/lldb/Host/windows/PipeWindows.h
new file mode 100644
index 00000000000..1a22a3ca9c6
--- /dev/null
+++ b/lldb/include/lldb/Host/windows/PipeWindows.h
@@ -0,0 +1,78 @@
+//===-- PipePosix.h ---------------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef liblldb_Host_windows_PipeWindows_h_
+#define liblldb_Host_windows_PipeWindows_h_
+
+#include "lldb/Host/windows/windows.h"
+
+namespace lldb_private
+{
+
+//----------------------------------------------------------------------
+/// @class Pipe PipeWindows.h "lldb/Host/windows/PipeWindows.h"
+/// @brief A windows-based implementation of Pipe, a class that abtracts
+///        unix style pipes.
+///
+/// A class that abstracts the LLDB core from host pipe functionality.
+//----------------------------------------------------------------------
+class Pipe
+{
+  public:
+    Pipe();
+
+    ~Pipe();
+
+    bool Open(bool read_overlapped = false, bool write_overlapped = false);
+
+    bool IsValid() const;
+
+    bool ReadDescriptorIsValid() const;
+
+    bool WriteDescriptorIsValid() const;
+
+    int GetReadFileDescriptor() const;
+
+    int GetWriteFileDescriptor() const;
+
+    // Close both descriptors
+    void Close();
+
+    bool CloseReadFileDescriptor();
+
+    bool CloseWriteFileDescriptor();
+
+    int ReleaseReadFileDescriptor();
+
+    int ReleaseWriteFileDescriptor();
+
+    HANDLE
+    GetReadNativeHandle();
+
+    HANDLE
+    GetWriteNativeHandle();
+
+    size_t Read(void *buf, size_t size);
+
+    size_t Write(const void *buf, size_t size);
+
+  private:
+    HANDLE m_read;
+    HANDLE m_write;
+
+    int m_read_fd;
+    int m_write_fd;
+
+    OVERLAPPED *m_read_overlapped;
+    OVERLAPPED *m_write_overlapped;
+};
+
+} // namespace lldb_private
+
+#endif // liblldb_Host_posix_PipePosix_h_
diff --git a/lldb/source/Host/CMakeLists.txt b/lldb/source/Host/CMakeLists.txt
index a2adb8a1ea4..73da2a890af 100644
--- a/lldb/source/Host/CMakeLists.txt
+++ b/lldb/source/Host/CMakeLists.txt
@@ -21,7 +21,6 @@ add_host_subdirectory(common
   common/NativeProcessProtocol.cpp
   common/NativeThreadProtocol.cpp
   common/OptionParser.cpp
-  common/Pipe.cpp
   common/ProcessRunLock.cpp
   common/Socket.cpp
   common/SocketAddress.cpp
@@ -48,6 +47,7 @@ if (CMAKE_SYSTEM_NAME MATCHES "Windows")
     windows/HostProcessWindows.cpp
     windows/HostThreadWindows.cpp
     windows/Mutex.cpp
+    windows/PipeWindows.cpp
     windows/ProcessRunLock.cpp
     windows/ThisThread.cpp
     windows/Windows.cpp
@@ -58,6 +58,7 @@ else()
     posix/HostInfoPosix.cpp
     posix/HostProcessPosix.cpp
     posix/HostThreadPosix.cpp
+    posix/PipePosix.cpp
     )
 
   if (CMAKE_SYSTEM_NAME MATCHES "Darwin")
diff --git a/lldb/source/Host/common/Pipe.cpp b/lldb/source/Host/posix/PipePosix.cpp
index 4db0e32c93b..ebc2033268e 100644
--- a/lldb/source/Host/common/Pipe.cpp
+++ b/lldb/source/Host/posix/PipePosix.cpp
@@ -1,4 +1,4 @@
-//===-- Pipe.cpp ------------------------------------------------*- C++ -*-===//
+//===-- PipePosix.cpp -------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
@@ -7,14 +7,9 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "lldb/Host/Pipe.h"
+#include "lldb/Host/posix/PipePosix.h"
 
-#if defined(_WIN32)
-#include <io.h>
-#include <fcntl.h>
-#else
 #include <unistd.h>
-#endif
 
 using namespace lldb_private;
 
@@ -39,13 +34,9 @@ Pipe::Open()
     if (IsValid())
         return true;
 
-#ifdef _WIN32
-    if (::_pipe(m_fds, 256, O_BINARY) == 0)
-        return true;
-#else
     if (::pipe(m_fds) == 0)
         return true;
-#endif
+
     m_fds[READ] = Pipe::kInvalidDescriptor;
     m_fds[WRITE] = Pipe::kInvalidDescriptor;
     return false;
@@ -110,11 +101,7 @@ Pipe::CloseReadFileDescriptor()
     if (ReadDescriptorIsValid())
     {
         int err;
-#ifdef _WIN32
-        err = _close(m_fds[READ]);
-#else
         err = close(m_fds[READ]);
-#endif
         m_fds[READ] = Pipe::kInvalidDescriptor;
         return err == 0;
     }
@@ -127,11 +114,7 @@ Pipe::CloseWriteFileDescriptor()
     if (WriteDescriptorIsValid())
     {
         int err;
-#ifdef _WIN32
-        err = _close(m_fds[WRITE]);
-#else
         err = close(m_fds[WRITE]);
-#endif
         m_fds[WRITE] = Pipe::kInvalidDescriptor;
         return err == 0;
     }
@@ -145,11 +128,7 @@ Pipe::Read (void *buf, size_t num_bytes)
     if (ReadDescriptorIsValid())
     {
         const int fd = GetReadFileDescriptor();
-#ifdef _WIN32
-        return _read (fd, (char *)buf, num_bytes);
-#else
         return read (fd, buf, num_bytes);
-#endif
     }
     return 0; // Return 0 since errno won't be set if we didn't call read
 }
@@ -160,12 +139,7 @@ Pipe::Write (const void *buf, size_t num_bytes)
     if (WriteDescriptorIsValid())
     {
         const int fd = GetWriteFileDescriptor();
-#ifdef _WIN32
-        return _write (fd, (char *)buf, num_bytes);
-#else
         return write (fd, buf, num_bytes);
-#endif
     }
     return 0; // Return 0 since errno won't be set if we didn't call write
 }
-
diff --git a/lldb/source/Host/windows/ConnectionGenericFileWindows.cpp b/lldb/source/Host/windows/ConnectionGenericFileWindows.cpp
index 312cef62d65..93d37945992 100644
--- a/lldb/source/Host/windows/ConnectionGenericFileWindows.cpp
+++ b/lldb/source/Host/windows/ConnectionGenericFileWindows.cpp
@@ -241,9 +241,16 @@ ConnectionGenericFile::Read(void *dst, size_t dst_len, uint32_t timeout_usec, ll
 
         goto finish;
     }
-
-    // An unknown error occured.  Fail out.
-    return_info.Set(0, eConnectionStatusError, ::GetLastError());
+    else if (::GetLastError() == ERROR_BROKEN_PIPE)
+    {
+        // The write end of a pipe was closed.  This is equivalent to EOF.
+        return_info.Set(0, eConnectionStatusEndOfFile, 0);
+    }
+    else
+    {
+        // An unknown error occured.  Fail out.
+        return_info.Set(0, eConnectionStatusError, ::GetLastError());
+    }
     goto finish;
 
 finish:
diff --git a/lldb/source/Host/windows/PipeWindows.cpp b/lldb/source/Host/windows/PipeWindows.cpp
new file mode 100644
index 00000000000..572be878409
--- /dev/null
+++ b/lldb/source/Host/windows/PipeWindows.cpp
@@ -0,0 +1,231 @@
+//===-- PipeWindows.cpp -----------------------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "lldb/Host/windows/PipeWindows.h"
+
+#include "llvm/Support/raw_ostream.h"
+
+#include <fcntl.h>
+#include <io.h>
+
+#include <atomic>
+#include <string>
+
+using namespace lldb_private;
+
+namespace
+{
+std::atomic<uint32_t> g_pipe_serial(0);
+}
+
+Pipe::Pipe()
+{
+    m_read = INVALID_HANDLE_VALUE;
+    m_write = INVALID_HANDLE_VALUE;
+
+    m_read_fd = -1;
+    m_write_fd = -1;
+
+    m_read_overlapped = nullptr;
+    m_write_overlapped = nullptr;
+}
+
+Pipe::~Pipe()
+{
+    Close();
+}
+
+bool
+Pipe::Open(bool read_overlapped, bool write_overlapped)
+{
+    if (IsValid())
+        return true;
+
+    uint32_t serial = g_pipe_serial.fetch_add(1);
+    std::string pipe_name;
+    llvm::raw_string_ostream pipe_name_stream(pipe_name);
+    pipe_name_stream << "\\\\.\\Pipe\\lldb.pipe." << ::GetCurrentProcessId() << "." << serial;
+    pipe_name_stream.flush();
+
+    DWORD read_mode = 0;
+    DWORD write_mode = 0;
+    if (read_overlapped)
+        read_mode |= FILE_FLAG_OVERLAPPED;
+    if (write_overlapped)
+        write_mode |= FILE_FLAG_OVERLAPPED;
+    m_read =
+        ::CreateNamedPipe(pipe_name.c_str(), PIPE_ACCESS_INBOUND | read_mode, PIPE_TYPE_BYTE | PIPE_WAIT, 1, 1024, 1024, 120 * 1000, NULL);
+    if (INVALID_HANDLE_VALUE == m_read)
+        return false;
+    m_write = ::CreateFile(pipe_name.c_str(), GENERIC_WRITE, 0, NULL, OPEN_EXISTING, write_mode, NULL);
+    if (INVALID_HANDLE_VALUE == m_write)
+    {
+        ::CloseHandle(m_read);
+        m_read = INVALID_HANDLE_VALUE;
+        return false;
+    }
+
+    m_read_fd = _open_osfhandle((intptr_t)m_read, _O_RDONLY);
+    m_write_fd = _open_osfhandle((intptr_t)m_write, _O_WRONLY);
+
+    if (read_overlapped)
+    {
+        m_read_overlapped = new OVERLAPPED;
+        ZeroMemory(m_read_overlapped, sizeof(OVERLAPPED));
+    }
+    if (write_overlapped)
+    {
+        m_write_overlapped = new OVERLAPPED;
+        ZeroMemory(m_write_overlapped, sizeof(OVERLAPPED));
+    }
+    return true;
+}
+
+int
+Pipe::GetReadFileDescriptor() const
+{
+    return m_read_fd;
+}
+
+int
+Pipe::GetWriteFileDescriptor() const
+{
+    return m_write_fd;
+}
+
+int
+Pipe::ReleaseReadFileDescriptor()
+{
+    int result = m_read_fd;
+    m_read_fd = -1;
+    m_read = INVALID_HANDLE_VALUE;
+    if (m_read_overlapped)
+    {
+        delete m_read_overlapped;
+        m_read_overlapped = nullptr;
+    }
+    return result;
+}
+
+int
+Pipe::ReleaseWriteFileDescriptor()
+{
+    int result = m_write_fd;
+    m_write_fd = -1;
+    m_write = INVALID_HANDLE_VALUE;
+    if (m_write_overlapped)
+    {
+        delete m_write_overlapped;
+        m_write_overlapped = nullptr;
+    }
+    return result;
+}
+
+void
+Pipe::Close()
+{
+    CloseReadFileDescriptor();
+    CloseWriteFileDescriptor();
+}
+
+bool
+Pipe::ReadDescriptorIsValid() const
+{
+    return m_read_fd != -1;
+}
+
+bool
+Pipe::WriteDescriptorIsValid() const
+{
+    return m_write_fd != -1;
+}
+
+bool
+Pipe::IsValid() const
+{
+    return ReadDescriptorIsValid() && WriteDescriptorIsValid();
+}
+
+bool
+Pipe::CloseReadFileDescriptor()
+{
+    if (ReadDescriptorIsValid())
+    {
+        int err;
+        err = _close(m_read_fd);
+        m_read_fd = -1;
+        m_read = INVALID_HANDLE_VALUE;
+        if (m_read_overlapped)
+        {
+            delete m_read_overlapped;
+            m_read_overlapped = nullptr;
+        }
+        return err == 0;
+    }
+    return true;
+}
+
+bool
+Pipe::CloseWriteFileDescriptor()
+{
+    if (WriteDescriptorIsValid())
+    {
+        int err;
+        err = _close(m_write_fd);
+        m_write_fd = -1;
+        m_write = INVALID_HANDLE_VALUE;
+        if (m_write_overlapped)
+        {
+            delete m_write_overlapped;
+            m_write_overlapped = nullptr;
+        }
+        return err == 0;
+    }
+    return true;
+}
+
+HANDLE
+Pipe::GetReadNativeHandle()
+{
+    return m_read;
+}
+
+HANDLE
+Pipe::GetWriteNativeHandle()
+{
+    return m_write;
+}
+
+size_t
+Pipe::Read(void *buf, size_t num_bytes)
+{
+    if (ReadDescriptorIsValid())
+    {
+        DWORD bytes_read = 0;
+        ::ReadFile(m_read, buf, num_bytes, &bytes_read, m_read_overlapped);
+        if (m_read_overlapped)
+            GetOverlappedResult(m_read, m_read_overlapped, &bytes_read, TRUE);
+        return bytes_read;
+    }
+    return 0; // Return 0 since errno won't be set if we didn't call read
+}
+
+size_t
+Pipe::Write(const void *buf, size_t num_bytes)
+{
+    if (WriteDescriptorIsValid())
+    {
+        DWORD bytes_written = 0;
+        ::WriteFile(m_write, buf, num_bytes, &bytes_written, m_read_overlapped);
+        if (m_write_overlapped)
+            GetOverlappedResult(m_write, m_write_overlapped, &bytes_written, TRUE);
+        return bytes_written;
+    }
+    return 0; // Return 0 since errno won't be set if we didn't call write
+}
diff --git a/lldb/source/Interpreter/ScriptInterpreterPython.cpp b/lldb/source/Interpreter/ScriptInterpreterPython.cpp
index 987a168079a..8e03e7b631e 100644
--- a/lldb/source/Interpreter/ScriptInterpreterPython.cpp
+++ b/lldb/source/Interpreter/ScriptInterpreterPython.cpp
@@ -39,6 +39,10 @@
 #include "lldb/Target/Thread.h"
 #include "lldb/Target/ThreadPlan.h"
 
+#if defined(_WIN32)
+#include "lldb/Host/windows/ConnectionGenericFileWindows.h"
+#endif
+
 using namespace lldb;
 using namespace lldb_private;
 
@@ -598,9 +602,20 @@ ScriptInterpreterPython::ExecuteOneLine (const char *command, CommandReturnObjec
                 // Set output to a temporary file so we can forward the results on to the result object
                 
                 Pipe pipe;
+#if defined(_WIN32)
+                // By default Windows does not create a pipe object that can be used for a non-blocking read.
+                // We must explicitly request it.  Furthermore, we can't use an fd for non-blocking read
+                // operations, and must use the native os HANDLE.
+                if (pipe.Open(true, false))
+                {
+                    lldb::file_t read_file = pipe.GetReadNativeHandle();
+                    pipe.ReleaseReadFileDescriptor();
+                    std::unique_ptr<ConnectionGenericFile> conn_ap(new ConnectionGenericFile(read_file, true));
+#else
                 if (pipe.Open())
                 {
                     std::unique_ptr<ConnectionFileDescriptor> conn_ap(new ConnectionFileDescriptor(pipe.ReleaseReadFileDescriptor(), true));
+#endif
                     if (conn_ap->IsConnected())
                     {
                         output_comm.SetConnection(conn_ap.release());