LTO: Try to open cache files before renaming them.

It appears that a potential race between the cache client and the cache
pruner that I thought was unlikely actually happened in practice [1].
Try to avoid the race condition by opening the temporary file before
renaming it. Do this only on non-Windows platforms because we cannot
rename open files on Windows using the sys::fs::rename function.

[1] https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.memory%2FLinux_CFI%2F1610%2F%2B%2Frecipes%2Fsteps%2Fcompile%2F0%2Fstdout

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

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@312567 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/llvm/LTO/Caching.h b/include/llvm/LTO/Caching.h
index f5ec70e081c12ce4d824173acf02619eca7bd101..25c719a68b922201ccd141a86b706d7b279a11d5 100644 (file)
--- a/include/llvm/LTO/Caching.h
+++ b/include/llvm/LTO/Caching.h
@@ -24,12 +24,13 @@ namespace lto {
 /// This type defines the callback to add a pre-existing native object file
 /// (e.g. in a cache).
 ///
-/// MB->getBufferIdentifier() is a valid path for the file at the time that it
-/// was opened, but clients should prefer to access MB directly in order to
-/// avoid a potential race condition.
+/// Path is generally expected to be a valid path for the file at the point when
+/// the AddBufferFn function is called, but clients should prefer to access MB
+/// directly in order to avoid a potential race condition.
 ///
 /// Buffer callbacks must be thread safe.
-typedef std::function<void(unsigned Task, std::unique_ptr<MemoryBuffer> MB)>
+typedef std::function<void(unsigned Task, std::unique_ptr<MemoryBuffer> MB,
+                           StringRef Path)>
     AddBufferFn;
 
 /// Create a local file system cache which uses the given cache directory and

diff --git a/lib/LTO/Caching.cpp b/lib/LTO/Caching.cpp
index e32e46c4c3c8d9070a407d95edef6640e973e090..3f10c154683d983624d90a1e3caab7c7aabf6aad 100644 (file)
--- a/lib/LTO/Caching.cpp
+++ b/lib/LTO/Caching.cpp
@@ -36,7 +36,7 @@ Expected<NativeObjectCache> lto::localCache(StringRef CacheDirectoryPath,
     ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr =
         MemoryBuffer::getFile(EntryPath);
     if (MBOrErr) {
-      AddBuffer(Task, std::move(*MBOrErr));
+      AddBuffer(Task, std::move(*MBOrErr), EntryPath);
       return AddStreamFn();
     }
 
@@ -60,22 +60,37 @@ Expected<NativeObjectCache> lto::localCache(StringRef CacheDirectoryPath,
             EntryPath(std::move(EntryPath)), Task(Task) {}
 
       ~CacheStream() {
-        // FIXME: This code could race with the cache pruner, but it is unlikely
-        // that the cache pruner will choose to remove a newly created file.
-
         // Make sure the file is closed before committing it.
         OS.reset();
-        // This is atomic on POSIX systems.
+
+#ifdef _WIN32
+        // Rename the file first on Windows because we cannot rename an open
+        // file on that platform using the sys::fs::rename function.
+        // FIXME: This code could race with the cache pruner, but it is unlikely
+        // that the cache pruner will choose to remove a newly created file.
+        // We should look at using the SetFileInformationByHandle function to
+        // rename the file while it is open.
         if (auto EC = sys::fs::rename(TempFilename, EntryPath))
           report_fatal_error(Twine("Failed to rename temporary file ") +
                              TempFilename + ": " + EC.message() + "\n");
 
         ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr =
             MemoryBuffer::getFile(EntryPath);
+#else
+        // Open the file first to avoid racing with a cache pruner.
+        ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr =
+            MemoryBuffer::getFile(TempFilename);
+
+        // This is atomic on POSIX systems.
+        if (auto EC = sys::fs::rename(TempFilename, EntryPath))
+          report_fatal_error(Twine("Failed to rename temporary file ") +
+                             TempFilename + ": " + EC.message() + "\n");
+#endif
+
         if (!MBOrErr)
           report_fatal_error(Twine("Failed to open cache file ") + EntryPath +
                              ": " + MBOrErr.getError().message() + "\n");
-        AddBuffer(Task, std::move(*MBOrErr));
+        AddBuffer(Task, std::move(*MBOrErr), EntryPath);
       }
     };
 

diff --git a/tools/gold/gold-plugin.cpp b/tools/gold/gold-plugin.cpp
index 4d4ff195364ff6a464fceb8089e8c31174d5f995..947fc2d1652c7660975484790049321d037a76ad 100644 (file)
--- a/tools/gold/gold-plugin.cpp
+++ b/tools/gold/gold-plugin.cpp
@@ -908,10 +908,11 @@ static ld_plugin_status allSymbolsReadHook() {
         llvm::make_unique<llvm::raw_fd_ostream>(FD, true));
   };
 
-  auto AddBuffer = [&](size_t Task, std::unique_ptr<MemoryBuffer> MB) {
+  auto AddBuffer = [&](size_t Task, std::unique_ptr<MemoryBuffer> MB,
+                       StringRef Path) {
     // Note that this requires that the memory buffers provided to AddBuffer are
     // backed by a file.
-    Filenames[Task] = MB->getBufferIdentifier();
+    Filenames[Task] = Path;
   };
 
   NativeObjectCache Cache;

diff --git a/tools/llvm-lto2/llvm-lto2.cpp b/tools/llvm-lto2/llvm-lto2.cpp
index 1c7038e6a2e7c7cc0c223ec453415312bb77f6e6..bc22b509b651698b6abc58ed1b4dd8424c1aa95f 100644 (file)
--- a/tools/llvm-lto2/llvm-lto2.cpp
+++ b/tools/llvm-lto2/llvm-lto2.cpp
@@ -296,7 +296,8 @@ static int run(int argc, char **argv) {
     return llvm::make_unique<lto::NativeObjectStream>(std::move(S));
   };
 
-  auto AddBuffer = [&](size_t Task, std::unique_ptr<MemoryBuffer> MB) {
+  auto AddBuffer = [&](size_t Task, std::unique_ptr<MemoryBuffer> MB,
+                       StringRef Path) {
     *AddStream(Task)->OS << MB->getBuffer();
   };