LTO: Fix a potential race condition in the caching API.

[pcc]

Summary

After the call to sys::fs::exists succeeds, indicating a cache hit, we call
AddFile and the client will open the file using the supplied path. If the
client is using cache pruning, there is a potential race between the pruner
and the client. To avoid this, change the caching API so that it provides
a MemoryBuffer to the client, and have clients use that MemoryBuffer where
possible.

This scheme won't work with the gold plugin because the plugin API expects a
file path. So we have the gold plugin use the buffer identifier as a path and
live with the race for now. (Note that the gold plugin isn't actually affected
by the problem at the moment because it doesn't support cache pruning.)

This effectively reverts r279883 modulo the change to use the existing path
in the gold plugin.

[mehdi_amini]

LGTM. Thanks!

[mehdi_amini]

llvm/tools/gold/gold-plugin.cpp

835 ↗

(On Diff #92088)

I may comment what's going-on here though, there is an implicit expectation that the API is always backed up by a file.

[uweigand]

It looks like this commit broke the SystemZ build bot:

http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/6102

• TEST 'LLVM :: ThinLTO/X86/cache-config.ll' FAILED ****

LLVM ERROR: Failed to open cache file /home/uweigand/sandbox/buildbot/clang-s390x-linux/stage1/test/ThinLTO/X86/Output/cache-config.ll.tmp.cache/88CBD6AEFE21EF8594951733D6A16FDEBB8AE7E5: No such file or directory

Any suggestions what might go wrong here? Using strace it appears that the file indeed does not exist:
[pid 54322] open("/home/uweigand/sandbox/buildbot/clang-s390x-linux/stage1/test/ThinLTO/X86/Output/cache-config.ll.tmp.cache/88CBD6AEFE21EF8594951733D6A16FDEBB8AE7E5", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)

Not sure what the expected behavior should have been here ...

[pcc]

Is there a rename syscall immediately before the open syscall? If that is the case, the fatal error is probably coming from Caching.cpp:73, which is unexpected because we just put a file in place with the correct name, so maybe something is going on with the filesystem on that machine?

The other alternative is that the error is coming from Caching.cpp:41, and in that case the file not existing is expected. If the file does not exist we are supposed to fail the if condition on line 40 and create a new cache entry.

The only explanation I can think of is that there is some sort of bug in your stdlib that causes the if condition to succeed. What happens if you apply this patch?

diff --git a/llvm/lib/LTO/Caching.cpp b/llvm/lib/LTO/Caching.cpp
index d8b91c48ee3..c24540e1860 100644
--- a/llvm/lib/LTO/Caching.cpp
+++ b/llvm/lib/LTO/Caching.cpp
@@ -37,7 +37,8 @@ Expected<NativeObjectCache> lto::localCache(StringRef CacheDirectoryPath,
       return AddStreamFn();
     }
 
-    if (MBOrErr.getError() != std::errc::no_such_file_or_directory)
+    if (MBOrErr.getError() != std::errc::no_such_file_or_directory &&
+        MBOrErr.getError().message() != "No such file or directory")
       report_fatal_error(Twine("Failed to open cache file ") + EntryPath +
                          ": " + MBOrErr.getError().message() + "\n");

[uweigand]

In D31063#704025, @pcc wrote:

The only explanation I can think of is that there is some sort of bug in your stdlib that causes the if condition to succeed.

Indeed, this does appear to be the case. I can reproduce the problem with this simple program:

#include <iostream>
#include <system_error>

int main() {
    std::error_code code = std::error_code(ENOENT, std::generic_category());
    if (code != std::errc::no_such_file_or_directory)
      std::cout << "comparison failed" << std::endl;
}

This fails on the build bot machine, but succeeds on my other development machines. It appears the problem is related to the fact on this machine (running SLES12), the system compiler is a gcc 4.8, but the system libstdc++ library is one from gcc 6. This of course is supposed to be a compatible combination, but apparently there is a problem when comparing error codes. The value is the same, but the category class pointer doesn't match, apparently because in the gcc 6 libstdc++ there are two flavours of those classes, and when mixing gcc 4.8 and gcc 6 code, sometimes wrong versions get compared ...

What happens if you apply this patch?

diff --git a/llvm/lib/LTO/Caching.cpp b/llvm/lib/LTO/Caching.cpp
index d8b91c48ee3..c24540e1860 100644
--- a/llvm/lib/LTO/Caching.cpp
+++ b/llvm/lib/LTO/Caching.cpp
@@ -37,7 +37,8 @@ Expected<NativeObjectCache> lto::localCache(StringRef CacheDirectoryPath,
       return AddStreamFn();
     }
 
-    if (MBOrErr.getError() != std::errc::no_such_file_or_directory)
+    if (MBOrErr.getError() != std::errc::no_such_file_or_directory &&
+        MBOrErr.getError().message() != "No such file or directory")
       report_fatal_error(Twine("Failed to open cache file ") + EntryPath +
                          ": " + MBOrErr.getError().message() + "\n");

This does fix the problem for me. Likewise the (maybe more straightforward) fix

if ((std::errc)MBOrErr.getError().value() != std::errc::no_such_file_or_directory)
  ...

I'll work with the distro to hopefully get this fixed, but maybe we can get a workaround along those lines installed so the build bot can continue to run in the meantime?

[pcc]

In D31063#704297, @uweigand wrote:

I'll work with the distro to hopefully get this fixed, but maybe we can get a workaround along those lines installed so the build bot can continue to run in the meantime?

I committed your proposed workaround in r298127.

[mehdi_amini]

I'm quite amazed how you guys narrowed this down so quickly! Well done...

[uweigand]

In D31063#704297, @uweigand wrote:

This fails on the build bot machine, but succeeds on my other development machines. It appears the problem is related to the fact on this machine (running SLES12), the system compiler is a gcc 4.8, but the system libstdc++ library is one from gcc 6. This of course is supposed to be a compatible combination, but apparently there is a problem when comparing error codes. The value is the same, but the category class pointer doesn't match, apparently because in the gcc 6 libstdc++ there are two flavours of those classes, and when mixing gcc 4.8 and gcc 6 code, sometimes wrong versions get compared ...

I'll work with the distro to hopefully get this fixed, but maybe we can get a workaround along those lines installed so the build bot can continue to run in the meantime?

Unfortunately it looks like this is a known issue; there are some C++11 related incompatibilities between the gcc 4.8 and gcc 6 versions of libstdc++. Since C++11 support in GCC 4.8 was considered experimental to begin with, there are no plans for fixing this at this point.

I guess that means we're stuck with the workaround (the latest iteration using LLVM's errc works fine).

lld/trunk/ELF/LTO.cpp

std::vector<InputFile *> BitcodeCompiler::compile() {
   Buff.resize(MaxTasks);
   Files.resize(MaxTasks);
 
   // The --thinlto-cache-dir option specifies the path to a directory in which
   // to cache native object files for ThinLTO incremental builds. If a path was
   // specified, configure LTO to use it as the cache directory.
   lto::NativeObjectCache Cache;
   if (!Config->ThinLTOCacheDir.empty())
-    Cache = check(lto::localCache(
-        Config->ThinLTOCacheDir, [&](size_t Task, StringRef Path) {
-          Files[Task] = check(MemoryBuffer::getFile(Path));
+    Cache = check(
+        lto::localCache(Config->ThinLTOCacheDir,
+                        [&](size_t Task, std::unique_ptr<MemoryBuffer> MB) {
+                          Files[Task] = std::move(MB);
                         }));
 
   checkError(LTOObj->run(
       [&](size_t Task) {
         return llvm::make_unique<lto::NativeObjectStream>(
             llvm::make_unique<raw_svector_ostream>(Buff[Task]));
       },
       Cache));
 

llvm/trunk/include/llvm/LTO/Caching.h

 #include <string>
 
 namespace llvm {
 namespace lto {
 
 /// This type defines the callback to add a pre-existing native object file
 /// (e.g. in a cache).
 ///
-/// File callbacks must be thread safe.
-typedef std::function<void(unsigned Task, StringRef Path)> AddFileFn;
+/// 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.
+///
+/// Buffer callbacks must be thread safe.
+typedef std::function<void(unsigned Task, std::unique_ptr<MemoryBuffer> MB)>
+    AddBufferFn;
 
 /// Create a local file system cache which uses the given cache directory and
 /// file callback. This function also creates the cache directory if it does not
 /// already exist.
 Expected<NativeObjectCache> localCache(StringRef CacheDirectoryPath,
-                                       AddFileFn AddFile);
+                                       AddBufferFn AddBuffer);
 
 } // namespace lto
 } // namespace llvm
 
 #endif

llvm/trunk/lib/LTO/Caching.cpp

 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
 using namespace llvm::lto;
 
 Expected<NativeObjectCache> lto::localCache(StringRef CacheDirectoryPath,
-                                            AddFileFn AddFile) {
+                                            AddBufferFn AddBuffer) {
   if (std::error_code EC = sys::fs::create_directories(CacheDirectoryPath))
     return errorCodeToError(EC);
 
   return [=](unsigned Task, StringRef Key) -> AddStreamFn {
     // First, see if we have a cache hit.
     SmallString<64> EntryPath;
     sys::path::append(EntryPath, CacheDirectoryPath, Key);
-    if (sys::fs::exists(EntryPath)) {
-      AddFile(Task, EntryPath);
+    ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr =
+        MemoryBuffer::getFile(EntryPath);
+    if (MBOrErr) {
+      AddBuffer(Task, std::move(*MBOrErr));
       return AddStreamFn();
     }
 
+    if (MBOrErr.getError() != std::errc::no_such_file_or_directory)
+      report_fatal_error(Twine("Failed to open cache file ") + EntryPath +
+                         ": " + MBOrErr.getError().message() + "\n");
+
     // This native object stream is responsible for commiting the resulting
-    // file to the cache and calling AddFile to add it to the link.
+    // file to the cache and calling AddBuffer to add it to the link.
     struct CacheStream : NativeObjectStream {
-      AddFileFn AddFile;
+      AddBufferFn AddBuffer;
       std::string TempFilename;
       std::string EntryPath;
       unsigned Task;
 
-      CacheStream(std::unique_ptr<raw_pwrite_stream> OS, AddFileFn AddFile,
+      CacheStream(std::unique_ptr<raw_pwrite_stream> OS, AddBufferFn AddBuffer,
                   std::string TempFilename, std::string EntryPath,
                   unsigned Task)
-          : NativeObjectStream(std::move(OS)), AddFile(std::move(AddFile)),
+          : NativeObjectStream(std::move(OS)), AddBuffer(std::move(AddBuffer)),
             TempFilename(std::move(TempFilename)),
             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.
         if (auto EC = sys::fs::rename(TempFilename, EntryPath))
           report_fatal_error(Twine("Failed to rename temporary file ") +
                              TempFilename + ": " + EC.message() + "\n");
-        AddFile(Task, EntryPath);
+
+        ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr =
+            MemoryBuffer::getFile(EntryPath);
+        if (!MBOrErr)
+          report_fatal_error(Twine("Failed to open cache file ") + EntryPath +
+                             ": " + MBOrErr.getError().message() + "\n");
+        AddBuffer(Task, std::move(*MBOrErr));
       }
     };
 
     return [=](size_t Task) -> std::unique_ptr<NativeObjectStream> {
       // Write to a temporary to avoid race condition
       int TempFD;
       SmallString<64> TempFilenameModel, TempFilename;
       sys::path::append(TempFilenameModel, CacheDirectoryPath, "Thin-%%%%%%.tmp.o");
       std::error_code EC =
           sys::fs::createUniqueFile(TempFilenameModel, TempFD, TempFilename,
                                     sys::fs::owner_read | sys::fs::owner_write);
       if (EC) {
         errs() << "Error: " << EC.message() << "\n";
         report_fatal_error("ThinLTO: Can't get a temporary file");
       }
 
       // This CacheStream will move the temporary file into the cache when done.
       return llvm::make_unique<CacheStream>(
           llvm::make_unique<raw_fd_ostream>(TempFD, /* ShouldClose */ true),
-          AddFile, TempFilename.str(), EntryPath.str(), Task);
+          AddBuffer, TempFilename.str(), EntryPath.str(), Task);
     };
   };
 }

llvm/trunk/tools/gold/gold-plugin.cpp

std::error_code EC =
         sys::fs::openFileForWrite(Filenames[Task], FD, sys::fs::F_None);
     if (EC)
       message(LDPL_FATAL, "Could not open file %s: %s", Filenames[Task].c_str(),
               EC.message().c_str());
     return llvm::make_unique<lto::NativeObjectStream>(
         llvm::make_unique<llvm::raw_fd_ostream>(FD, true));
   };
 
-  auto AddFile = [&](size_t Task, StringRef Path) { Filenames[Task] = Path; };
+  auto AddBuffer = [&](size_t Task, std::unique_ptr<MemoryBuffer> MB) {
+    // Note that this requires that the memory buffers provided to AddBuffer are
+    // backed by a file.
+    Filenames[Task] = MB->getBufferIdentifier();
+  };
 
   NativeObjectCache Cache;
   if (!options::cache_dir.empty())
-    Cache = check(localCache(options::cache_dir, AddFile));
+    Cache = check(localCache(options::cache_dir, AddBuffer));
 
   check(Lto->run(AddStream, Cache));
 
   if (options::TheOutputType == options::OT_DISABLE ||
       options::TheOutputType == options::OT_BC_ONLY)
     return LDPS_OK;
 
   if (options::thinlto_index_only) {

llvm/trunk/tools/llvm-lto2/llvm-lto2.cpp

auto AddStream =
     std::string Path = OutputFilename + "." + utostr(Task);
 
     std::error_code EC;
     auto S = llvm::make_unique<raw_fd_ostream>(Path, EC, sys::fs::F_None);
     check(EC, Path);
     return llvm::make_unique<lto::NativeObjectStream>(std::move(S));
   };
 
-  auto AddFile = [&](size_t Task, StringRef Path) {
-    auto ReloadedBufferOrErr = MemoryBuffer::getFile(Path);
-    if (auto EC = ReloadedBufferOrErr.getError())
-      report_fatal_error(Twine("Can't reload cached file '") + Path + "': " +
-                         EC.message() + "\n");
-
-    *AddStream(Task)->OS << (*ReloadedBufferOrErr)->getBuffer();
+  auto AddBuffer = [&](size_t Task, std::unique_ptr<MemoryBuffer> MB) {
+    *AddStream(Task)->OS << MB->getBuffer();
   };
 
   NativeObjectCache Cache;
   if (!CacheDir.empty())
-    Cache = check(localCache(CacheDir, AddFile), "failed to create cache");
+    Cache = check(localCache(CacheDir, AddBuffer), "failed to create cache");
 
   check(Lto.run(AddStream, Cache), "LTO::run failed");
 }