Support: Rewrite Windows implementation of sys::fs::rename to be more POSIXy.

The current implementation of rename uses ReplaceFile if the
destination file already exists. According to the documentation for
ReplaceFile, the source file is opened without a sharing mode. This
means that there is a short interval of time between when ReplaceFile
renames the file and when it closes the file during which the
destination file cannot be opened.

This behaviour is not POSIX compliant because rename is supposed
to be atomic. It was also causing intermittent link failures when
linking with a ThinLTO cache; the ThinLTO cache implementation expects
all cache files to be openable.

This patch addresses that problem by re-implementing rename
using CreateFile and SetFileInformationByHandle. It is roughly a
reimplementation of ReplaceFile with a better sharing policy as well
as support for renaming in the case where the destination file does
not exist.

This implementation is still not fully POSIX. Specifically in the case
where the destination file is open at the point when rename is called,
there will be a short interval of time during which the destination
file will not exist. It isn't clear whether it is possible to avoid
this using the Windows API.

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

llvm-svn: 315079

3 files changed, 187 insertions, 62 deletions

diff --git a/llvm/include/llvm/Support/FileSystem.h b/llvm/include/llvm/Support/FileSystem.h
index e8460ca0a31..09f2e34a488 100644
--- a/llvm/include/llvm/Support/FileSystem.h
+++ b/llvm/include/llvm/Support/FileSystem.h
@@ -343,7 +343,11 @@ std::error_code remove(const Twine &path, bool IgnoreNonExisting = true);
 ///          platform-specific error code.
 std::error_code remove_directories(const Twine &path, bool IgnoreErrors = true);
 
-/// @brief Rename \a from to \a to. Files are renamed as if by POSIX rename().
+/// @brief Rename \a from to \a to.
+///
+/// Files are renamed as if by POSIX rename(), except that on Windows there may
+/// be a short interval of time during which the destination file does not
+/// exist.
 ///
 /// @param from The path to rename from.
 /// @param to The path to rename to. This is created.
diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc
index c54bdedbde9..fbed7f8b025 100644
--- a/llvm/lib/Support/Windows/Path.inc
+++ b/llvm/lib/Support/Windows/Path.inc
@@ -359,65 +359,126 @@ std::error_code is_local(int FD, bool &Result) {
   return is_local_internal(FinalPath, Result);
 }
 
-std::error_code rename(const Twine &from, const Twine &to) {
-  // Convert to utf-16.
-  SmallVector<wchar_t, 128> wide_from;
-  SmallVector<wchar_t, 128> wide_to;
-  if (std::error_code ec = widenPath(from, wide_from))
-    return ec;
-  if (std::error_code ec = widenPath(to, wide_to))
-    return ec;
-
-  std::error_code ec = std::error_code();
+static std::error_code rename_internal(HANDLE FromHandle, const Twine &To,
+                                       bool ReplaceIfExists) {
+  SmallVector<wchar_t, 0> ToWide;
+  if (auto EC = widenPath(To, ToWide))
+    return EC;
 
-  // Retry while we see recoverable errors.
-  // System scanners (eg. indexer) might open the source file when it is written
-  // and closed.
+  std::vector<char> RenameInfoBuf(sizeof(FILE_RENAME_INFO) - sizeof(wchar_t) +
+                                  (ToWide.size() * sizeof(wchar_t)));
+  FILE_RENAME_INFO &RenameInfo =
+      *reinterpret_cast<FILE_RENAME_INFO *>(RenameInfoBuf.data());
+  RenameInfo.ReplaceIfExists = ReplaceIfExists;
+  RenameInfo.RootDirectory = 0;
+  RenameInfo.FileNameLength = ToWide.size();
+  std::copy(ToWide.begin(), ToWide.end(), RenameInfo.FileName);
+
+  if (!SetFileInformationByHandle(FromHandle, FileRenameInfo, &RenameInfo,
+                                  RenameInfoBuf.size()))
+    return mapWindowsError(GetLastError());
 
-  bool TryReplace = true;
+  return std::error_code();
+}
 
-  for (int i = 0; i < 2000; i++) {
-    if (i > 0)
-      ::Sleep(1);
+std::error_code rename(const Twine &From, const Twine &To) {
+  // Convert to utf-16.
+  SmallVector<wchar_t, 128> WideFrom;
+  SmallVector<wchar_t, 128> WideTo;
+  if (std::error_code EC = widenPath(From, WideFrom))
+    return EC;
+  if (std::error_code EC = widenPath(To, WideTo))
+    return EC;
 
-    if (TryReplace) {
-      // Try ReplaceFile first, as it is able to associate a new data stream
-      // with the destination even if the destination file is currently open.
-      if (::ReplaceFileW(wide_to.data(), wide_from.data(), NULL, 0, NULL, NULL))
-        return std::error_code();
+  ScopedFileHandle FromHandle;
+  // Retry this a few times to defeat badly behaved file system scanners.
+  for (unsigned Retry = 0; Retry != 200; ++Retry) {
+    if (Retry != 0)
+      ::Sleep(10);
+    FromHandle =
+        ::CreateFileW(WideFrom.begin(), GENERIC_READ | DELETE,
+                      FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
+                      NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
+    if (FromHandle)
+      break;
+  }
+  if (!FromHandle)
+    return mapWindowsError(GetLastError());
 
-      DWORD ReplaceError = ::GetLastError();
-      ec = mapWindowsError(ReplaceError);
+  // We normally expect this loop to succeed after a few iterations. If it
+  // requires more than 200 tries, it's more likely that the failures are due to
+  // a true error, so stop trying.
+  for (unsigned Retry = 0; Retry != 200; ++Retry) {
+    auto EC = rename_internal(FromHandle, To, true);
+    if (!EC || EC != errc::permission_denied)
+      return EC;
 
-      // If ReplaceFileW returned ERROR_UNABLE_TO_MOVE_REPLACEMENT or
-      // ERROR_UNABLE_TO_MOVE_REPLACEMENT_2, retry but only use MoveFileExW().
-      if (ReplaceError == ERROR_UNABLE_TO_MOVE_REPLACEMENT ||
-          ReplaceError == ERROR_UNABLE_TO_MOVE_REPLACEMENT_2) {
-        TryReplace = false;
+    // The destination file probably exists and is currently open in another
+    // process, either because the file was opened without FILE_SHARE_DELETE or
+    // it is mapped into memory (e.g. using MemoryBuffer). Rename it in order to
+    // move it out of the way of the source file. Use FILE_FLAG_DELETE_ON_CLOSE
+    // to arrange for the destination file to be deleted when the other process
+    // closes it.
+    ScopedFileHandle ToHandle(
+        ::CreateFileW(WideTo.begin(), GENERIC_READ | DELETE,
+                      FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
+                      NULL, OPEN_EXISTING,
+                      FILE_ATTRIBUTE_NORMAL | FILE_FLAG_DELETE_ON_CLOSE, NULL));
+    if (!ToHandle) {
+      auto EC = mapWindowsError(GetLastError());
+      // Another process might have raced with us and moved the existing file
+      // out of the way before we had a chance to open it. If that happens, try
+      // to rename the source file again.
+      if (EC == errc::no_such_file_or_directory)
         continue;
-      }
-      // If ReplaceFileW returned ERROR_UNABLE_TO_REMOVE_REPLACED, retry
-      // using ReplaceFileW().
-      if (ReplaceError == ERROR_UNABLE_TO_REMOVE_REPLACED)
-        continue;
-      // We get ERROR_FILE_NOT_FOUND if the destination file is missing.
-      // MoveFileEx can handle this case.
-      if (ReplaceError != ERROR_ACCESS_DENIED &&
-          ReplaceError != ERROR_FILE_NOT_FOUND &&
-          ReplaceError != ERROR_SHARING_VIOLATION)
-        break;
+      return EC;
     }
 
-    if (::MoveFileExW(wide_from.begin(), wide_to.begin(),
-                      MOVEFILE_COPY_ALLOWED | MOVEFILE_REPLACE_EXISTING))
-      return std::error_code();
+    BY_HANDLE_FILE_INFORMATION FI;
+    if (!GetFileInformationByHandle(ToHandle, &FI))
+      return mapWindowsError(GetLastError());
+
+    // Try to find a unique new name for the destination file.
+    for (unsigned UniqueId = 0; UniqueId != 200; ++UniqueId) {
+      std::string TmpFilename = (To + ".tmp" + utostr(UniqueId)).str();
+      if (auto EC = rename_internal(ToHandle, TmpFilename, false)) {
+        if (EC == errc::file_exists || EC == errc::permission_denied) {
+          // Again, another process might have raced with us and moved the file
+          // before we could move it. Check whether this is the case, as it
+          // might have caused the permission denied error. If that was the
+          // case, we don't need to move it ourselves.
+          ScopedFileHandle ToHandle2(::CreateFileW(
+              WideTo.begin(), 0,
+              FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, NULL,
+              OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL));
+          if (!ToHandle2) {
+            auto EC = mapWindowsError(GetLastError());
+            if (EC == errc::no_such_file_or_directory)
+              break;
+            return EC;
+          }
+          BY_HANDLE_FILE_INFORMATION FI2;
+          if (!GetFileInformationByHandle(ToHandle2, &FI2))
+            return mapWindowsError(GetLastError());
+          if (FI.nFileIndexHigh != FI2.nFileIndexHigh ||
+              FI.nFileIndexLow != FI2.nFileIndexLow ||
+              FI.dwVolumeSerialNumber != FI2.dwVolumeSerialNumber)
+            break;
+          continue;
+        }
+        return EC;
+      }
+      break;
+    }
 
-    DWORD MoveError = ::GetLastError();
-    ec = mapWindowsError(MoveError);
-    if (MoveError != ERROR_ACCESS_DENIED) break;
+    // Okay, the old destination file has probably been moved out of the way at
+    // this point, so try to rename the source file again. Still, another
+    // process might have raced with us to create and open the destination
+    // file, so we need to keep doing this until we succeed.
   }
 
-  return ec;
+  // The most likely root cause.
+  return errc::permission_denied;
 }
 
 std::error_code resize_file(int FD, uint64_t Size) {
diff --git a/llvm/unittests/Support/ReplaceFileTest.cpp b/llvm/unittests/Support/ReplaceFileTest.cpp
index 8b16daf3233..794f36b1f65 100644
--- a/llvm/unittests/Support/ReplaceFileTest.cpp
+++ b/llvm/unittests/Support/ReplaceFileTest.cpp
@@ -52,6 +52,21 @@ class ScopedFD {
   ~ScopedFD() { Process::SafelyCloseFileDescriptor(FD); }
 };
 
+bool FDHasContent(int FD, StringRef Content) {
+  auto Buffer = MemoryBuffer::getOpenFile(FD, "", -1);
+  assert(Buffer);
+  return Buffer.get()->getBuffer() == Content;
+}
+
+bool FileHasContent(StringRef File, StringRef Content) {
+  int FD = 0;
+  auto EC = fs::openFileForRead(File, FD);
+  (void)EC;
+  assert(!EC);
+  ScopedFD EventuallyCloseIt(FD);
+  return FDHasContent(FD, Content);
+}
+
 TEST(rename, FileOpenedForReadingCanBeReplaced) {
   // Create unique temporary directory for this test.
   SmallString<128> TestDirectory;
@@ -79,25 +94,15 @@ TEST(rename, FileOpenedForReadingCanBeReplaced) {
 
     // We should still be able to read the old data through the existing
     // descriptor.
-    auto Buffer = MemoryBuffer::getOpenFile(ReadFD, TargetFileName, -1);
-    ASSERT_TRUE(static_cast<bool>(Buffer));
-    EXPECT_EQ(Buffer.get()->getBuffer(), "!!target!!");
+    EXPECT_TRUE(FDHasContent(ReadFD, "!!target!!"));
 
     // The source file should no longer exist
     EXPECT_FALSE(fs::exists(SourceFileName));
   }
 
-  {
-    // If we obtain a new descriptor for the target file, we should find that it
-    // contains the content that was in the source file.
-    int ReadFD = 0;
-    ASSERT_NO_ERROR(fs::openFileForRead(TargetFileName, ReadFD));
-    ScopedFD EventuallyCloseIt(ReadFD);
-    auto Buffer = MemoryBuffer::getOpenFile(ReadFD, TargetFileName, -1);
-    ASSERT_TRUE(static_cast<bool>(Buffer));
-
-    EXPECT_EQ(Buffer.get()->getBuffer(), "!!source!!");
-  }
+  // If we obtain a new descriptor for the target file, we should find that it
+  // contains the content that was in the source file.
+  EXPECT_TRUE(FileHasContent(TargetFileName, "!!source!!"));
 
   // Rename the target file back to the source file name to confirm that rename
   // still works if the destination does not already exist.
@@ -110,4 +115,59 @@ TEST(rename, FileOpenedForReadingCanBeReplaced) {
   ASSERT_NO_ERROR(fs::remove(TestDirectory.str()));
 }
 
+TEST(rename, ExistingTemp) {
+  // Test that existing .tmpN files don't get deleted by the Windows
+  // sys::fs::rename implementation.
+  SmallString<128> TestDirectory;
+  ASSERT_NO_ERROR(
+      fs::createUniqueDirectory("ExistingTemp-test", TestDirectory));
+
+  SmallString<128> SourceFileName(TestDirectory);
+  path::append(SourceFileName, "source");
+
+  SmallString<128> TargetFileName(TestDirectory);
+  path::append(TargetFileName, "target");
+
+  SmallString<128> TargetTmp0FileName(TestDirectory);
+  path::append(TargetTmp0FileName, "target.tmp0");
+
+  SmallString<128> TargetTmp1FileName(TestDirectory);
+  path::append(TargetTmp1FileName, "target.tmp1");
+
+  ASSERT_NO_ERROR(CreateFileWithContent(SourceFileName, "!!source!!"));
+  ASSERT_NO_ERROR(CreateFileWithContent(TargetFileName, "!!target!!"));
+  ASSERT_NO_ERROR(CreateFileWithContent(TargetTmp0FileName, "!!target.tmp0!!"));
+
+  {
+    // Use mapped_file_region to make sure that the destination file is mmap'ed.
+    // This will cause SetInformationByHandle to fail when renaming to the
+    // destination, and we will follow the code path that tries to give target
+    // a temporary name.
+    int TargetFD;
+    std::error_code EC;
+    ASSERT_NO_ERROR(fs::openFileForRead(TargetFileName, TargetFD));
+    ScopedFD X(TargetFD);
+    sys::fs::mapped_file_region MFR(
+        TargetFD, sys::fs::mapped_file_region::readonly, 10, 0, EC);
+    ASSERT_FALSE(EC);
+
+    ASSERT_NO_ERROR(fs::rename(SourceFileName, TargetFileName));
+
+#ifdef _WIN32
+    // Make sure that target was temporarily renamed to target.tmp1 on Windows.
+    // This is signified by a permission denied error as opposed to no such file
+    // or directory when trying to open it.
+    int Tmp1FD;
+    EXPECT_EQ(errc::permission_denied,
+              fs::openFileForRead(TargetTmp1FileName, Tmp1FD));
+#endif
+  }
+
+  EXPECT_TRUE(FileHasContent(TargetTmp0FileName, "!!target.tmp0!!"));
+
+  ASSERT_NO_ERROR(fs::remove(TargetFileName));
+  ASSERT_NO_ERROR(fs::remove(TargetTmp0FileName));
+  ASSERT_NO_ERROR(fs::remove(TestDirectory.str()));
+}
+
 }  // anonymous namespace