LTO: Try to open cache files before renaming them.

[pcc]

Summary

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.

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

[zturner]

I think you actually can rename open files on Windows. Or at least, you can try to. I'll have to dig in when I have a bit more time though to see if there's any weird things we need to be aware of when doing it, or casse where it won't work.

But basically, what you do is call SetFileInformationByHandle and set the FileInformationClass parameter to FileRenameInfo, and fill out a FILE_RENAME_INFO structure to pass for the 3rd argument.

[mehdi_amini]

LGTM

llvm/include/llvm/LTO/Caching.h

27 ↗

(On Diff #113632)

The comment is not accurate I believe: the Path didn't exist when when the memory buffer was created, and it was never opened using this path.

[pcc]

In D37410#859518, @zturner wrote:

I think you actually can rename open files on Windows. Or at least, you can try to. I'll have to dig in when I have a bit more time though to see if there's any weird things we need to be aware of when doing it, or casse where it won't work.

But basically, what you do is call SetFileInformationByHandle and set the FileInformationClass parameter to FileRenameInfo, and fill out a FILE_RENAME_INFO structure to pass for the 3rd argument.

Thanks, I've left a FIXME about renaming the file in the way you've suggested.

llvm/include/llvm/LTO/Caching.h

27 ↗

(On Diff #113632)

Reworded this comment.

lld/trunk/ELF/LTO.cpp

std::vector<InputFile *> BitcodeCompiler::compile() {
 
   // The --thinlto-cache-dir option specifies the path to a directory in which
   // to cache native object files for ThinLTO incremental builds. If a path was
   // specified, configure LTO to use it as the cache directory.
   lto::NativeObjectCache Cache;
   if (!Config->ThinLTOCacheDir.empty())
     Cache = check(
         lto::localCache(Config->ThinLTOCacheDir,
-                        [&](size_t Task, std::unique_ptr<MemoryBuffer> MB) {
-                          Files[Task] = std::move(MB);
-                        }));
+                        [&](size_t Task, std::unique_ptr<MemoryBuffer> MB,
+                            StringRef Path) { Files[Task] = std::move(MB); }));
 
   checkError(LTOObj->run(
       [&](size_t Task) {
         return llvm::make_unique<lto::NativeObjectStream>(
             llvm::make_unique<raw_svector_ostream>(Buff[Task]));
       },
       Cache));
 

llvm/trunk/include/llvm/LTO/Caching.h

 #include <string>
 
 namespace llvm {
 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
 /// file callback. This function also creates the cache directory if it does not
 /// already exist.
 Expected<NativeObjectCache> localCache(StringRef CacheDirectoryPath,
                                        AddBufferFn AddBuffer);
 
 } // namespace lto
 } // namespace llvm
 
 #endif

llvm/trunk/lib/LTO/Caching.cpp

return [=](unsigned Task, StringRef Key) -> AddStreamFn {
     // This choice of file name allows the cache to be pruned (see pruneCache()
     // in include/llvm/Support/CachePruning.h).
     SmallString<64> EntryPath;
     sys::path::append(EntryPath, CacheDirectoryPath, "llvmcache-" + Key);
     // First, see if we have a cache hit.
     ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr =
         MemoryBuffer::getFile(EntryPath);
     if (MBOrErr) {
-      AddBuffer(Task, std::move(*MBOrErr));
+      AddBuffer(Task, std::move(*MBOrErr), EntryPath);
       return AddStreamFn();
     }
 
     if (MBOrErr.getError() != 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 AddBuffer to add it to the link.
     struct CacheStream : NativeObjectStream {
       AddBufferFn AddBuffer;
       std::string TempFilename;
       std::string EntryPath;
       unsigned Task;
 
       CacheStream(std::unique_ptr<raw_pwrite_stream> OS, AddBufferFn AddBuffer,
                   std::string TempFilename, std::string EntryPath,
                   unsigned Task)
           : 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.
+
+#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);
       }
     };
 
     return [=](size_t Task) -> std::unique_ptr<NativeObjectStream> {
       // Write to a temporary to avoid race condition
       int TempFD;
       SmallString<64> TempFilenameModel, TempFilename;
       sys::path::append(TempFilenameModel, CacheDirectoryPath, "Thin-%%%%%%.tmp.o");

llvm/trunk/tools/gold/gold-plugin.cpp

auto AddStream =
       [&](size_t Task) -> std::unique_ptr<lto::NativeObjectStream> {
     IsTemporary[Task] = !SaveTemps;
     int FD = getOutputFileName(Filename, /*TempOutFile=*/!SaveTemps,
                                Filenames[Task], Task);
     return llvm::make_unique<lto::NativeObjectStream>(
         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;
   if (!options::cache_dir.empty())
     Cache = check(localCache(options::cache_dir, AddBuffer));
 
   check(Lto->run(AddStream, Cache));
 

llvm/trunk/tools/llvm-lto2/llvm-lto2.cpp

auto AddStream =
     std::string Path = OutputFilename + "." + utostr(Task);
 
     std::error_code EC;
     auto S = llvm::make_unique<raw_fd_ostream>(Path, EC, sys::fs::F_None);
     check(EC, Path);
     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();
   };
 
   NativeObjectCache Cache;
   if (!CacheDir.empty())
     Cache = check(localCache(CacheDir, AddBuffer), "failed to create cache");
 
   check(Lto.run(AddStream, Cache), "LTO::run failed");