Merge pull request #424 from apple/revert-422-AUnitaryOperation

Revert "Fixed the Unit Tests on Windows" to unblock Swift-CI

rdar://problem/47741242

Author: dmbryson

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
-usual `shell` tool. The reason why is that the build system will, by default,
+usuall `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,9 +47,6 @@ 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,17 +171,10 @@ 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,14 +11,12 @@
 #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>
@@ -30,9 +28,7 @@ using namespace llbuild::basic;
 
 bool sys::chdir(const char *fileName) {
 #if defined(_WIN32)
-  llvm::SmallVector<UTF16, 20> wFileName;
-  llvm::convertUTF8ToUTF16String(fileName, wFileName);
-  return SetCurrentDirectoryW((LPCWSTR)wFileName.data());
+  return SetCurrentDirectoryA(fileName);
 #else
   return ::chdir(fileName) == 0;
 #endif
@@ -46,68 +42,11 @@ 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)
-  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;
+  // We deliberately ignore lstat on Windows, and delegate
+  // to stat.
+  return ::_stat(fileName, buf);
 #else
   return ::lstat(fileName, buf);
 #endif
@@ -170,25 +109,17 @@ int sys::stat(const char *fileName, StatStruct *buf) {
 #endif
 }
 
-// Create a symlink named linkPath which contains the string pointsTo
-int sys::symlink(const char *pointsTo, const char *linkPath) {
+int sys::symlink(const char *source, const char *target) {
 #if defined(_WIN32)
-  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);
+  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);
 #else
-  return ::symlink(pointsTo, linkPath);
+  return ::symlink(source, target);
 #endif
 }
 
@@ -286,29 +217,23 @@ std::string sys::strerror(int error) {
 
 char *sys::strsep(char **stringp, const char *delim) {
 #if defined(_WIN32)
-  // If *stringp is NULL, the strsep() function returns NULL and does nothing
-  // else.
+  char *begin, *end = *stringp;
   if (*stringp == NULL) {
     return NULL;
   }
-  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++) {
+  int delimLen = strlen(delim);
+  bool found = false;
+  while (*end) {
+    for (int i = 0; i < delimLen; i++) {
       if (*end == delim[i]) {
-        // 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;
+        found = true;
       }
     }
-  } 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;
+    if (found) {
+      *stringp = end + 1;
+    }
+  }
+  *end = '\0';
   return begin;
 #else
   return ::strsep(stringp, delim);
@@ -319,20 +244,9 @@ 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,10 +285,11 @@ static void cleanUpExecutedProcess(ProcessDelegate& delegate,
   FILETIME stimeTicks;
   int waitResult = WaitForSingleObject(pid, INFINITE);
   int err = GetLastError();
-  DWORD exitCode = 0;
+  DWORD exitCode;
   GetExitCodeProcess(pid, &exitCode);
 
-  if (waitResult == WAIT_FAILED || waitResult == WAIT_ABANDONED) {
+  if (exitCode != 0 || waitResult == WAIT_FAILED ||
+      waitResult == WAIT_ABANDONED) {
     auto result = ProcessResult::makeFailed(exitCode);
     delegate.processHadError(ctx, handle,
                              Twine("unable to wait for process (") +
@@ -297,25 +298,6 @@ 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);
@@ -335,6 +317,9 @@ 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
@@ -345,16 +330,32 @@ static void cleanUpExecutedProcess(ProcessDelegate& delegate,
   // subprocess (probably as a new point sample).
 
   // Notify of the process completion.
-  ProcessStatus processStatus =
-      (exitCode == 0) ? ProcessStatus::Succeeded : ProcessStatus::Failed;
+  ProcessStatus processStatus = ProcessStatus::Succeeded;
   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;
@@ -438,8 +439,8 @@ void llbuild::basic::spawnProcess(
   startupInfo.dwFlags = STARTF_USESTDHANDLES;
   // Set NUL as stdin
   HANDLE nul =
-      CreateFileW(L"NUL", GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE,
-                  NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+      CreateFileA("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 (basic::sys::symlink(contents.c_str(), outputPath.str().c_str())) {
+    if (llvm::sys::fs::create_link(contents, outputPath)) {
       // On failure, we attempt to unlink the file and retry.
       basic::sys::unlink(outputPath.str().c_str());
-
-      if (basic::sys::symlink(contents.c_str(), outputPath.str().c_str())) {
+        
+      if (llvm::sys::fs::create_link(contents, outputPath)) {
         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;
 
-  std::string pathSeparators = llbuild::basic::sys::getPathSeparators();
+  char path_separator = llvm::sys::path::get_separator()[0];
 
   virtual void configureDescription(const ConfigureContext&, StringRef value) override {
     description = value;
@@ -3790,8 +3790,7 @@ 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 &&
-          pathSeparators.find(fileToDelete[0]) == std::string::npos) {
+      if (roots.size() > 0 && fileToDelete[0] != path_separator) {
         bsci.getDelegate().commandHadWarning(this, "Stale file '" + fileToDelete + "' has a relative path. This is invalid in combination with the root path attribute.\n");
         continue;
       }
@@ -4013,14 +4012,10 @@ 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) {
-  std::string pathSeparators = llbuild::basic::sys::getPathSeparators();
-  auto res = std::mismatch(prefixPath.begin(), prefixPath.end(), path.begin(),
-                           path.end());
+  static char path_separator = llvm::sys::path::get_separator()[0];
+  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() ||
-                  (pathSeparators.find(*(res.first++)) != std::string::npos);
+  bool isPrefix = res.first == prefixPath.end() || (*(res.first++) == path_separator);
   // Check if `path` has been exhausted or just a separator remains.
-  return isPrefix &&
-         (res.second == path.end() ||
-          (pathSeparators.find(*(res.second++)) != std::string::npos));
+  return isPrefix && (res.second == path.end() || (*(res.second++) == path_separator));
 }