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

[Steven Wu via Phabricator via llvm-commits]

steven_wu added a comment.

LGTM with some feedback inline.



================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:417
     }
     // Rename to final destination (hopefully race condition won't matter here)
     EC = sys::fs::rename(TempFilename, EntryPath);
----------------
Can you rewrite this comment?


================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:419
     EC = sys::fs::rename(TempFilename, EntryPath);
     if (EC) {
       sys::fs::remove(TempFilename);
----------------
You can remove {}


================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:1035
             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);
----------------
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. 


Repository:
  rL LLVM

https://reviews.llvm.org/D45076