[llvm] r312567 - LTO: Try to open cache files before renaming them.
Peter Collingbourne via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 5 12:51:38 PDT 2017
Author: pcc
Date: Tue Sep 5 12:51:38 2017
New Revision: 312567
URL: http://llvm.org/viewvc/llvm-project?rev=312567&view=rev
Log:
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
Modified:
llvm/trunk/include/llvm/LTO/Caching.h
llvm/trunk/lib/LTO/Caching.cpp
llvm/trunk/tools/gold/gold-plugin.cpp
llvm/trunk/tools/llvm-lto2/llvm-lto2.cpp
Modified: llvm/trunk/include/llvm/LTO/Caching.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/LTO/Caching.h?rev=312567&r1=312566&r2=312567&view=diff
==============================================================================
--- llvm/trunk/include/llvm/LTO/Caching.h (original)
+++ llvm/trunk/include/llvm/LTO/Caching.h Tue Sep 5 12:51:38 2017
@@ -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
Modified: llvm/trunk/lib/LTO/Caching.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/LTO/Caching.cpp?rev=312567&r1=312566&r2=312567&view=diff
==============================================================================
--- llvm/trunk/lib/LTO/Caching.cpp (original)
+++ llvm/trunk/lib/LTO/Caching.cpp Tue Sep 5 12:51:38 2017
@@ -36,7 +36,7 @@ Expected<NativeObjectCache> lto::localCa
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::localCa
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);
}
};
Modified: llvm/trunk/tools/gold/gold-plugin.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/gold/gold-plugin.cpp?rev=312567&r1=312566&r2=312567&view=diff
==============================================================================
--- llvm/trunk/tools/gold/gold-plugin.cpp (original)
+++ llvm/trunk/tools/gold/gold-plugin.cpp Tue Sep 5 12:51:38 2017
@@ -908,10 +908,11 @@ static ld_plugin_status allSymbolsReadHo
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;
Modified: llvm/trunk/tools/llvm-lto2/llvm-lto2.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-lto2/llvm-lto2.cpp?rev=312567&r1=312566&r2=312567&view=diff
==============================================================================
--- llvm/trunk/tools/llvm-lto2/llvm-lto2.cpp (original)
+++ llvm/trunk/tools/llvm-lto2/llvm-lto2.cpp Tue Sep 5 12:51:38 2017
@@ -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();
};
More information about the llvm-commits
mailing list