Fix the Unit Tests on Windows [Fixed Mismatch]

- Fixing Symlink handling on Windows
- The windows process handling I did was broken in a few places
- Handling Windows paths
- Working around a few Clang/MSVC inconsistencies

Author: Jason Mittertreiner

docs/buildsystem.rst

@@ -437,7 +437,7 @@ Symlink Tool
 This tool is used to create a symbolic link at a particular location, with
 appropriate dependency tracking. Due to the nature of symbolic links it is
 important to use this tool when creating links during a build, as opposed to the
-usuall `shell` tool. The reason why is that the build system will, by default,
+usual `shell` tool. The reason why is that the build system will, by default,
 use `stat(2)` to examine the contents of output files for the purposes of
 evaluating the build state. In the case of a symbolic link this is incorrect, as
 it will retrieve the status information of the target, not the link itself. This

include/llbuild/Basic/PlatformUtility.h

@@ -47,6 +47,9 @@ char *strsep(char **stringp, const char *delim);
 // it's path.
 std::string makeTmpDir();
 
+// Return a string containing all valid path separators on the current platform
+std::string getPathSeparators();
+
 /// Sets the max open file limit to min(max(soft_limit, limit), hard_limit),
 /// where soft_limit and hard_limit are gathered from the system.
 ///

lib/Basic/FileSystem.cpp

@@ -171,10 +171,17 @@ class LocalFileSystem : public FileSystem {
       return true;
     }
 
+#if defined(_WIN32)
+    // Windows sets EACCES if the file is a directory
+    if (errno != EACCES) {
+      return false;
+    }
+#else
     // Error can't be that `path` is actually a directory (on Linux `EISDIR` will be returned since 2.1.132).
     if (errno != EPERM && errno != EISDIR) {
       return false;
     }
+#endif
 
     // Check if `path` is a directory.
     llbuild::basic::sys::StatStruct statbuf;

lib/Basic/PlatformUtility.cpp

@@ -11,12 +11,14 @@
 #include "llbuild/Basic/Stat.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/ConvertUTF.h"
+#include "llvm/Support/Path.h"
 
 #if defined(_WIN32)
 #include "LeanWindows.h"
 #include <Shlwapi.h>
 #include <direct.h>
 #include <io.h>
+#include <time.h>
 #else
 #include <fnmatch.h>
 #include <unistd.h>
@@ -28,7 +30,9 @@ using namespace llbuild::basic;
 
 bool sys::chdir(const char *fileName) {
 #if defined(_WIN32)
-  return SetCurrentDirectoryA(fileName);
+  llvm::SmallVector<UTF16, 20> wFileName;
+  llvm::convertUTF8ToUTF16String(fileName, wFileName);
+  return SetCurrentDirectoryW((LPCWSTR)wFileName.data());
 #else
   return ::chdir(fileName) == 0;
 #endif
@@ -42,11 +46,68 @@ int sys::close(int fileHandle) {
 #endif
 }
 
+#if defined(_WIN32)
+time_t filetimeToTime_t(FILETIME ft) {
+  long long ltime = ft.dwLowDateTime | ((long long)ft.dwHighDateTime << 32);
+  return (time_t)((ltime - 116444736000000000) / 10000000);
+}
+#endif
+
 int sys::lstat(const char *fileName, sys::StatStruct *buf) {
 #if defined(_WIN32)
-  // We deliberately ignore lstat on Windows, and delegate
-  // to stat.
-  return ::_stat(fileName, buf);
+  llvm::SmallVector<UTF16, 20> wfilename;
+  llvm::convertUTF8ToUTF16String(fileName, wfilename);
+  HANDLE h = CreateFileW(
+      /*lpFileName=*/(LPCWSTR)wfilename.data(),
+      /*dwDesiredAccess=*/0,
+      /*dwShareMode=*/FILE_SHARE_READ,
+      /*lpSecurityAttributes=*/NULL,
+      /*dwCreationDisposition=*/OPEN_EXISTING,
+      /*dwFlagsAndAttributes=*/FILE_FLAG_OPEN_REPARSE_POINT |
+          FILE_FLAG_BACKUP_SEMANTICS,
+      /*hTemplateFile=*/NULL);
+  if (h == INVALID_HANDLE_VALUE) {
+    int err = GetLastError();
+    if (err == ERROR_FILE_NOT_FOUND) {
+      errno = ENOENT;
+    }
+    return -1;
+  }
+  BY_HANDLE_FILE_INFORMATION info;
+  GetFileInformationByHandle(h, &info);
+  // Group id is always 0 on Windows
+  buf->st_gid = 0;
+  buf->st_atime = filetimeToTime_t(info.ftLastAccessTime);
+  buf->st_ctime = filetimeToTime_t(info.ftCreationTime);
+  buf->st_dev = info.dwVolumeSerialNumber;
+  // inodes have meaning on FAT/HPFS/NTFS
+  buf->st_ino = 0;
+  buf->st_rdev = info.dwVolumeSerialNumber;
+  buf->st_mode =
+      // On a symlink to a directory, Windows sets both the REPARSE_POINT and
+      // DIRECTORY attributes. Since Windows doesn't provide S_IFLNK and we
+      // want unix style "symlinks to directories are not directories
+      // themselves, we say symlinks are regular files
+      (info.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT)
+          ? _S_IFREG
+          : (info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) ? _S_IFDIR
+                                                               : _S_IFREG;
+  buf->st_mode |= (info.dwFileAttributes & FILE_ATTRIBUTE_READONLY)
+                      ? _S_IREAD
+                      : _S_IREAD | _S_IWRITE;
+  llvm::StringRef extension =
+      llvm::sys::path::extension(llvm::StringRef(fileName));
+  if (extension == ".exe" || extension == ".cmd" || extension == ".bat" ||
+      extension == ".com") {
+    buf->st_mode |= _S_IEXEC;
+  }
+  buf->st_mtime = filetimeToTime_t(info.ftLastWriteTime);
+  buf->st_nlink = info.nNumberOfLinks;
+  buf->st_size = ((long long)info.nFileSizeHigh << 32) | info.nFileSizeLow;
+  // Uid is always 0 on Windows systems
+  buf->st_uid = 0;
+  CloseHandle(h);
+  return 0;
 #else
   return ::lstat(fileName, buf);
 #endif
@@ -109,17 +170,25 @@ int sys::stat(const char *fileName, StatStruct *buf) {
 #endif
 }
 
-int sys::symlink(const char *source, const char *target) {
+// Create a symlink named linkPath which contains the string pointsTo
+int sys::symlink(const char *pointsTo, const char *linkPath) {
 #if defined(_WIN32)
-  DWORD attributes = GetFileAttributesA(source);
-  if (attributes != INVALID_FILE_ATTRIBUTES &&
-      (attributes & FILE_ATTRIBUTE_DIRECTORY) != 0) {
-    return ::CreateSymbolicLinkA(source, target, SYMBOLIC_LINK_FLAG_DIRECTORY);
-  }
-
-  return ::CreateSymbolicLinkA(source, target, 0);
+  llvm::SmallVector<UTF16, 20> wPointsTo;
+  llvm::convertUTF8ToUTF16String(pointsTo, wPointsTo);
+  llvm::SmallVector<UTF16, 20> wLinkPath;
+  llvm::convertUTF8ToUTF16String(linkPath, wLinkPath);
+  DWORD attributes = GetFileAttributesW((LPCWSTR)wPointsTo.data());
+  DWORD directoryFlag = (attributes != INVALID_FILE_ATTRIBUTES &&
+                         attributes & FILE_ATTRIBUTE_DIRECTORY)
+                            ? SYMBOLIC_LINK_FLAG_DIRECTORY
+                            : 0;
+  // Note that CreateSymbolicLinkW takes its arguments in reverse order
+  // compared to symlink/_symlink
+  return !::CreateSymbolicLinkW(
+      (LPCWSTR)wLinkPath.data(), (LPCWSTR)wPointsTo.data(),
+      SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE | directoryFlag);
 #else
-  return ::symlink(source, target);
+  return ::symlink(pointsTo, linkPath);
 #endif
 }
 
@@ -217,23 +286,29 @@ std::string sys::strerror(int error) {
 
 char *sys::strsep(char **stringp, const char *delim) {
 #if defined(_WIN32)
-  char *begin, *end = *stringp;
+  // If *stringp is NULL, the strsep() function returns NULL and does nothing
+  // else.
   if (*stringp == NULL) {
     return NULL;
   }
-  int delimLen = strlen(delim);
-  bool found = false;
-  while (*end) {
-    for (int i = 0; i < delimLen; i++) {
+  char *begin = *stringp;
+  char *end = *stringp;
+  do {
+    // Otherwise, this function finds the first token in the string *stringp,
+    // that is delimited by one of the bytes in the string delim.
+    for (int i = 0; delim[i] != '\0'; i++) {
       if (*end == delim[i]) {
-        found = true;
+        // This token is terminated by overwriting the delimiter with a null
+        // byte ('\0'), and *stringp is updated to point past the token.
+        *end = '\0';
+        *stringp = end + 1;
+        return begin;
       }
     }
-    if (found) {
-      *stringp = end + 1;
-    }
-  }
-  *end = '\0';
+  } while (*(++end));
+  // In case no delimiter was found, the token is taken to be the entire string
+  // *stringp, and *stringp is made NULL.
+  *stringp = NULL;
   return begin;
 #else
   return ::strsep(stringp, delim);
@@ -244,9 +319,20 @@ std::string sys::makeTmpDir() {
 #if defined(_WIN32)
   char path[MAX_PATH];
   tmpnam_s(path, MAX_PATH);
+  llvm::SmallVector<UTF16, 20> wPath;
+  llvm::convertUTF8ToUTF16String(path, wPath);
+  CreateDirectoryW((LPCWSTR)wPath.data(), NULL);
   return std::string(path);
 #else
   char tmpDirPathBuf[] = "/tmp/fileXXXXXX";
   return std::string(mkdtemp(tmpDirPathBuf));
 #endif
 }
+
+std::string sys::getPathSeparators() {
+#if defined(_WIN32)
+  return "/\\";
+#else
+  return "/";
+#endif
+}

lib/Basic/Subprocess.cpp

@@ -285,11 +285,10 @@ static void cleanUpExecutedProcess(ProcessDelegate& delegate,
   FILETIME stimeTicks;
   int waitResult = WaitForSingleObject(pid, INFINITE);
   int err = GetLastError();
-  DWORD exitCode;
+  DWORD exitCode = 0;
   GetExitCodeProcess(pid, &exitCode);
 
-  if (exitCode != 0 || waitResult == WAIT_FAILED ||
-      waitResult == WAIT_ABANDONED) {
+  if (waitResult == WAIT_FAILED || waitResult == WAIT_ABANDONED) {
     auto result = ProcessResult::makeFailed(exitCode);
     delegate.processHadError(ctx, handle,
                              Twine("unable to wait for process (") +
@@ -298,6 +297,25 @@ static void cleanUpExecutedProcess(ProcessDelegate& delegate,
     completionFn(result);
     return;
   }
+#else
+  // Wait for the command to complete.
+  struct rusage usage;
+  int exitCode, result = wait4(pid, &exitCode, 0, &usage);
+  while (result == -1 && errno == EINTR)
+    result = wait4(pid, &exitCode, 0, &usage);
+#endif
+  // Close the release pipe
+  //
+  // Note: We purposely hold this open until after the process has finished as
+  // it simplifies client implentation. If we close it early, clients need to be
+  // aware of and potentially handle a SIGPIPE.
+  if (releaseFd >= 0) {
+    sys::FileDescriptorTraits<>::Close(releaseFd);
+  }
+
+  // Update the set of spawned processes.
+  pgrp.remove(pid);
+#if defined(_WIN32)
   PROCESS_MEMORY_COUNTERS counters;
   bool res =
       GetProcessTimes(pid, &creationTime, &exitTime, &stimeTicks, &utimeTicks);
@@ -317,9 +335,6 @@ static void cleanUpExecutedProcess(ProcessDelegate& delegate,
       ((uint64_t)stimeTicks.dwHighDateTime << 32 | stimeTicks.dwLowDateTime) /
       10;
   GetProcessMemoryInfo(pid, &counters, sizeof(counters));
-  sys::FileDescriptorTraits<>::Close(releaseFd);
-
-  pgrp.remove(pid);
 
   // We report additional info in the tracing interval
   //   - user time, in µs
@@ -330,32 +345,16 @@ static void cleanUpExecutedProcess(ProcessDelegate& delegate,
   // subprocess (probably as a new point sample).
 
   // Notify of the process completion.
-  ProcessStatus processStatus = ProcessStatus::Succeeded;
+  ProcessStatus processStatus =
+      (exitCode == 0) ? ProcessStatus::Succeeded : ProcessStatus::Failed;
   ProcessResult processResult(processStatus, exitCode, pid, utime, stime,
                               counters.PeakWorkingSetSize);
 #else  // !defined(_WIN32)
-  // Wait for the command to complete.
-  struct rusage usage;
-  int exitCode, result = wait4(pid, &exitCode, 0, &usage);
-  while (result == -1 && errno == EINTR)
-    result = wait4(pid, &exitCode, 0, &usage);
-
-  // Close the release pipe
-  //
-  // Note: We purposely hold this open until after the process has finished as
-  // it simplifies client implentation. If we close it early, clients need to be
-  // aware of and potentially handle a SIGPIPE.
-  if (releaseFd >= 0) {
-    sys::FileDescriptorTraits<>::Close(releaseFd);
-  }
-
-  // Update the set of spawned processes.
-  pgrp.remove(pid);
-
   if (result == -1) {
     auto result = ProcessResult::makeFailed(exitCode);
     delegate.processHadError(ctx, handle,
-                             Twine("unable to wait for process (") + strerror(errno) + ")");
+                             Twine("unable to wait for process (") +
+                                 strerror(errno) + ")");
     delegate.processFinished(ctx, handle, result);
     completionFn(result);
     return;
@@ -439,8 +438,8 @@ void llbuild::basic::spawnProcess(
   startupInfo.dwFlags = STARTF_USESTDHANDLES;
   // Set NUL as stdin
   HANDLE nul =
-      CreateFileA("NUL", GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE, NULL,
-                  OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+      CreateFileW(L"NUL", GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE,
+                  NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
   startupInfo.hStdInput = nul;
 #else
   // Initialize the spawn attributes.

lib/BuildSystem/BuildSystem.cpp

@@ -3453,11 +3453,11 @@ class SymlinkCommand : public Command {
     // FIXME: Need to use the filesystem interfaces.
     bsci.getDelegate().commandStarted(this);
     auto success = true;
-    if (llvm::sys::fs::create_link(contents, outputPath)) {
+    if (basic::sys::symlink(contents.c_str(), outputPath.str().c_str())) {
       // On failure, we attempt to unlink the file and retry.
       basic::sys::unlink(outputPath.str().c_str());
-        
-      if (llvm::sys::fs::create_link(contents, outputPath)) {
+
+      if (basic::sys::symlink(contents.c_str(), outputPath.str().c_str())) {
         getBuildSystem(bsci.getBuildEngine()).getDelegate().commandHadError(this,
                        "unable to create symlink at '" + outputPath.str() + "'");
         success = false;
@@ -3650,7 +3650,7 @@ class StaleFileRemovalCommand : public Command {
   BuildValue priorValue;
   bool hasPriorResult = false;
 
-  char path_separator = llvm::sys::path::get_separator()[0];
+  std::string pathSeparators = llbuild::basic::sys::getPathSeparators();
 
   virtual void configureDescription(const ConfigureContext&, StringRef value) override {
     description = value;
@@ -3790,7 +3790,8 @@ class StaleFileRemovalCommand : public Command {
       bool isLocatedUnderRootPath = roots.size() == 0 ? true : false;
 
       // If root paths are defined, stale file paths should be absolute.
-      if (roots.size() > 0 && fileToDelete[0] != path_separator) {
+      if (roots.size() > 0 &&
+          pathSeparators.find(fileToDelete[0]) == std::string::npos) {
         bsci.getDelegate().commandHadWarning(this, "Stale file '" + fileToDelete + "' has a relative path. This is invalid in combination with the root path attribute.\n");
         continue;
       }
@@ -4011,11 +4012,25 @@ void BuildSystem::resetForBuild() {
 }
 
 // This function checks if the given path is prefixed by another path.
-bool llbuild::buildsystem::pathIsPrefixedByPath(std::string path, std::string prefixPath) {
-  static char path_separator = llvm::sys::path::get_separator()[0];
+bool llbuild::buildsystem::pathIsPrefixedByPath(std::string path,
+                                                std::string prefixPath) {
+  std::string pathSeparators = llbuild::basic::sys::getPathSeparators();
+  // Note: GCC 4.8 doesn't support the mismatch(first1, last1, first2, last2)
+  // overload, just mismatch(first1, last1, first2), so we have to handle the
+  // case where prefixPath is longer than path.
+  if (prefixPath.length() > path.length()) {
+    // The only case where the prefix can be longer and still be a valid prefix
+    // is "/foo/" is a prefix of "/foo"
+    return prefixPath.substr(0, prefixPath.length() - 1) == path &&
+           pathSeparators.find(prefixPath[prefixPath.length() - 1]) !=
+               std::string::npos;
+  }
   auto res = std::mismatch(prefixPath.begin(), prefixPath.end(), path.begin());
   // Check if `prefixPath` has been exhausted or just a separator remains.
-  bool isPrefix = res.first == prefixPath.end() || (*(res.first++) == path_separator);
+  bool isPrefix = res.first == prefixPath.end() ||
+                  (pathSeparators.find(*(res.first++)) != std::string::npos);
   // Check if `path` has been exhausted or just a separator remains.
-  return isPrefix && (res.second == path.end() || (*(res.second++) == path_separator));
+  return isPrefix &&
+         (res.second == path.end() ||
+          (pathSeparators.find(*(res.second++)) != std::string::npos));
 }