Create a TempFile class

[• rafael]

Summary

This just adds a TempFile class and replaces the use in FileOutputBuffer with it.

The only difference for now is better error handling. My idea is to once this is in:

• Convert other user of temporary files to it.
• Add support for automatically deleting on windows.
• Add a createUnnamed method that returns a potentially unnamed file. It would be actually unnamed on modern linux and have a unknown name on windows.

[rnk]

include/llvm/Support/FileSystem.h

698

Some basic Doxygen here would be good.

703

Basic doxygen, probably just referring the reader to createUniqueFile.

711

= -1, just in case we forget to initialize FD one day.

lib/Support/Path.cpp

770

Should this attempt to discard the file if it wasn't committed? Otherwise users may need to invent RAII things like:

DiscardingTempFile : TempFile {
  ~DiscardingTempFile() { if (!Done) consumeError(discard()); }
};

I guess if we leave it this way, users are discouraged from doing that.

[• rafael]

Add comments.

[rnk]

Looks good

[ruiu]

lib/Support/Path.cpp

773–774

I think that calling keep or Done more than once on an object should be an error because I can't think of a use case in which allowing it is useful.

[• rafael]

r318069 .

include/llvm/Support/FileSystem.h

 
 #ifndef LLVM_SUPPORT_FILESYSTEM_H
 #define LLVM_SUPPORT_FILESYSTEM_H
 
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/Chrono.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/MD5.h"
 #include <cassert>
 #include <cstdint>
 #include <ctime>
 #include <memory>
 #include <stack>

 std::error_code createUniqueFile(const Twine &Model, int &ResultFD,
                                  SmallVectorImpl<char> &ResultPath,
                                  unsigned Mode = all_read | all_write);
 
 /// @brief Simpler version for clients that don't want an open file.
 std::error_code createUniqueFile(const Twine &Model,
                                  SmallVectorImpl<char> &ResultPath);
 
+/// Represents a temporary file.
+///
+/// The temporary file must be eventually discarded or given a final name and
+/// kept.
+///
+/// The destructor doesn't implicitly discard because there is no way to properly
+/// handle errors in a destructor.
+class TempFile {
+  bool Done = false;
+  TempFile(StringRef Name, int FD);
+
+public:
+  /// This creates a temporary file with createUniqueFile and schedules it for
+  /// deletion with sys::RemoveFileOnSignal.
+  static Expected<TempFile> create(const Twine &Model,
+                                   unsigned Mode = all_read | all_write);
+  TempFile(TempFile &&Other);
+
+  // Name of the temporary file.
+  std::string TmpName;
+
+  // The open file descriptor.
+  int FD = -1;
+
+  // Keep this with the given name.
+  Error keep(const Twine &Name);
+
+  // Delete the file.
+  Error discard();
+
+  // This checks that keep or delete was called.
+  ~TempFile();
+};
+
 /// @brief Create a file in the system temporary directory.
 ///
 /// The filename is of the form prefix-random_chars.suffix. Since the directory
 /// is not know to the caller, Prefix and Suffix cannot have path separators.
 /// The files are created with mode 0600.
 ///
 /// This should be used for things like a temporary .s that is removed after
 /// running the assembler.

lib/Support/FileOutputBuffer.cpp

 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/FileOutputBuffer.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Memory.h"
 #include "llvm/Support/Path.h"
-#include "llvm/Support/Signals.h"
 #include <system_error>
 
 #if !defined(_MSC_VER) && !defined(__MINGW32__)
 #include <unistd.h>
 #else
 #include <io.h>
 #endif
 
 using namespace llvm;
 using namespace llvm::sys;
 
 // A FileOutputBuffer which creates a temporary file in the same directory
 // as the final output file. The final output file is atomically replaced
 // with the temporary file on commit().
 class OnDiskBuffer : public FileOutputBuffer {
 public:
-  OnDiskBuffer(StringRef Path, StringRef TempPath,
+  OnDiskBuffer(StringRef Path, fs::TempFile Temp,
                std::unique_ptr<fs::mapped_file_region> Buf)
-      : FileOutputBuffer(Path), Buffer(std::move(Buf)), TempPath(TempPath) {}
+      : FileOutputBuffer(Path), Buffer(std::move(Buf)), Temp(std::move(Temp)) {}
 
   uint8_t *getBufferStart() const override { return (uint8_t *)Buffer->data(); }
 
   uint8_t *getBufferEnd() const override {
     return (uint8_t *)Buffer->data() + Buffer->size();
   }
 
   size_t getBufferSize() const override { return Buffer->size(); }
 
   Error commit() override {
     // Unmap buffer, letting OS flush dirty pages to file on disk.
     Buffer.reset();
 
     // Atomically replace the existing file with the new one.
-    auto EC = fs::rename(TempPath, FinalPath);
-    sys::DontRemoveFileOnSignal(TempPath);
-    return errorCodeToError(EC);
+    return Temp.keep(FinalPath);
   }
 
   ~OnDiskBuffer() override {
     // Close the mapping before deleting the temp file, so that the removal
     // succeeds.
     Buffer.reset();
-    fs::remove(TempPath);
+    consumeError(Temp.discard());
   }
 
 private:
   std::unique_ptr<fs::mapped_file_region> Buffer;
-  std::string TempPath;
+  fs::TempFile Temp;
 };
 
 // A FileOutputBuffer which keeps data in memory and writes to the final
 // output file on commit(). This is used only when we cannot use OnDiskBuffer.
 class InMemoryBuffer : public FileOutputBuffer {
 public:
   InMemoryBuffer(StringRef Path, MemoryBlock Buf, unsigned Mode)
       : FileOutputBuffer(Path), Buffer(Buf), Mode(Mode) {}
MemoryBlock MB = Memory::allocateMappedMemory(
       Size, nullptr, sys::Memory::MF_READ | sys::Memory::MF_WRITE, EC);
   if (EC)
     return errorCodeToError(EC);
   return llvm::make_unique<InMemoryBuffer>(Path, MB, Mode);
 }
 
 static Expected<std::unique_ptr<OnDiskBuffer>>
 createOnDiskBuffer(StringRef Path, size_t Size, unsigned Mode) {
-  // Create new file in same directory but with random name.
-  SmallString<128> TempPath;
-  int FD;
-  if (auto EC = fs::createUniqueFile(Path + ".tmp%%%%%%%", FD, TempPath, Mode))
-    return errorCodeToError(EC);
-
-  sys::RemoveFileOnSignal(TempPath);
+  Expected<fs::TempFile> FileOrErr =
+      fs::TempFile::create(Path + ".tmp%%%%%%%", Mode);
+  if (!FileOrErr)
+    return FileOrErr.takeError();
+  fs::TempFile File = std::move(*FileOrErr);
 
 #ifndef LLVM_ON_WIN32
   // On Windows, CreateFileMapping (the mmap function on Windows)
   // automatically extends the underlying file. We don't need to
   // extend the file beforehand. _chsize (ftruncate on Windows) is
   // pretty slow just like it writes specified amount of bytes,
   // so we should avoid calling that function.
-  if (auto EC = fs::resize_file(FD, Size))
+  if (auto EC = fs::resize_file(File.FD, Size)) {
+    consumeError(File.discard());
     return errorCodeToError(EC);
+  }
 #endif
 
   // Mmap it.
   std::error_code EC;
   auto MappedFile = llvm::make_unique<fs::mapped_file_region>(
-      FD, fs::mapped_file_region::readwrite, Size, 0, EC);
-  close(FD);
-  if (EC)
+      File.FD, fs::mapped_file_region::readwrite, Size, 0, EC);
+  if (EC) {
+    consumeError(File.discard());
     return errorCodeToError(EC);
-  return llvm::make_unique<OnDiskBuffer>(Path, TempPath, std::move(MappedFile));
+  }
+  return llvm::make_unique<OnDiskBuffer>(Path, std::move(File),
+                                         std::move(MappedFile));
 }
 
 // Create an instance of FileOutputBuffer.
 Expected<std::unique_ptr<FileOutputBuffer>>
 FileOutputBuffer::create(StringRef Path, size_t Size, unsigned Flags) {
   unsigned Mode = fs::all_read | fs::all_write;
   if (Flags & F_executable)
     Mode |= fs::all_exe;

lib/Support/Path.cpp

 
 #include "llvm/Support/Path.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Process.h"
+#include "llvm/Support/Signals.h"
 #include <cctype>
 #include <cstring>
 
 #if !defined(_MSC_VER) && !defined(__MINGW32__)
 #include <unistd.h>
 #else
 #include <io.h>
 #endif

 }
 
 std::error_code createUniqueFile(const Twine &Model,
                                  SmallVectorImpl<char> &ResultPath) {
   int Dummy;
   return createUniqueEntity(Model, Dummy, ResultPath, false, 0, FS_Name);
 }
 
+TempFile::TempFile(StringRef Name, int FD) : TmpName(Name), FD(FD) {}
+TempFile::TempFile(TempFile &&Other) {
+  TmpName = std::move(Other.TmpName);
+  FD = Other.FD;
+  Other.Done = true;
+}
+
+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);
+  sys::DontRemoveFileOnSignal(TmpName);
+  if (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 (close(FD) == -1) {
+    std::error_code EC(errno, std::generic_category());
+    return errorCodeToError(EC);
+  }
+  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);
+
+  // Make sure we delete the file when RemoveFileOnSignal fails.
+  TempFile Ret(ResultPath, FD);
+  if (sys::RemoveFileOnSignal(ResultPath)) {
+    consumeError(Ret.discard());
+    std::error_code EC(errc::operation_not_permitted);
+    return errorCodeToError(EC);
+  }
+  return std::move(Ret);
+}
+
 static std::error_code
 createTemporaryFile(const Twine &Model, int &ResultFD,
                     llvm::SmallVectorImpl<char> &ResultPath, FSEntity Type) {
   SmallString<128> Storage;
   StringRef P = Model.toNullTerminatedStringRef(Storage);
   assert(P.find_first_of(separators(Style::native)) == StringRef::npos &&
          "Model must be a simple filename.");
   // Use P.begin() so that createUniqueEntity doesn't need to recreate Storage.