Add platform::POpen.

Fixes ClangInternalState::differentContent showing no differences when there
actually are, just less than 128 characters in length.

Invoking diff on Windows with --ignore-matching-lines arguments will fail because the command string is too long. Using Windows APIs to launch the process succeeds.

Signed-off-by: Vassil Vassilev <vvasilev@cern.ch>

include/cling/Interpreter/ClangInternalState.h

@@ -72,12 +72,12 @@ namespace cling {
     ///\brief Runs diff on two files.
     ///\param[in] file1 - A file to diff
     ///\param[in] file2 - A file to diff
-    ///\param[in] differences - The differences if any between file1 and file2
+    ///\param[in] type - The type/name of the differences to print.
     ///\param[in] ignores - A list of differences to ignore.
     ///\returns true if there is difference in the contents.
     ///
     bool differentContent(const std::string& file1, const std::string& file2,
-                          std::string& differences,
+                          const char* type = nullptr,
                    const llvm::SmallVectorImpl<const char*>* ignores = 0) const;
 
     static void printLookupTables(llvm::raw_ostream& Out, clang::ASTContext& C);

include/cling/Utils/Platform.h

@@ -61,6 +61,17 @@ namespace platform {
   ///
   bool IsMemoryValid(const void *P);
 
+  ///\brief Invoke a command and read it's output.
+  ///
+  /// \param [in] Cmd - Command and arguments to invoke.
+  /// \param [out] Buf - Buffer to write output to.
+  /// \param [in] StdErrToStdOut - Redirect stderr to stdout.
+  ///
+  /// \returns whether any output was written to Buf
+  ///
+  bool Popen(const std::string& Cmd, llvm::SmallVectorImpl<char>& Buf,
+             bool StdErrToStdOut = false);
+
 #if defined(LLVM_ON_UNIX)
 
 #if defined(__APPLE__)

lib/Interpreter/ClangInternalState.cpp

@@ -8,6 +8,7 @@
 //------------------------------------------------------------------------------
 
 #include "cling/Interpreter/ClangInternalState.h"
+#include "cling/Utils/Platform.h"
 
 #include "clang/AST/ASTContext.h"
 #include "clang/Lex/Preprocessor.h"
@@ -29,11 +30,6 @@
 #include <string>
 #include <time.h>
 
-#ifdef WIN32
-#define popen _popen
-#define pclose _pclose
-#endif
-
 using namespace clang;
 
 namespace cling {
@@ -42,7 +38,12 @@ namespace cling {
                                          llvm::Module* M, CodeGenerator* CG,
                                          const std::string& name)
     : m_ASTContext(AC), m_Preprocessor(PP), m_CodeGen(CG), m_Module(M),
-      m_DiffCommand("diff -u --text "), m_Name(name), m_DiffPair(nullptr) {
+#if defined(LLVM_ON_WIN32)
+      m_DiffCommand("diff.exe -u --text "),
+#else
+      m_DiffCommand("diff -u --text "),
+#endif
+      m_Name(name), m_DiffPair(nullptr) {
     store();
   }
 
@@ -153,23 +154,14 @@ namespace cling {
 
     builtinNames.push_back(".*__builtin.*");
 
-    if (differentContent(m_LookupTablesFile, m_DiffPair->m_LookupTablesFile,
-                         differences, &builtinNames)) {
-      llvm::errs() << "Differences in the lookup tables\n";
-      llvm::errs() << differences << "\n";
-      differences = "";
-    }
-    if (differentContent(m_IncludedFilesFile, m_DiffPair->m_IncludedFilesFile,
-                         differences)) {
-      llvm::errs() << "Differences in the included files\n";
-      llvm::errs() << differences << "\n";
-      differences = "";
-    }
-    if (differentContent(m_ASTFile, m_DiffPair->m_ASTFile, differences)) {
-      llvm::errs() << "Differences in the AST \n";
-      llvm::errs() << differences << "\n";
-      differences = "";
-    }
+    differentContent(m_LookupTablesFile, m_DiffPair->m_LookupTablesFile,
+                     "lookup tables", &builtinNames);
+
+    differentContent(m_IncludedFilesFile, m_DiffPair->m_IncludedFilesFile,
+                     "included files");
+
+    differentContent(m_ASTFile, m_DiffPair->m_ASTFile, "AST");
+
     if (m_Module) {
       assert(m_CodeGen && "Must have CodeGen set");
       // We want to skip the intrinsics
@@ -179,24 +171,19 @@ namespace cling {
         if (I->isIntrinsic())
           builtinNames.push_back(I->getName().data());
 
-      if (differentContent(m_LLVMModuleFile, m_DiffPair->m_LLVMModuleFile,
-                           differences, &builtinNames)) {
-        llvm::errs() << "Differences in the llvm Module \n";
-        llvm::errs() << differences << "\n";
-        differences = "";
-      }
-    }
-    if (differentContent(m_MacrosFile, m_DiffPair->m_MacrosFile, differences)){
-      llvm::errs() << "Differences in the Macro Definitions \n";
-      llvm::errs() << differences << "\n";
-      differences = "";
+      differentContent(m_LLVMModuleFile, m_DiffPair->m_LLVMModuleFile,
+                       "llvm Module", &builtinNames);
     }
+
+    differentContent(m_MacrosFile, m_DiffPair->m_MacrosFile,
+                     "Macro Definitions");
   }
 
   bool ClangInternalState::differentContent(const std::string& file1,
                                             const std::string& file2,
-                                            std::string& differences,
-                const llvm::SmallVectorImpl<const char*>* ignores/*=0*/) const {
+                                            const char* type,
+                const llvm::SmallVectorImpl<const char*>* ignores/*=0*/) const {    
+
     std::string diffCall = m_DiffCommand;
     if (ignores) {
       for (size_t i = 0, e = ignores->size(); i < e; ++i) {
@@ -205,22 +192,23 @@ namespace cling {
         diffCall += ".*\"";
       }
     }
+    diffCall += " ";
+    diffCall += file1;
+    diffCall += " ";
+    diffCall += file2;
 
-    FILE* pipe = popen((diffCall + " " + file1 + " " + file2).c_str() , "r");
-    assert(pipe && "Error creating the pipe");
-    assert(differences.empty() && "Must be empty");
+    llvm::SmallString<1024> Difs;
+    platform::Popen(diffCall, Difs);
 
-    char buffer[128];
-    while(!feof(pipe)) {
-      if(fgets(buffer, 128, pipe) != NULL)
-        differences += buffer;
+    if (Difs.empty())
+      return false;
+
+    if (type) {
+      llvm::errs() << diffCall << "\n";
+      llvm::errs() << "Differences in the " << type << ":\n";
+      llvm::errs() << Difs << "\n";
     }
-    pclose(pipe);
-
-    if (!differences.empty())
-      llvm::errs() << diffCall << " " << file1 << " " << file2 << "\n";
-
-    return !differences.empty();
+    return true;
   }
 
   class DumpLookupTables : public RecursiveASTVisitor<DumpLookupTables> {

lib/Utils/PlatformPosix.cpp

@@ -12,6 +12,7 @@
 #if defined(LLVM_ON_UNIX)
 
 #include "cling/Utils/Paths.h"
+#include "llvm/ADT/SmallString.h"
 
 #include <string>
 #include <dlfcn.h>
@@ -84,6 +85,25 @@ std::string NormalizePath(const std::string& Path) {
   return std::string();
 }
 
+bool Popen(const std::string& Cmd, llvm::SmallVectorImpl<char>& Buf, bool RdE) {
+  if (FILE *PF = ::popen(RdE ? (Cmd + " 2>&1").c_str() : Cmd.c_str(), "r")) {
+    Buf.resize(0);
+    const size_t Chunk = Buf.capacity_in_bytes();
+    while (true) {
+      const size_t Len = Buf.size();
+      Buf.resize(Len + Chunk);
+      const size_t R = ::fread(&Buf[Len], sizeof(char), Chunk, PF);
+      if (R < Chunk) {
+        Buf.resize(Len + R);
+        break;
+      }
+    }
+    ::pclose(PF);
+    return !Buf.empty();
+  }
+  return false;
+}
+
 bool GetSystemLibraryPaths(llvm::SmallVectorImpl<std::string>& Paths) {
 #if defined(__APPLE__) || defined(__CYGWIN__)
   Paths.push_back("/usr/local/lib/");
@@ -98,13 +118,9 @@ bool GetSystemLibraryPaths(llvm::SmallVectorImpl<std::string>& Paths) {
   Paths.push_back("/lib64/");
  #endif
 #else
-  std::string Result;
-  if (FILE* F = ::popen("LD_DEBUG=libs LD_PRELOAD=DOESNOTEXIST ls 2>&1", "r")) {
-    char Buf[1024];
-    while (::fgets(&Buf[0], sizeof(Buf), F))
-      Result += Buf;
-    ::pclose(F);
-  }
+  llvm::SmallString<1024> Buf;
+  platform::Popen("LD_DEBUG=libs LD_PRELOAD=DOESNOTEXIST ls", Buf, true);
+  const llvm::StringRef Result = Buf.str();
 
   const std::size_t NPos = std::string::npos;
   const std::size_t LD = Result.find("(LD_LIBRARY_PATH)");

lib/Utils/PlatformWin.cpp

@@ -587,6 +587,104 @@ bool GetSystemLibraryPaths(llvm::SmallVectorImpl<std::string>& Paths) {
   return true;
 }
 
+static void CloseHandle(HANDLE H) {
+  if (::CloseHandle(H) == 0)
+    ReportLastError("CloseHandle");
+}
+
+bool Popen(const std::string& Cmd, llvm::SmallVectorImpl<char>& Buf, bool RdE) {
+  Buf.resize(0);
+
+  SECURITY_ATTRIBUTES saAttr;
+  saAttr.nLength = sizeof(SECURITY_ATTRIBUTES);
+  saAttr.bInheritHandle = TRUE;
+  saAttr.lpSecurityDescriptor = NULL;
+
+  HANDLE Process = ::GetCurrentProcess();
+  HANDLE hOutputReadTmp, hOutputRead, hOutputWrite, hErrorWrite;
+
+  if (::CreatePipe(&hOutputReadTmp, &hOutputWrite, &saAttr, 0) == 0)
+    return false;
+  if (RdE) {
+    if (::DuplicateHandle(Process, hOutputWrite, Process, &hErrorWrite, 0, TRUE,
+                          DUPLICATE_SAME_ACCESS) == 0) {
+      ReportLastError("DuplicateHandle");
+      ::CloseHandle(hOutputReadTmp);
+      ::CloseHandle(hOutputWrite);
+      return false;
+    }
+  }
+
+  // Create new output read handle. Set the Properties to FALSE, otherwise the
+  // child inherits the properties and, as a result, non-closeable handles to
+  // the pipes are created.
+  if (::DuplicateHandle(Process, hOutputReadTmp, Process, &hOutputRead, 0,
+                        FALSE, DUPLICATE_SAME_ACCESS) == 0) {
+    ReportLastError("DuplicateHandle");
+    ::CloseHandle(hOutputReadTmp);
+    ::CloseHandle(hOutputWrite);
+    if (RdE)
+      ::CloseHandle(hErrorWrite);
+    return false;
+  }
+
+  // Close inheritable copies of the handles you do not want to be inherited.
+  CloseHandle(hOutputReadTmp);
+
+  STARTUPINFOA si;
+  ZeroMemory(&si, sizeof(si));
+  si.cb = sizeof(si);
+  si.dwFlags = STARTF_USESTDHANDLES;
+  si.hStdOutput = hOutputWrite;
+  if (RdE)
+    si.hStdError = hErrorWrite;
+
+  PROCESS_INFORMATION pi;
+  ZeroMemory(&pi, sizeof(pi));
+
+  // https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425%28v=vs.85%29.aspx
+  // CreateProcessW can write back to second arguement, CreateProcessA not
+  BOOL Result = ::CreateProcessA(NULL, (LPSTR)Cmd.c_str(), NULL, NULL, TRUE, 0,
+                                 NULL, NULL, &si, &pi);
+  DWORD Err = ::GetLastError();
+
+  // Close pipe handles (do not continue to modify the parent) to make sure
+  // that no handles to the write end of the output pipe are maintained in this
+  // process or else the pipe will not close when the child process exits and
+  // the ReadFile will hang.
+  CloseHandle(hOutputWrite);
+  if (RdE)
+    CloseHandle(hErrorWrite);
+  
+  if (Result != 0) {
+    DWORD dwRead;
+    const size_t Chunk = Buf.capacity_in_bytes();
+    while (true) {
+      const size_t Len = Buf.size();
+      Buf.resize(Len + Chunk);
+      Result = ::ReadFile(hOutputRead, &Buf[Len], Chunk, &dwRead, NULL);
+      if (!Result || !dwRead) {
+        Err = ::GetLastError();
+        if (Err != ERROR_BROKEN_PIPE)
+          ReportError(Err, "ReadFile");
+        Buf.resize(Len);
+        break;
+      }
+      if (dwRead < Chunk)
+        Buf.resize(Len + dwRead);
+    }
+
+    // Close process and thread handles.
+    CloseHandle(pi.hProcess);
+    CloseHandle(pi.hThread);
+  } else
+    ReportError(Err, "CreateProcess");
+
+  CloseHandle(hOutputRead);
+
+  return !Buf.empty();
+}
+
 } // namespace platform
 } // namespace utils
 } // namespace cling