Add a ReadWriteMemoryBuffer class that can map an input file for modification

[zturner]

Summary

We currently have two MemoryBuffer abstractions. The first is the most commonly used, and maps a file for read only access. The second is called WritableMemoryBuffer and maps a file privately (e.g. using copy-on-write) in case you need to make local modifications. But until this patch, we have no abstraction that can map a file in a way that allows to commit modifications back to the underlying file. There are no users of this yet (in this patch anyway), but I have a patch locally which will use this in a followup.

[zturner]

clang-format the patch.

[rnk]

We should have a unit test where we fill a file with some text and then modify it in place, like toUpper every character or something. Otherwise, looks good.

[labath]

It seems fine, but a test would certainly be useful. Two things had come to mind, but I don't have a definitive answer to either of them:

• the difference between a WritableMemoryBuffer and ReadWriteMemoryBuffer is not very obvious. After all, both of them support reading and writing... Maybe name the second one something like WriteThroughMemoryBuffer, to emphasize that the changes are not local?
• Right now the F_NoTrunc flag is ignored if one specifies F_Append (we always treat it as present). However, the Append&&!NoTrunc combination also has a well defined (although not extremely useful) semantics. I wonder if it wouldn't be clearer to make the two completely independent. This would require adding F_NoTrunc to every existing call that specifies F_Append, but I currently count only about half a dozen of those..

llvm/include/llvm/Support/MemoryBuffer.h

216 ↗

(On Diff #137494)

The word "commit" bring database associations, like the changes being atomic and not permanent until you explicitly apply them, but that's not true. I would just say the changes are written back to the original file.

llvm/lib/Support/MemoryBuffer.cpp

372 ↗

(On Diff #137494)

I think this is not used.

[zturner]

In D44230#1031111, @labath wrote:

• Right now the F_NoTrunc flag is ignored if one specifies F_Append (we always treat it as present). However, the Append&&!NoTrunc combination also has a well defined (although not extremely useful) semantics. I wonder if it wouldn't be clearer to make the two completely independent. This would require adding F_NoTrunc to every existing call that specifies F_Append, but I currently count only about half a dozen of those..

It might be clearer, to someone reading the code, but probably less obvious to someone writing new code. People familiar with a posix-like api expect the current F_Append semantics, so I think we should make those assumptions continue to hold.

I'll work on a test.

[zturner]

Added a test.

llvm/trunk/include/llvm/Support/FileSystem.h

enum OpenFlags : unsigned {
   /// report an error if the file already exists.
   F_Excl = 1,
 
   /// F_Append - When opening a file, if it already exists append to the
   /// existing file instead of returning an error.  This may not be specified
   /// with F_Excl.
   F_Append = 2,
 
+  /// F_NoTrunc - When opening a file, if it already exists don't truncate
+  /// the file contents.  F_Append implies F_NoTrunc, but F_Append seeks to
+  /// the end of the file, which F_NoTrunc doesn't.
+  F_NoTrunc = 4,
+
   /// The file should be opened in text mode on platforms that make this
   /// distinction.
-  F_Text = 4,
+  F_Text = 8,
 
   /// Open the file for read and write.
-  F_RW = 8,
+  F_RW = 16,
 
   /// Delete the file on close. Only makes a difference on windows.
-  F_Delete = 16
+  F_Delete = 32
 };
 
 /// @brief Create a uniquely named file.
 ///
 /// Generates a unique path suitable for a temporary file and then opens it as a
 /// file. The name is based on \a model with '%' replaced by a random char in
 /// [0-9a-f]. If \a model is not an absolute path, the temporary file will be
 /// created in the current directory.

llvm/trunk/include/llvm/Support/MemoryBuffer.h

 #define LLVM_SUPPORT_MEMORYBUFFER_H
 
 #include "llvm-c/Types.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Support/CBindingWrapping.h"
 #include "llvm/Support/ErrorOr.h"
+#include "llvm/Support/FileSystem.h"
 #include <cstddef>
 #include <cstdint>
 #include <memory>
 
 namespace llvm {
 
 class MemoryBufferRef;
 

 
 
 protected:
   MemoryBuffer() = default;
 
   void init(const char *BufStart, const char *BufEnd,
             bool RequiresNullTerminator);
 
-  static constexpr bool Writable = false;
+  static constexpr sys::fs::mapped_file_region::mapmode Mapmode =
+      sys::fs::mapped_file_region::readonly;
 
 public:
   MemoryBuffer(const MemoryBuffer &) = delete;
   MemoryBuffer &operator=(const MemoryBuffer &) = delete;
   virtual ~MemoryBuffer();
 
   const char *getBufferStart() const { return BufferStart; }
   const char *getBufferEnd() const   { return BufferEnd; }
public:
 
   /// Return information on the memory mechanism used to support the
   /// MemoryBuffer.
   virtual BufferKind getBufferKind() const = 0;
 
   MemoryBufferRef getMemBufferRef() const;
 };
 
-/// This class is an extension of MemoryBuffer, which allows writing to the
-/// underlying contents.  It only supports creation methods that are guaranteed
-/// to produce a writable buffer.  For example, mapping a file read-only is not
-/// supported.
+/// This class is an extension of MemoryBuffer, which allows copy-on-write
+/// access to the underlying contents.  It only supports creation methods that
+/// are guaranteed to produce a writable buffer.  For example, mapping a file
+/// read-only is not supported.
 class WritableMemoryBuffer : public MemoryBuffer {
 protected:
   WritableMemoryBuffer() = default;
 
-  static constexpr bool Writable = true;
+  static constexpr sys::fs::mapped_file_region::mapmode Mapmode =
+      sys::fs::mapped_file_region::priv;
 
 public:
   using MemoryBuffer::getBuffer;
   using MemoryBuffer::getBufferEnd;
   using MemoryBuffer::getBufferStart;
 
   // const_cast is well-defined here, because the underlying buffer is
   // guaranteed to have been initialized with a mutable buffer.
private:
   using MemoryBuffer::getFileOrSTDIN;
   using MemoryBuffer::getMemBuffer;
   using MemoryBuffer::getMemBufferCopy;
   using MemoryBuffer::getOpenFile;
   using MemoryBuffer::getOpenFileSlice;
   using MemoryBuffer::getSTDIN;
 };
 
+/// This class is an extension of MemoryBuffer, which allows write access to
+/// the underlying contents and committing those changes to the original source.
+/// It only supports creation methods that are guaranteed to produce a writable
+/// buffer.  For example, mapping a file read-only is not supported.
+class WriteThroughMemoryBuffer : public MemoryBuffer {
+protected:
+  WriteThroughMemoryBuffer() = default;
+
+  static constexpr sys::fs::mapped_file_region::mapmode Mapmode =
+      sys::fs::mapped_file_region::readwrite;
+
+public:
+  using MemoryBuffer::getBuffer;
+  using MemoryBuffer::getBufferEnd;
+  using MemoryBuffer::getBufferStart;
+
+  // const_cast is well-defined here, because the underlying buffer is
+  // guaranteed to have been initialized with a mutable buffer.
+  char *getBufferStart() {
+    return const_cast<char *>(MemoryBuffer::getBufferStart());
+  }
+  char *getBufferEnd() {
+    return const_cast<char *>(MemoryBuffer::getBufferEnd());
+  }
+  MutableArrayRef<char> getBuffer() {
+    return {getBufferStart(), getBufferEnd()};
+  }
+
+  static ErrorOr<std::unique_ptr<WriteThroughMemoryBuffer>>
+  getFile(const Twine &Filename, int64_t FileSize = -1);
+
+  /// Map a subrange of the specified file as a ReadWriteMemoryBuffer.
+  static ErrorOr<std::unique_ptr<WriteThroughMemoryBuffer>>
+  getFileSlice(const Twine &Filename, uint64_t MapSize, uint64_t Offset);
+
+private:
+  // Hide these base class factory function so one can't write
+  //   WritableMemoryBuffer::getXXX()
+  // and be surprised that he got a read-only Buffer.
+  using MemoryBuffer::getFileAsStream;
+  using MemoryBuffer::getFileOrSTDIN;
+  using MemoryBuffer::getMemBuffer;
+  using MemoryBuffer::getMemBufferCopy;
+  using MemoryBuffer::getOpenFile;
+  using MemoryBuffer::getOpenFileSlice;
+  using MemoryBuffer::getSTDIN;
+};
+
 class MemoryBufferRef {
   StringRef Buffer;
   StringRef Identifier;
 
 public:
   MemoryBufferRef() = default;
   MemoryBufferRef(MemoryBuffer& Buffer)
       : Buffer(Buffer.getBuffer()), Identifier(Buffer.getBufferIdentifier()) {}

llvm/trunk/lib/Support/MemoryBuffer.cpp

class MemoryBufferMMapFile : public MB {
 
   const char *getStart(uint64_t Len, uint64_t Offset) {
     return MFR.const_data() + (Offset - getLegalMapOffset(Offset));
   }
 
 public:
   MemoryBufferMMapFile(bool RequiresNullTerminator, int FD, uint64_t Len,
                        uint64_t Offset, std::error_code &EC)
-      : MFR(FD,
-            MB::Writable ? sys::fs::mapped_file_region::priv
-                         : sys::fs::mapped_file_region::readonly,
-            getLegalMapSize(Len, Offset), getLegalMapOffset(Offset), EC) {
+      : MFR(FD, MB::Mapmode, getLegalMapSize(Len, Offset),
+            getLegalMapOffset(Offset), EC) {
     if (!EC) {
       const char *Start = getStart(Len, Offset);
       MemoryBuffer::init(Start, Start + Len, RequiresNullTerminator);
     }
   }
 
   /// Disable sized deallocation for MemoryBufferMMapFile, because it has
   /// tail-allocated data.
#if defined(__CYGWIN__)
   // FIXME: We should reorganize again getPageSize() on Win32.
   if ((FileSize & (4096 - 1)) == 0)
     return false;
 #endif
 
   return true;
 }
 
+static ErrorOr<std::unique_ptr<WriteThroughMemoryBuffer>>
+getReadWriteFile(const Twine &Filename, int64_t FileSize, uint64_t MapSize,
+                 uint64_t Offset) {
+  int FD;
+  std::error_code EC = sys::fs::openFileForWrite(
+      Filename, FD, sys::fs::F_RW | sys::fs::F_NoTrunc);
+
+  if (EC)
+    return EC;
+
+  // Default is to map the full file.
+  if (MapSize == uint64_t(-1)) {
+    // If we don't know the file size, use fstat to find out.  fstat on an open
+    // file descriptor is cheaper than stat on a random path.
+    if (FileSize == uint64_t(-1)) {
+      sys::fs::file_status Status;
+      std::error_code EC = sys::fs::status(FD, Status);
+      if (EC)
+        return EC;
+
+      // If this not a file or a block device (e.g. it's a named pipe
+      // or character device), we can't mmap it, so error out.
+      sys::fs::file_type Type = Status.type();
+      if (Type != sys::fs::file_type::regular_file &&
+          Type != sys::fs::file_type::block_file)
+        return make_error_code(errc::invalid_argument);
+
+      FileSize = Status.getSize();
+    }
+    MapSize = FileSize;
+  }
+
+  std::unique_ptr<WriteThroughMemoryBuffer> Result(
+      new (NamedBufferAlloc(Filename))
+          MemoryBufferMMapFile<WriteThroughMemoryBuffer>(false, FD, MapSize,
+                                                         Offset, EC));
+  if (EC)
+    return EC;
+  return std::move(Result);
+}
+
+ErrorOr<std::unique_ptr<WriteThroughMemoryBuffer>>
+WriteThroughMemoryBuffer::getFile(const Twine &Filename, int64_t FileSize) {
+  return getReadWriteFile(Filename, FileSize, FileSize, 0);
+}
+
+/// Map a subrange of the specified file as a WritableMemoryBuffer.
+ErrorOr<std::unique_ptr<WriteThroughMemoryBuffer>>
+WriteThroughMemoryBuffer::getFileSlice(const Twine &Filename, uint64_t MapSize,
+                                       uint64_t Offset) {
+  return getReadWriteFile(Filename, -1, MapSize, Offset);
+}
+
 template <typename MB>
 static ErrorOr<std::unique_ptr<MB>>
 getOpenFileImpl(int FD, const Twine &Filename, uint64_t FileSize,
                 uint64_t MapSize, int64_t Offset, bool RequiresNullTerminator,
                 bool IsVolatile) {
   static int PageSize = sys::Process::getPageSize();
 
   // Default is to map the full file.

llvm/trunk/lib/Support/Unix/Path.inc

#endif
 
   if (Flags & F_RW)
     OpenFlags |= O_RDWR;
   else
     OpenFlags |= O_WRONLY;
 
   if (Flags & F_Append)
     OpenFlags |= O_APPEND;
-  else
+  else if (!(Flags & F_NoTrunc))
     OpenFlags |= O_TRUNC;
 
   if (Flags & F_Excl)
     OpenFlags |= O_EXCL;
 
   SmallString<128> Storage;
   StringRef P = Name.toNullTerminatedStringRef(Storage);
   if ((ResultFD = sys::RetryAfterSignal(-1, open, P.begin(), OpenFlags, Mode)) < 0)

llvm/trunk/lib/Support/Windows/Path.inc

std::error_code openFileForWrite(const Twine &Name, int &ResultFD,
   SmallVector<wchar_t, 128> PathUTF16;
 
   if (std::error_code EC = widenPath(Name, PathUTF16))
     return EC;
 
   DWORD CreationDisposition;
   if (Flags & F_Excl)
     CreationDisposition = CREATE_NEW;
-  else if (Flags & F_Append)
+  else if ((Flags & F_Append) || (Flags & F_NoTrunc))
     CreationDisposition = OPEN_ALWAYS;
   else
     CreationDisposition = CREATE_ALWAYS;
 
   DWORD Access = GENERIC_WRITE;
   DWORD Attributes = FILE_ATTRIBUTE_NORMAL;
   if (Flags & F_RW)
     Access |= GENERIC_READ;

llvm/trunk/unittests/Support/MemoryBufferTest.cpp

TEST_F(MemoryBufferTest, writableSlice) {
 
   auto MBOrError = MemoryBuffer::getFile(TestPath);
   ASSERT_FALSE(MBOrError.getError());
   auto &MB = **MBOrError;
   ASSERT_EQ(0x10000u, MB.getBufferSize());
   for (size_t i = 0; i < MB.getBufferSize(); i += 0x10)
     EXPECT_EQ("0123456789abcdef", MB.getBuffer().substr(i, 0x10)) << "i: " << i;
 }
+
+TEST_F(MemoryBufferTest, writeThroughFile) {
+  // Create a file initialized with some data
+  int FD;
+  SmallString<64> TestPath;
+  sys::fs::createTemporaryFile("MemoryBufferTest_WriteThrough", "temp", FD,
+                               TestPath);
+  FileRemover Cleanup(TestPath);
+  raw_fd_ostream OF(FD, true);
+  OF << "0123456789abcdef";
+  OF.close();
+  {
+    auto MBOrError = WriteThroughMemoryBuffer::getFile(TestPath);
+    ASSERT_FALSE(MBOrError.getError());
+    // Write some data.  It should be mapped readwrite, so that upon completion
+    // the original file contents are modified.
+    WriteThroughMemoryBuffer &MB = **MBOrError;
+    ASSERT_EQ(16, MB.getBufferSize());
+    char *Start = MB.getBufferStart();
+    ASSERT_EQ(MB.getBufferEnd(), MB.getBufferStart() + MB.getBufferSize());
+    ::memset(Start, 'x', MB.getBufferSize());
+  }
+
+  auto MBOrError = MemoryBuffer::getFile(TestPath);
+  ASSERT_FALSE(MBOrError.getError());
+  auto &MB = **MBOrError;
+  ASSERT_EQ(16, MB.getBufferSize());
+  EXPECT_EQ("xxxxxxxxxxxxxxxx", MB.getBuffer());
+}
 }