Rewrite FileOutputBuffer as two separate classes.

[ruiu]

Summary

This patch is to rewrite FileOutputBuffer as two separate classes;
one for file-backed output buffer and the other for memory-backed
output buffer. I think the new code is easier to follow because two
different implementations are now actually separated as different
classes.

Unlike the previous implementation, the class that does not replace the
final output file using rename(2) does not create a temporary file at
all. Instead, it allocates memory using mmap(2) and use it. I think
this is an improvement because it is now guaranteed that the temporary
memory region doesn't trigger any I/O and there's now zero chance to
leave a temporary file behind. Also, it shouldn't impose new restrictions
because were using mmap IO too.

[zturner]

llvm/lib/Support/FileOutputBuffer.cpp

84–85 ↗

(On Diff #120931)

Why can't we just use new here?

145 ↗

(On Diff #120931)

This is a bit confusing, in the sense that the function is called isFileRegular but then it can return true even if is_regular_file returns false.

170 ↗

(On Diff #120931)

It seems like we can just inline this.

fs::file_status Stat;
if (auto EC = fs::status(Path, Stat))
  return EC;
if (fs::is_directory(Stat))
  return errc::is_a_directory;

if (!fs::exists(Stat) || fs::is_regular_file(Stat))
  return OnDiskBuffer::create(Path, Size, Mode);
return InMemoryBuffer::create(Path, Size, Mode);

the extra function here just confuses things IMO.

That said, if it reaches the InMemoryBuffer code path, then what exactly *is* Path? It definitely exists, but it's neither a file nor a directory. So it's probably either a symlink, block file, character file, fifo, socket, or unknown file. Why would we be want to overwrite one of those with something?

[ruiu]

llvm/lib/Support/FileOutputBuffer.cpp

84–85 ↗

(On Diff #120931)

I didn't design Memory class, but it looks like that class can be instantiated only via allocateMappedMemory.

170 ↗

(On Diff #120931)

Thank you for the suggestion. Your code looks indeed better. Updated the patch. Also added comment to answer your question.

[ruiu]

• Address Zach's comments

[ruiu]

• Fix typo

[zturner]

llvm/lib/Support/FileOutputBuffer.cpp

62 ↗

(On Diff #121016)

Maybe want to add override to this and the other class's destructor

84–85 ↗

(On Diff #120931)

Right, but why do we even need to use the Memory class at all? It seems like overkill. The comments say:

/// This method allocates a block of memory that is suitable for loading
/// dynamically generated code (e.g. JIT).

Since we don't care about the protection (RWX) of the temporary storage, and only about the protection of the final file on disk, it seems like this is unnecessary.

Instead of storing OwningMemoryBlock as a member of the class, you could just store std::vector<uint8_t>.

There are fewer failure paths this way, as it's literally just one call to new, whereas allocateMappedMemory has several ways it can return errors.

[ruiu]

llvm/lib/Support/FileOutputBuffer.cpp

84–85 ↗

(On Diff #120931)

How can you catch memory exhaustion error if you use new?

[zturner]

llvm/lib/Support/FileOutputBuffer.cpp

84–85 ↗

(On Diff #120931)

Hmm, yea. I guess that's a problem especially since file can be really large.

Honestly I've always had a strong dislike for how in LLVM we write to files by mapping the entire contents into memory and then writing to it.

There was actually a bug just yesterday because of this exact reason, where someone was trying to write a TAR file > 4GB, and so a bunch of hacks have to be added to lit to deal with the address space of the process. If something is a file, you should just be able to write to it. Especially in this case, where "writing directly to the underlying file" (as opposed to writing to a separate file first and then renaming) is actually the desired behavior.

Anyway, I guess for now since we need to handle OOM error this is fine.

llvm/trunk/include/llvm/Support/FileOutputBuffer.h

 /// buffer which will be written to a file. During the lifetime of these
 /// objects, the content or existence of the specified file is undefined. That
 /// is, creating an OutputBuffer for a file may immediately remove the file.
 /// If the FileOutputBuffer is committed, the target file's content will become
 /// the buffer content at the time of the commit.  If the FileOutputBuffer is
 /// not committed, the file will be deleted in the FileOutputBuffer destructor.
 class FileOutputBuffer {
 public:
-
   enum  {
     F_executable = 1  /// set the 'x' bit on the resulting file
   };
 
   /// Factory method to create an OutputBuffer object which manages a read/write
   /// buffer of the specified size. When committed, the buffer will be written
   /// to the file at the specified path.
   static ErrorOr<std::unique_ptr<FileOutputBuffer>>
   create(StringRef FilePath, size_t Size, unsigned Flags = 0);
 
   /// Returns a pointer to the start of the buffer.
-  uint8_t *getBufferStart() {
-    return (uint8_t*)Region->data();
-  }
+  virtual uint8_t *getBufferStart() const = 0;
 
   /// Returns a pointer to the end of the buffer.
-  uint8_t *getBufferEnd() {
-    return (uint8_t*)Region->data() + Region->size();
-  }
+  virtual uint8_t *getBufferEnd() const = 0;
 
   /// Returns size of the buffer.
-  size_t getBufferSize() const {
-    return Region->size();
-  }
+  virtual size_t getBufferSize() const = 0;
 
   /// Returns path where file will show up if buffer is committed.
-  StringRef getPath() const {
-    return FinalPath;
-  }
+  StringRef getPath() const { return FinalPath; }
 
   /// Flushes the content of the buffer to its file and deallocates the
   /// buffer.  If commit() is not called before this object's destructor
   /// is called, the file is deleted in the destructor. The optional parameter
   /// is used if it turns out you want the file size to be smaller than
   /// initially requested.
-  std::error_code commit();
+  virtual std::error_code commit() = 0;
 
   /// If this object was previously committed, the destructor just deletes
   /// this object.  If this object was not committed, the destructor
   /// deallocates the buffer and the target file is never written.
-  ~FileOutputBuffer();
+  virtual ~FileOutputBuffer() {}
+
+protected:
+  FileOutputBuffer(StringRef Path) : FinalPath(Path) {}
 
-private:
-  FileOutputBuffer(const FileOutputBuffer &) = delete;
-  FileOutputBuffer &operator=(const FileOutputBuffer &) = delete;
-
-  FileOutputBuffer(std::unique_ptr<llvm::sys::fs::mapped_file_region> R,
-                   StringRef Path, StringRef TempPath, bool IsRegular);
-
-  std::unique_ptr<llvm::sys::fs::mapped_file_region> Region;
-  SmallString<128>    FinalPath;
-  SmallString<128>    TempPath;
-  bool IsRegular;
+  std::string FinalPath;
 };
 } // end namespace llvm
 
 #endif

llvm/trunk/lib/Support/FileOutputBuffer.cpp

 // Utility for creating a in-memory buffer that will be written to a file.
 //
 //===----------------------------------------------------------------------===//
 
 #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 llvm::sys::fs::mapped_file_region;
+using namespace llvm;
+using namespace llvm::sys;
 
-namespace llvm {
-FileOutputBuffer::FileOutputBuffer(std::unique_ptr<mapped_file_region> R,
-                                   StringRef Path, StringRef TmpPath,
-                                   bool IsRegular)
-    : Region(std::move(R)), FinalPath(Path), TempPath(TmpPath),
-      IsRegular(IsRegular) {}
+// 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,
+               std::unique_ptr<fs::mapped_file_region> Buf)
+      : FileOutputBuffer(Path), Buffer(std::move(Buf)), TempPath(TempPath) {}
 
-FileOutputBuffer::~FileOutputBuffer() {
+  static ErrorOr<std::unique_ptr<OnDiskBuffer>>
+  create(StringRef Path, size_t Size, unsigned Mode);
+
+  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(); }
+
+  std::error_code 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 EC;
+  }
+
+  ~OnDiskBuffer() override {
     // Close the mapping before deleting the temp file, so that the removal
     // succeeds.
-  Region.reset();
-  sys::fs::remove(Twine(TempPath));
+    Buffer.reset();
+    fs::remove(TempPath);
   }
 
-ErrorOr<std::unique_ptr<FileOutputBuffer>>
-FileOutputBuffer::create(StringRef FilePath, size_t Size, unsigned Flags) {
-  // Check file is not a regular file, in which case we cannot remove it.
-  sys::fs::file_status Stat;
-  std::error_code EC = sys::fs::status(FilePath, Stat);
-  bool IsRegular = true;
-  switch (Stat.type()) {
-    case sys::fs::file_type::file_not_found:
-      // If file does not exist, we'll create one.
-      break;
-    case sys::fs::file_type::regular_file: {
-        // If file is not currently writable, error out.
-        // FIXME: There is no sys::fs:: api for checking this.
-        // FIXME: In posix, you use the access() call to check this.
-      }
-      break;
-    case sys::fs::file_type::directory_file:
-      return errc::is_a_directory;
-    default:
-      if (EC)
-        return EC;
-      IsRegular = false;
+private:
+  std::unique_ptr<fs::mapped_file_region> Buffer;
+  std::string TempPath;
+};
+
+// 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) {}
+
+  static ErrorOr<std::unique_ptr<InMemoryBuffer>>
+  create(StringRef Path, size_t Size, unsigned Mode) {
+    std::error_code EC;
+    MemoryBlock MB = Memory::allocateMappedMemory(
+        Size, nullptr, sys::Memory::MF_READ | sys::Memory::MF_WRITE, EC);
+    return llvm::make_unique<InMemoryBuffer>(Path, MB, Mode);
+  }
+
+  uint8_t *getBufferStart() const override { return (uint8_t *)Buffer.base(); }
+
+  uint8_t *getBufferEnd() const override {
+    return (uint8_t *)Buffer.base() + Buffer.size();
   }
 
-  SmallString<128> TempFilePath;
+  size_t getBufferSize() const override { return Buffer.size(); }
+
+  std::error_code commit() override {
     int FD;
-  if (IsRegular) {
-    unsigned Mode = sys::fs::all_read | sys::fs::all_write;
-    // If requested, make the output file executable.
-    if (Flags & F_executable)
-      Mode |= sys::fs::all_exe;
-    // Create new file in same directory but with random name.
-    EC = sys::fs::createUniqueFile(Twine(FilePath) + ".tmp%%%%%%%", FD,
-                                   TempFilePath, Mode);
-  } else {
-    // Create a temporary file. Since this is a special file, we will not move
-    // it and the new file can be in another filesystem. This avoids trying to
-    // create a temporary file in /dev when outputting to /dev/null for example.
-    EC = sys::fs::createTemporaryFile(sys::path::filename(FilePath), "", FD,
-                                      TempFilePath);
+    std::error_code EC;
+    if (auto EC = openFileForWrite(FinalPath, FD, fs::F_None, Mode))
+      return EC;
+    raw_fd_ostream OS(FD, /*shouldClose=*/true, /*unbuffered=*/true);
+    OS << StringRef((const char *)Buffer.base(), Buffer.size());
+    return std::error_code();
   }
 
-  if (EC)
+private:
+  OwningMemoryBlock Buffer;
+  unsigned Mode;
+};
+
+ErrorOr<std::unique_ptr<OnDiskBuffer>>
+OnDiskBuffer::create(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 EC;
 
-  sys::RemoveFileOnSignal(TempFilePath);
+  sys::RemoveFileOnSignal(TempPath);
 
 #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.
-  EC = sys::fs::resize_file(FD, Size);
-  if (EC)
+  // so we should avoid calling that function.
+  if (auto EC = fs::resize_file(FD, Size))
     return EC;
 #endif
 
-  auto MappedFile = llvm::make_unique<mapped_file_region>(
-      FD, mapped_file_region::readwrite, Size, 0, EC);
-  int Ret = close(FD);
+  // 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)
     return EC;
-  if (Ret)
-    return std::error_code(errno, std::generic_category());
-
-  std::unique_ptr<FileOutputBuffer> Buf(new FileOutputBuffer(
-      std::move(MappedFile), FilePath, TempFilePath, IsRegular));
-  return std::move(Buf);
+  return llvm::make_unique<OnDiskBuffer>(Path, TempPath, std::move(MappedFile));
 }
 
-std::error_code FileOutputBuffer::commit() {
-  // Unmap buffer, letting OS flush dirty pages to file on disk.
-  Region.reset();
+// Create an instance of FileOutputBuffer.
+ErrorOr<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;
 
-  std::error_code EC;
-  if (IsRegular) {
-    // Atomically replace the existing file with the new one.
-    EC = sys::fs::rename(Twine(TempPath), Twine(FinalPath));
-    sys::DontRemoveFileOnSignal(TempPath);
-  } else {
-    EC = sys::fs::copy_file(TempPath, FinalPath);
-    std::error_code RMEC = sys::fs::remove(TempPath);
-    sys::DontRemoveFileOnSignal(TempPath);
-    if (RMEC)
-      return RMEC;
-  }
+  fs::file_status Stat;
+  fs::status(Path, Stat);
 
-  return EC;
+  // Usually, we want to create OnDiskBuffer to create a temporary file in
+  // the same directory as the destination file and atomically replaces it
+  // by rename(2).
+  //
+  // However, if the destination file is a special file, we don't want to
+  // use rename (e.g. we don't want to replace /dev/null with a regular
+  // file.) If that's the case, we create an in-memory buffer, open the
+  // destination file and write to it on commit().
+  switch (Stat.type()) {
+  case fs::file_type::directory_file:
+    return errc::is_a_directory;
+  case fs::file_type::regular_file:
+  case fs::file_type::file_not_found:
+  case fs::file_type::status_error:
+    return OnDiskBuffer::create(Path, Size, Mode);
+  default:
+    return InMemoryBuffer::create(Path, Size, Mode);
+  }
 }
-} // namespace