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

[Katya Romanova via Phabricator via llvm-commits]

kromanova added inline comments.


================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:1033
+            // 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();
----------------
I reversed the meaning in the last sentence (wasn't too high -> was to high). Could you please double check that this is correct?


================
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);
----------------
steven_wu wrote:
> 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. 
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D45076