[PATCH] D45076: Prevent data races in concurrent ThinLTO processes

[Katya Romanova via Phabricator via llvm-commits]

kromanova created this revision.
kromanova added reviewers: steven_wu, pcc, tejohnson, mehdi_amini, bd1976llvm, rafael.
Herald added subscribers: eraman, inglorion.

I few days ago I sent an email to the llvm-dev mailing list, describing a list of problems that the user could face with concurrent ThinLTO processes with caching enabled. More specifically, I identified a data race condition and other inefficiencies, caused by resource sharing problems. Here it is for the reference.

http://lists.llvm.org/pipermail/llvm-dev/2018-March/122046.html

After the long subsequent discussion on the mailing list we decided to implement the following straightforward solution described in this proposal.

Namely,

a.  Place temp file to the same place where the caching directory is (instead of creating it the directory pointed to by TMP/TEMP variable). This will help to prevent using non-atomic rename and falling back to non-atomic "direct" write to the cache file.
b.  if rename failed do not write to the cache file directly (direct write to the file is non-atomic and could cause data race conditions).
c.  if cache file doesn't exist (e.g., because 'rename' failed or because some other reasons), bypass using the cache altogether.


Repository:
  rL LLVM

https://reviews.llvm.org/D45076

Files:
  lib/LTO/ThinLTOCodeGenerator.cpp


Index: lib/LTO/ThinLTOCodeGenerator.cpp
===================================================================
--- lib/LTO/ThinLTOCodeGenerator.cpp
+++ lib/LTO/ThinLTOCodeGenerator.cpp
@@ -400,9 +400,12 @@
 
     // Write to a temporary to avoid race condition
     SmallString<128> TempFilename;
+    SmallString<128> CachePath(EntryPath);
     int TempFD;
-    std::error_code EC =
-        sys::fs::createTemporaryFile("Thin", "tmp.o", TempFD, TempFilename);
+    llvm::sys::path::remove_filename(CachePath);
+    sys::path::append(TempFilename, CachePath, "Thin-%%%%%%.tmp.o");
+    std::error_code EC = 
+      sys::fs::createUniqueFile(TempFilename, TempFD, TempFilename);
     if (EC) {
       errs() << "Error: " << EC.message() << "\n";
       report_fatal_error("ThinLTO: Can't get a temporary file");
@@ -415,11 +418,6 @@
     EC = sys::fs::rename(TempFilename, EntryPath);
     if (EC) {
       sys::fs::remove(TempFilename);
-      raw_fd_ostream OS(EntryPath, EC, sys::fs::F_None);
-      if (EC)
-        report_fatal_error(Twine("Failed to open ") + EntryPath +
-                           " to save cached entry\n");
-      OS << OutputBuffer.getBuffer();
     }
   }
 };
@@ -1027,18 +1025,14 @@
         if (SavedObjectsDirectoryPath.empty()) {
           // We need to generated a memory buffer for the linker.
           if (!CacheEntryPath.empty()) {
-            // Cache is enabled, reload from the cache
-            // We do this to lower memory pressuree: the buffer is on the heap
-            // and releasing it frees memory that can be used for the next input
-            // file. The final binary link will read from the VFS cache
-            // (hopefully!) or from disk if the memory pressure wasn't too high.
+            // When cache is enabled, reload from the cache if possible. 
+            // Releasing the buffer from the heap and reloading it from the
+            // cache file with mmap helps us to lower memory pressure. 
+            // The freed memory can be used for the next input file. 
+            // The final binary link will read from the VFS cache (hopefully!)
+            // or from disk (if the memory pressure was too high).
             auto ReloadedBufferOrErr = CacheEntry.tryLoadingBuffer();
-            if (auto EC = ReloadedBufferOrErr.getError()) {
-              // On error, keeping the preexisting buffer and printing a
-              // diagnostic is more friendly than just crashing.
-              errs() << "error: can't reload cached file '" << CacheEntryPath
-                     << "': " << EC.message() << "\n";
-            } else {
+            if (!ReloadedBufferOrErr.getError()) {
               OutputBuffer = std::move(*ReloadedBufferOrErr);
             }
           }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D45076.140378.patch
Type: text/x-patch
Size: 2758 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180330/bbe26641/attachment.bin>