LTO: Fix a potential race condition in the caching API.

After the call to sys::fs::exists succeeds, indicating a cache hit, we call
AddFile and the client will open the file using the supplied path. If the
client is using cache pruning, there is a potential race between the pruner
and the client. To avoid this, change the caching API so that it provides
a MemoryBuffer to the client, and have clients use that MemoryBuffer where
possible.

This scheme won't work with the gold plugin because the plugin API expects a
file path. So we have the gold plugin use the buffer identifier as a path and
live with the race for now. (Note that the gold plugin isn't actually affected
by the problem at the moment because it doesn't support cache pruning.)

This effectively reverts r279883 modulo the change to use the existing path
in the gold plugin.

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

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

diff --git a/include/llvm/LTO/Caching.h b/include/llvm/LTO/Caching.h
index fe18009b14c760a6cf2be17385724a7e18940994..f5ec70e081c12ce4d824173acf02619eca7bd101 100644 (file)
--- a/include/llvm/LTO/Caching.h
+++ b/include/llvm/LTO/Caching.h
@@ -24,14 +24,19 @@ namespace lto {
 /// This type defines the callback to add a pre-existing native object file
 /// (e.g. in a cache).
 ///
-/// File callbacks must be thread safe.
-typedef std::function<void(unsigned Task, StringRef Path)> AddFileFn;
+/// 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.
+///
+/// Buffer callbacks must be thread safe.
+typedef std::function<void(unsigned Task, std::unique_ptr<MemoryBuffer> MB)>
+    AddBufferFn;
 
 /// Create a local file system cache which uses the given cache directory and
 /// file callback. This function also creates the cache directory if it does not
 /// already exist.
 Expected<NativeObjectCache> localCache(StringRef CacheDirectoryPath,
-                                       AddFileFn AddFile);
+                                       AddBufferFn AddBuffer);
 
 } // namespace lto
 } // namespace llvm

diff --git a/lib/LTO/Caching.cpp b/lib/LTO/Caching.cpp
index b7542049dbf66863577e1d81a272e65ab86577a1..d8b91c48ee306c2f97d99ef272b54db9a9d6a7f3 100644 (file)
--- a/lib/LTO/Caching.cpp
+++ b/lib/LTO/Caching.cpp
@@ -22,7 +22,7 @@ using namespace llvm;
 using namespace llvm::lto;
 
 Expected<NativeObjectCache> lto::localCache(StringRef CacheDirectoryPath,
-                                            AddFileFn AddFile) {
+                                            AddBufferFn AddBuffer) {
   if (std::error_code EC = sys::fs::create_directories(CacheDirectoryPath))
     return errorCodeToError(EC);
 
@@ -30,34 +30,49 @@ Expected<NativeObjectCache> lto::localCache(StringRef CacheDirectoryPath,
     // First, see if we have a cache hit.
     SmallString<64> EntryPath;
     sys::path::append(EntryPath, CacheDirectoryPath, Key);
-    if (sys::fs::exists(EntryPath)) {
-      AddFile(Task, EntryPath);
+    ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr =
+        MemoryBuffer::getFile(EntryPath);
+    if (MBOrErr) {
+      AddBuffer(Task, std::move(*MBOrErr));
       return AddStreamFn();
     }
 
+    if (MBOrErr.getError() != std::errc::no_such_file_or_directory)
+      report_fatal_error(Twine("Failed to open cache file ") + EntryPath +
+                         ": " + MBOrErr.getError().message() + "\n");
+
     // This native object stream is responsible for commiting the resulting
-    // file to the cache and calling AddFile to add it to the link.
+    // file to the cache and calling AddBuffer to add it to the link.
     struct CacheStream : NativeObjectStream {
-      AddFileFn AddFile;
+      AddBufferFn AddBuffer;
       std::string TempFilename;
       std::string EntryPath;
       unsigned Task;
 
-      CacheStream(std::unique_ptr<raw_pwrite_stream> OS, AddFileFn AddFile,
+      CacheStream(std::unique_ptr<raw_pwrite_stream> OS, AddBufferFn AddBuffer,
                   std::string TempFilename, std::string EntryPath,
                   unsigned Task)
-          : NativeObjectStream(std::move(OS)), AddFile(std::move(AddFile)),
+          : NativeObjectStream(std::move(OS)), AddBuffer(std::move(AddBuffer)),
             TempFilename(std::move(TempFilename)),
             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.
         if (auto EC = sys::fs::rename(TempFilename, EntryPath))
           report_fatal_error(Twine("Failed to rename temporary file ") +
                              TempFilename + ": " + EC.message() + "\n");
-        AddFile(Task, EntryPath);
+
+        ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr =
+            MemoryBuffer::getFile(EntryPath);
+        if (!MBOrErr)
+          report_fatal_error(Twine("Failed to open cache file ") + EntryPath +
+                             ": " + MBOrErr.getError().message() + "\n");
+        AddBuffer(Task, std::move(*MBOrErr));
       }
     };
 
@@ -77,7 +92,7 @@ Expected<NativeObjectCache> lto::localCache(StringRef CacheDirectoryPath,
       // This CacheStream will move the temporary file into the cache when done.
       return llvm::make_unique<CacheStream>(
           llvm::make_unique<raw_fd_ostream>(TempFD, /* ShouldClose */ true),
-          AddFile, TempFilename.str(), EntryPath.str(), Task);
+          AddBuffer, TempFilename.str(), EntryPath.str(), Task);
     };
   };
 }

diff --git a/tools/gold/gold-plugin.cpp b/tools/gold/gold-plugin.cpp
index 123d90df8904a39f786cf88ed1e2e90badfac93f..38674d2071737feee5cb00b1c6ce321e293031ab 100644 (file)
--- a/tools/gold/gold-plugin.cpp
+++ b/tools/gold/gold-plugin.cpp
@@ -831,11 +831,15 @@ static ld_plugin_status allSymbolsReadHook() {
         llvm::make_unique<llvm::raw_fd_ostream>(FD, true));
   };
 
-  auto AddFile = [&](size_t Task, StringRef Path) { Filenames[Task] = Path; };
+  auto AddBuffer = [&](size_t Task, std::unique_ptr<MemoryBuffer> MB) {
+    // Note that this requires that the memory buffers provided to AddBuffer are
+    // backed by a file.
+    Filenames[Task] = MB->getBufferIdentifier();
+  };
 
   NativeObjectCache Cache;
   if (!options::cache_dir.empty())
-    Cache = check(localCache(options::cache_dir, AddFile));
+    Cache = check(localCache(options::cache_dir, AddBuffer));
 
   check(Lto->run(AddStream, Cache));
 

diff --git a/tools/llvm-lto2/llvm-lto2.cpp b/tools/llvm-lto2/llvm-lto2.cpp
index 3fe0487476f78a39b8b6450289e2d77ac7aafb3d..ce0f4b98425582a669202652c0ae669695953a6e 100644 (file)
--- a/tools/llvm-lto2/llvm-lto2.cpp
+++ b/tools/llvm-lto2/llvm-lto2.cpp
@@ -275,18 +275,13 @@ int main(int argc, char **argv) {
     return llvm::make_unique<lto::NativeObjectStream>(std::move(S));
   };
 
-  auto AddFile = [&](size_t Task, StringRef Path) {
-    auto ReloadedBufferOrErr = MemoryBuffer::getFile(Path);
-    if (auto EC = ReloadedBufferOrErr.getError())
-      report_fatal_error(Twine("Can't reload cached file '") + Path + "': " +
-                         EC.message() + "\n");
-
-    *AddStream(Task)->OS << (*ReloadedBufferOrErr)->getBuffer();
+  auto AddBuffer = [&](size_t Task, std::unique_ptr<MemoryBuffer> MB) {
+    *AddStream(Task)->OS << MB->getBuffer();
   };
 
   NativeObjectCache Cache;
   if (!CacheDir.empty())
-    Cache = check(localCache(CacheDir, AddFile), "failed to create cache");
+    Cache = check(localCache(CacheDir, AddBuffer), "failed to create cache");
 
   check(Lto.run(AddStream, Cache), "LTO::run failed");
 }