From a3886c11ee55aa4cb683cbb34d9bf33e4de6fc63 Mon Sep 17 00:00:00 2001 From: Peter Collingbourne Date: Tue, 5 Sep 2017 19:51:38 +0000 Subject: [PATCH] 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 --- include/llvm/LTO/Caching.h | 9 +++++---- lib/LTO/Caching.cpp | 27 +++++++++++++++++++++------ tools/gold/gold-plugin.cpp | 5 +++-- tools/llvm-lto2/llvm-lto2.cpp | 3 ++- 4 files changed, 31 insertions(+), 13 deletions(-) diff --git a/include/llvm/LTO/Caching.h b/include/llvm/LTO/Caching.h index f5ec70e081c..25c719a68b9 100644 --- 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 MB)> +typedef std::function 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 e32e46c4c3c..3f10c154683 100644 --- a/lib/LTO/Caching.cpp +++ b/lib/LTO/Caching.cpp @@ -36,7 +36,7 @@ Expected lto::localCache(StringRef CacheDirectoryPath, ErrorOr> MBOrErr = MemoryBuffer::getFile(EntryPath); if (MBOrErr) { - AddBuffer(Task, std::move(*MBOrErr)); + AddBuffer(Task, std::move(*MBOrErr), EntryPath); return AddStreamFn(); } @@ -60,22 +60,37 @@ Expected 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> MBOrErr = MemoryBuffer::getFile(EntryPath); +#else + // Open the file first to avoid racing with a cache pruner. + ErrorOr> 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 4d4ff195364..947fc2d1652 100644 --- a/tools/gold/gold-plugin.cpp +++ b/tools/gold/gold-plugin.cpp @@ -908,10 +908,11 @@ static ld_plugin_status allSymbolsReadHook() { llvm::make_unique(FD, true)); }; - auto AddBuffer = [&](size_t Task, std::unique_ptr MB) { + auto AddBuffer = [&](size_t Task, std::unique_ptr 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 1c7038e6a2e..bc22b509b65 100644 --- 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(std::move(S)); }; - auto AddBuffer = [&](size_t Task, std::unique_ptr MB) { + auto AddBuffer = [&](size_t Task, std::unique_ptr MB, + StringRef Path) { *AddStream(Task)->OS << MB->getBuffer(); }; -- 2.40.0