Prevent data races in concurrent ThinLTO processes

[kromanova]

Summary

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.

[steven_wu]

LGTM with some feedback inline.

lib/LTO/ThinLTOCodeGenerator.cpp

417 ↗

(On Diff #140378)

Can you rewrite this comment?

419 ↗

(On Diff #140378)

You can remove {}

1035 ↗

(On Diff #140378)

You also don't need {} here

Is there any specific reason why the error message is removed here? We should not hit that if we fixed all the races, but it can still happen from user errors.

[kromanova]

Fixed code review comments from Steven Wu.

lib/LTO/ThinLTOCodeGenerator.cpp

1033 ↗

(On Diff #140378)

I reversed the meaning in the last sentence (wasn't too high -> was to high). Could you please double check that this is correct?

1035 ↗

(On Diff #140378)

I restored the code the way it was before my change, except I modified the comment slightly. I don't understand what crashing the comment is referring to, so I removed it.
This error message is just a diagnostic printed on the stderr, it won't stop the compilation, right? We want the compilation to continue even if the error occurred.

[steven_wu]

LGTM. Thanks!

lib/LTO/ThinLTOCodeGenerator.cpp

1035 ↗

(On Diff #140378)

Looks correct. This is simply printing to stderr.

llvm/trunk/lib/LTO/ThinLTOCodeGenerator.cpp

#endif
 
   // Cache the Produced object file
   void write(const MemoryBuffer &OutputBuffer) {
     if (EntryPath.empty())
       return;
 
     // 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");
     }
     {
       raw_fd_ostream OS(TempFD, /* ShouldClose */ true);
       OS << OutputBuffer.getBuffer();
     }
-    // Rename to final destination (hopefully race condition won't matter here)
+    // Rename temp file to final destination; rename is atomic 
     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();
-    }
+      sys::fs::remove(TempFilename);
   }
 };
 
 static std::unique_ptr<MemoryBuffer>
 ProcessThinLTOModule(Module &TheModule, ModuleSummaryIndex &Index,
                      StringMap<MemoryBufferRef> &ModuleMap, TargetMachine &TM,
                      const FunctionImporter::ImportMapTy &ImportList,
                      const FunctionImporter::ExportSetTy &ExportList,
for (auto IndexCount : ModulesOrdering) {
             DisableCodeGen, SaveTempsDir, Freestanding, OptLevel, count);
 
         // Commit to the cache (if enabled)
         CacheEntry.write(*OutputBuffer);
 
         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.
+              // On error, keep the preexisting buffer and print a diagnostic.
               errs() << "error: can't reload cached file '" << CacheEntryPath
                      << "': " << EC.message() << "\n";
             } else {
               OutputBuffer = std::move(*ReloadedBufferOrErr);
             }
           }
           ProducedBinaries[count] = std::move(OutputBuffer);
           return;