Use TempFile in lto caching

[• rafael]

Summary

This requires a small change to TempFile: allowing a discard after a failed keep.

With this the cache now handles signals and reuses a fd instead of reopening the file.

[mehdi_amini]

LGTM

lib/Support/Path.cpp

802

No else after return

[• rafael]

318322

lib/LTO/Caching.cpp

return [=](unsigned Task, StringRef Key) -> AddStreamFn {
     if (MBOrErr.getError() != 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 AddBuffer to add it to the link.
     struct CacheStream : NativeObjectStream {
       AddBufferFn AddBuffer;
-      std::string TempFilename;
+      sys::fs::TempFile TempFile;
       std::string EntryPath;
       unsigned Task;
 
       CacheStream(std::unique_ptr<raw_pwrite_stream> OS, AddBufferFn AddBuffer,
-                  std::string TempFilename, std::string EntryPath,
+                  sys::fs::TempFile TempFile, std::string EntryPath,
                   unsigned Task)
           : NativeObjectStream(std::move(OS)), AddBuffer(std::move(AddBuffer)),
-            TempFilename(std::move(TempFilename)),
-            EntryPath(std::move(EntryPath)), Task(Task) {}
+            TempFile(std::move(TempFile)), EntryPath(std::move(EntryPath)),
+            Task(Task) {}
 
       ~CacheStream() {
-        // Make sure the file is closed before committing it.
+        // Make sure the stream is closed before committing it.
         OS.reset();
 
         // Open the file first to avoid racing with a cache pruner.
         ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr =
-            MemoryBuffer::getFile(TempFilename);
+            MemoryBuffer::getOpenFile(TempFile.FD, TempFile.TmpName,
+                                      /*FileSize*/ -1,
+                                      /*RequiresNullTerminator*/ false);
         if (!MBOrErr)
           report_fatal_error(Twine("Failed to open new cache file ") +
-                             TempFilename + ": " +
+                             TempFile.TmpName + ": " +
                              MBOrErr.getError().message() + "\n");
 
         // This is atomic on POSIX systems.
         // On Windows, it can fail with permission denied if the destination
         // file already exists. Since the existing file should be semantically
         // equivalent to the one we are trying to write, we give AddBuffer
         // a copy of the bytes we wrote in that case. We do this instead of
         // just using the existing file, because the pruner might delete the
         // file before we get a chance to use it.
-        auto EC = sys::fs::rename(TempFilename, EntryPath);
-        if (EC == errc::permission_denied) {
-          auto MBCopy = MemoryBuffer::getMemBufferCopy(
-              (*MBOrErr)->getBuffer(), EntryPath);
+        Error E = TempFile.keep(EntryPath);
+        E = handleErrors(std::move(E), [&](const ECError &E) -> Error {
+          std::error_code EC = E.convertToErrorCode();
+          if (EC != errc::permission_denied)
+            return errorCodeToError(EC);
+
+          auto MBCopy = MemoryBuffer::getMemBufferCopy((*MBOrErr)->getBuffer(),
+                                                       EntryPath);
           MBOrErr = std::move(MBCopy);
-          sys::fs::remove(TempFilename);
-        } else if (EC) {
+
+          // FIXME: should we consume the discard error?
+          consumeError(TempFile.discard());
+
+          return Error::success();
+        });
+
+        if (E)
           report_fatal_error(Twine("Failed to rename temporary file ") +
-                             TempFilename + " to " + EntryPath + ": " +
-                             EC.message() + "\n");
-        }
+                             TempFile.TmpName + " to " + EntryPath + ": " +
+                             toString(std::move(E)) + "\n");
 
         AddBuffer(Task, std::move(*MBOrErr), EntryPath);
       }
     };
 
     return [=](size_t Task) -> std::unique_ptr<NativeObjectStream> {
       // Write to a temporary to avoid race condition
-      int TempFD;
-      SmallString<64> TempFilenameModel, TempFilename;
+      SmallString<64> TempFilenameModel;
       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";
+      Expected<sys::fs::TempFile> Temp = sys::fs::TempFile::create(
+          TempFilenameModel, sys::fs::owner_read | sys::fs::owner_write);
+      if (!Temp) {
+        errs() << "Error: " << toString(Temp.takeError()) << "\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),
-          AddBuffer, TempFilename.str(), EntryPath.str(), Task);
+          llvm::make_unique<raw_fd_ostream>(Temp->FD, /* ShouldClose */ false),
+          AddBuffer, std::move(*Temp), EntryPath.str(), Task);
     };
   };
 }

lib/Support/Path.cpp

TempFile &TempFile::operator=(TempFile &&Other) {
   FD = Other.FD;
   Other.Done = true;
   return *this;
 }
 
 TempFile::~TempFile() { assert(Done); }
 
 Error TempFile::discard() {
-  if (Done)
-    return Error::success();
   Done = true;
   // Always try to close and remove.
-  std::error_code RemoveEC = fs::remove(TmpName);
+  std::error_code RemoveEC;
+  if (!TmpName.empty()) {
+    RemoveEC = fs::remove(TmpName);
     sys::DontRemoveFileOnSignal(TmpName);
-  if (close(FD) == -1) {
+  }
+  if (FD != -1 && close(FD) == -1) {
     std::error_code EC = std::error_code(errno, std::generic_category());
     return errorCodeToError(EC);
   }
   return errorCodeToError(RemoveEC);
 }
 
 Error TempFile::keep(const Twine &Name) {
   assert(!Done);
   Done = true;
   // Always try to close and rename.
   std::error_code RenameEC = fs::rename(TmpName, Name);
   sys::DontRemoveFileOnSignal(TmpName);
+
+  if (!RenameEC)
+    TmpName = "";
+
   if (close(FD) == -1) {
     std::error_code EC(errno, std::generic_category());
     return errorCodeToError(EC);
+  } else {
+    FD = -1;
   }
+
   return errorCodeToError(RenameEC);
 }
 
 Expected<TempFile> TempFile::create(const Twine &Model, unsigned Mode) {
   int FD;
   SmallString<128> ResultPath;
   if (std::error_code EC = createUniqueFile(Model, FD, ResultPath, Mode))
     return errorCodeToError(EC);