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

1 files changed, 61 insertions, 25 deletions

diff --git a/llvm/lib/Support/LockFileManager.cpp b/llvm/lib/Support/LockFileManager.cpp
index 368b276f4b6..77baf7ac4bd 100644
--- a/llvm/lib/Support/LockFileManager.cpp
+++ b/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() {