LTO: Keep file handles open for memory mapped files.

On Windows we've observed that if you open a file, write to it, map it into
memory and close the file handle, the contents of the memory mapping can
sometimes be incorrect. That was what we did when adding an entry to the
ThinLTO cache using the TempFile and MemoryBuffer classes, and it was causing
intermittent build failures on Chromium's ThinLTO bots on Windows. More
details are in the associated Chromium bug (crbug.com/786127).

We can prevent this from happening by keeping a handle to the file open while
the mapping is active. So this patch changes the mapped_file_region class to
duplicate the file handle when mapping the file and close it upon unmapping it.

One gotcha is that the file handle that we keep open must not have been
created with FILE_FLAG_DELETE_ON_CLOSE, as otherwise the operating system
will prevent other processes from opening the file. We can achieve this
by avoiding the use of FILE_FLAG_DELETE_ON_CLOSE altogether.  Instead,
we use SetFileInformationByHandle with FileDispositionInfo to manage the
delete-on-close bit. This lets us remove the hack that we used to use to
clear the delete-on-close bit on a file opened with FILE_FLAG_DELETE_ON_CLOSE.

A downside of using SetFileInformationByHandle/FileDispositionInfo as
opposed to FILE_FLAG_DELETE_ON_CLOSE is that it prevents us from using
CreateFile to open the file while the flag is set, even within the same
process. This doesn't seem to matter for almost every client of TempFile,
except for LockFileManager, which calls sys::fs::create_link to create a
hard link from the lock file, and in the process of doing so tries to open
the file. To prevent this change from breaking LockFileManager I changed it
to stop using TempFile by effectively reverting r318550.

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

llvm-svn: 334630

llvm/include/llvm/Support/FileSystem.h

@@ -1043,7 +1043,9 @@ class mapped_file_region {
   /// Platform-specific mapping state.
   size_t Size;
   void *Mapping;
-  int FD;
+#ifdef _WIN32
+  void *FileHandle;
+#endif
   mapmode Mode;
 
   std::error_code init(int FD, uint64_t Offset, mapmode Mode);

llvm/include/llvm/Support/LockFileManager.h

@@ -11,7 +11,6 @@
 
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SmallString.h"
-#include "llvm/Support/FileSystem.h"
 #include <system_error>
 #include <utility> // for std::pair
 
@@ -54,7 +53,7 @@ class LockFileManager {
 private:
   SmallString<128> FileName;
   SmallString<128> LockFileName;
-  Optional<sys::fs::TempFile> UniqueLockFile;
+  SmallString<128> UniqueLockFileName;
 
   Optional<std::pair<std::string, int> > Owner;
   std::error_code ErrorCode;

llvm/lib/LTO/Caching.cpp

@@ -40,7 +40,14 @@ Expected<NativeObjectCache> lto::localCache(StringRef CacheDirectoryPath,
       return AddStreamFn();
     }
 
-    if (MBOrErr.getError() != errc::no_such_file_or_directory)
+    // On Windows we can fail to open a cache file with a permission denied
+    // error. This generally means that another process has requested to delete
+    // the file while it is still open, but it could also mean that another
+    // process has opened the file without the sharing permissions we need.
+    // Since the file is probably being deleted we handle it in the same way as
+    // if the file did not exist at all.
+    if (MBOrErr.getError() != errc::no_such_file_or_directory &&
+        MBOrErr.getError() != errc::permission_denied)
       report_fatal_error(Twine("Failed to open cache file ") + EntryPath +
                          ": " + MBOrErr.getError().message() + "\n");
 

llvm/lib/Support/LockFileManager.cpp

@@ -9,10 +9,8 @@
 
 #include "llvm/Support/LockFileManager.h"
 #include "llvm/ADT/None.h"
-#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
-#include "llvm/Config/llvm-config.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
@@ -123,6 +121,39 @@ bool LockFileManager::processStillExecuting(StringRef HostID, int PID) {
   return true;
 }
 
+namespace {
+
+/// An RAII helper object ensure that the unique lock file is removed.
+///
+/// Ensures that if there is an error or a signal before we finish acquiring the
+/// lock, the unique file will be removed. And if we successfully take the lock,
+/// the signal handler is left in place so that signals while the lock is held
+/// will remove the unique lock file. The caller should ensure there is a
+/// matching call to sys::DontRemoveFileOnSignal when the lock is released.
+class RemoveUniqueLockFileOnSignal {
+  StringRef Filename;
+  bool RemoveImmediately;
+public:
+  RemoveUniqueLockFileOnSignal(StringRef Name)
+  : Filename(Name), RemoveImmediately(true) {
+    sys::RemoveFileOnSignal(Filename, nullptr);
+  }
+
+  ~RemoveUniqueLockFileOnSignal() {
+    if (!RemoveImmediately) {
+      // Leave the signal handler enabled. It will be removed when the lock is
+      // released.
+      return;
+    }
+    sys::fs::remove(Filename);
+    sys::DontRemoveFileOnSignal(Filename);
+  }
+
+  void lockAcquired() { RemoveImmediately = false; }
+};
+
+} // end anonymous namespace
+
 LockFileManager::LockFileManager(StringRef FileName)
 {
   this->FileName = FileName;
@@ -141,22 +172,16 @@ LockFileManager::LockFileManager(StringRef FileName)
     return;
 
   // Create a lock file that is unique to this instance.
-  Expected<sys::fs::TempFile> Temp =
-      sys::fs::TempFile::create(LockFileName + "-%%%%%%%%");
-  if (!Temp) {
-    std::error_code EC = errorToErrorCode(Temp.takeError());
-    std::string S("failed to create unique file with prefix ");
-    S.append(LockFileName.str());
+  UniqueLockFileName = LockFileName;
+  UniqueLockFileName += "-%%%%%%%%";
+  int UniqueLockFileID;
+  if (std::error_code EC = sys::fs::createUniqueFile(
+          UniqueLockFileName, UniqueLockFileID, UniqueLockFileName)) {
+    std::string S("failed to create unique file ");
+    S.append(UniqueLockFileName.str());
     setError(EC, S);
     return;
   }
-  UniqueLockFile = std::move(*Temp);
-
-  // Make sure we discard the temporary file on exit.
-  auto RemoveTempFile = llvm::make_scope_exit([&]() {
-    if (Error E = UniqueLockFile->discard())
-      setError(errorToErrorCode(std::move(E)));
-  });
 
   // Write our process ID to our unique lock file.
   {
@@ -166,46 +191,54 @@ LockFileManager::LockFileManager(StringRef FileName)
       return;
     }
 
-    raw_fd_ostream Out(UniqueLockFile->FD, /*shouldClose=*/false);
+    raw_fd_ostream Out(UniqueLockFileID, /*shouldClose=*/true);
     Out << HostID << ' ';
 #if LLVM_ON_UNIX
     Out << getpid();
 #else
     Out << "1";
 #endif
-    Out.flush();
+    Out.close();
 
     if (Out.has_error()) {
       // We failed to write out PID, so report the error, remove the
       // unique lock file, and fail.
       std::string S("failed to write to ");
-      S.append(UniqueLockFile->TmpName);
+      S.append(UniqueLockFileName.str());
       setError(Out.error(), S);
+      sys::fs::remove(UniqueLockFileName);
       return;
     }
   }
 
+  // Clean up the unique file on signal, which also releases the lock if it is
+  // held since the .lock symlink will point to a nonexistent file.
+  RemoveUniqueLockFileOnSignal RemoveUniqueFile(UniqueLockFileName);
+
   while (true) {
     // Create a link from the lock file name. If this succeeds, we're done.
     std::error_code EC =
-        sys::fs::create_link(UniqueLockFile->TmpName, LockFileName);
+        sys::fs::create_link(UniqueLockFileName, LockFileName);
     if (!EC) {
-      RemoveTempFile.release();
+      RemoveUniqueFile.lockAcquired();
       return;
     }
 
     if (EC != errc::file_exists) {
       std::string S("failed to create link ");
       raw_string_ostream OSS(S);
-      OSS << LockFileName.str() << " to " << UniqueLockFile->TmpName;
+      OSS << LockFileName.str() << " to " << UniqueLockFileName.str();
       setError(EC, OSS.str());
       return;
     }
 
     // Someone else managed to create the lock file first. Read the process ID
     // from the lock file.
-    if ((Owner = readLockFile(LockFileName)))
-      return; // RemoveTempFile will delete out our unique lock file.
+    if ((Owner = readLockFile(LockFileName))) {
+      // Wipe out our unique lock file (it's useless now)
+      sys::fs::remove(UniqueLockFileName);
+      return;
+    }
 
     if (!sys::fs::exists(LockFileName)) {
       // The previous owner released the lock file before we could read it.
@@ -217,7 +250,7 @@ LockFileManager::LockFileManager(StringRef FileName)
     // ownership.
     if ((EC = sys::fs::remove(LockFileName))) {
       std::string S("failed to remove lockfile ");
-      S.append(LockFileName.str());
+      S.append(UniqueLockFileName.str());
       setError(EC, S);
       return;
     }
@@ -252,7 +285,10 @@ LockFileManager::~LockFileManager() {
 
   // Since we own the lock, remove the lock file and our own unique lock file.
   sys::fs::remove(LockFileName);
-  consumeError(UniqueLockFile->discard());
+  sys::fs::remove(UniqueLockFileName);
+  // The unique file is now gone, so remove it from the signal handler. This
+  // matches a sys::RemoveFileOnSignal() in LockFileManager().
+  sys::DontRemoveFileOnSignal(UniqueLockFileName);
 }
 
 LockFileManager::WaitForUnlockResult LockFileManager::waitForUnlock() {

llvm/lib/Support/Path.cpp

@@ -1129,12 +1129,13 @@ Error TempFile::keep(const Twine &Name) {
   // Always try to close and rename.
 #ifdef _WIN32
   // If we cant't cancel the delete don't rename.
-  std::error_code RenameEC = cancelDeleteOnClose(FD);
+  auto H = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
+  std::error_code RenameEC = setDeleteDisposition(H, false);
   if (!RenameEC)
     RenameEC = rename_fd(FD, Name);
   // If we can't rename, discard the temporary file.
   if (RenameEC)
-    removeFD(FD);
+    setDeleteDisposition(H, true);
 #else
   std::error_code RenameEC = fs::rename(TmpName, Name);
   // If we can't rename, discard the temporary file.
@@ -1160,7 +1161,8 @@ Error TempFile::keep() {
   Done = true;
 
 #ifdef _WIN32
-  if (std::error_code EC = cancelDeleteOnClose(FD))
+  auto H = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
+  if (std::error_code EC = setDeleteDisposition(H, false))
     return errorCodeToError(EC);
 #else
   sys::DontRemoveFileOnSignal(TmpName);

llvm/lib/Support/Unix/Path.inc

@@ -634,8 +634,7 @@ std::error_code mapped_file_region::init(int FD, uint64_t Offset,
 
 mapped_file_region::mapped_file_region(int fd, mapmode mode, size_t length,
                                        uint64_t offset, std::error_code &ec)
-    : Size(length), Mapping(), FD(fd), Mode(mode) {
-  (void)FD;
+    : Size(length), Mapping(), Mode(mode) {
   (void)Mode;
   ec = init(fd, offset, mode);
   if (ec)