Support: Add move semantics to mapped_file_region

[dexonsmith]

Summary

Update llvm::sys::fs::mapped_file_region to have a move constructor and
a move assignment operator, allowing it to be used as an Optional. Also,
update FileOutputBuffer's OnDiskBuffer to take advantage of this,
avoiding an extra allocation from the unique_ptr.

A nice follow-up would be to make the mapped_file_region constructor
private and replace its use with a factory function, such as
mapped_file_region::create(), that returns an Expected (or ErrorOr). I
don't plan on doing that immediately, but I might swing back later.

No functionality change, besides the saved allocation in OnDiskBuffer.

[dblaikie]

looks like the underlying mapped_file_region could be made movable without the overhead of Optional (both the Windows and Unix dtors of mapped_file_region test whether the mapping is non-null, so a move ctor can null out the Mapping member to ensure no double-closing, etc) which might be nicer (avoiding the Optional overhead)/more general?

[dexonsmith]

Follow @dblaikie's suggestion to allow mapped_file_region to be used directly as an optional.

• Add a default constructor.
• Add an operator bool().
• Add unmap().

[dblaikie]

Looks great!

llvm/include/llvm/Support/FileSystem.h

1289–1291

This is the default/doesn't need to be written, since the type has user-declared move ops, so they could be omitted - but don't mind if you prefer to have them here for documentation.

[dexonsmith]

Thanks for the review! Landed in 0db6488a7704a322ae05f20ef3466bed3eb1bb98.

llvm/include/llvm/Support/FileSystem.h

1289–1291

I left this in, as I find it's a bit clearer for reading, but if the style is moving away from that I don't feel strongly.

[dblaikie]

llvm/include/llvm/Support/FileSystem.h

1289–1291

Nah, don't think there's any major prevailing take on that - do whatever you reckon is most readable!

llvm/include/llvm/Support/FileSystem.h

public:
   enum mapmode {
     readonly, ///< May only access map via const_data as read only.
     readwrite, ///< May access map via data and modify it. Written to path.
     priv ///< May modify via data, but changes are lost on destruction.
   };
 
 private:
   /// Platform-specific mapping state.
-  size_t Size;
-  void *Mapping;
+  size_t Size = 0;
+  void *Mapping = nullptr;
 #ifdef _WIN32
-  sys::fs::file_t FileHandle;
+  sys::fs::file_t FileHandle = nullptr;
 #endif
-  mapmode Mode;
+  mapmode Mode = readonly;
+
+  void copyFrom(const mapped_file_region &Copied) {
+    Size = Copied.Size;
+    Mapping = Copied.Mapping;
+#ifdef _WIN32
+    FileHandle = Copied.FileHandle;
+#endif
+    Mode = Copied.Mode;
+  }
+
+  void moveFromImpl(mapped_file_region &Moved) {
+    copyFrom(Moved);
+    Moved.copyFrom(mapped_file_region());
+  }
+
+  void unmapImpl();
 
   std::error_code init(sys::fs::file_t FD, uint64_t Offset, mapmode Mode);
 
 public:
-  mapped_file_region() = delete;
-  mapped_file_region(mapped_file_region&) = delete;
-  mapped_file_region &operator =(mapped_file_region&) = delete;
+  mapped_file_region() = default;
+  mapped_file_region(mapped_file_region &&Moved) { moveFromImpl(Moved); }
+  mapped_file_region &operator=(mapped_file_region &&Moved) {
+    unmap();
+    moveFromImpl(Moved);
+    return *this;
+  }
+
+  mapped_file_region(const mapped_file_region &) = delete;
+  mapped_file_region &operator=(const mapped_file_region &) = delete;
 
   /// \param fd An open file descriptor to map. Does not take ownership of fd.
   mapped_file_region(sys::fs::file_t fd, mapmode mode, size_t length, uint64_t offset,
                      std::error_code &ec);
 
-  ~mapped_file_region();
+  ~mapped_file_region() { unmapImpl(); }
+
+  /// Check if this is a valid mapping.
+  explicit operator bool() const { return Mapping; }
+
+  /// Unmap.
+  void unmap() {
+    unmapImpl();
+    copyFrom(mapped_file_region());
+  }
 
   size_t size() const;
   char *data() const;
 
   /// Get a const view of the data. Modifying this memory has undefined
   /// behavior.
   const char *const_data() const;
 

llvm/lib/Support/FileOutputBuffer.cpp

 using namespace llvm::sys;
 
 namespace {
 // 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, fs::TempFile Temp,
-               std::unique_ptr<fs::mapped_file_region> Buf)
+  OnDiskBuffer(StringRef Path, fs::TempFile Temp, fs::mapped_file_region Buf)
       : FileOutputBuffer(Path), Buffer(std::move(Buf)), Temp(std::move(Temp)) {}
 
-  uint8_t *getBufferStart() const override { return (uint8_t *)Buffer->data(); }
+  uint8_t *getBufferStart() const override { return (uint8_t *)Buffer.data(); }
 
   uint8_t *getBufferEnd() const override {
-    return (uint8_t *)Buffer->data() + Buffer->size();
+    return (uint8_t *)Buffer.data() + Buffer.size();
   }
 
-  size_t getBufferSize() const override { return 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();
+    Buffer.unmap();
 
     // Atomically replace the existing file with the new one.
     return Temp.keep(FinalPath);
   }
 
   ~OnDiskBuffer() override {
     // Close the mapping before deleting the temp file, so that the removal
     // succeeds.
-    Buffer.reset();
+    Buffer.unmap();
     consumeError(Temp.discard());
   }
 
   void discard() override {
     // Delete the temp file if it still was open, but keeping the mapping
     // active.
     consumeError(Temp.discard());
   }
 
 private:
-  std::unique_ptr<fs::mapped_file_region> Buffer;
+  fs::mapped_file_region Buffer;
   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, std::size_t BufSize,
createOnDiskBuffer(StringRef Path, size_t Size, unsigned Mode) {
 
   if (auto EC = fs::resize_file_before_mapping_readwrite(File.FD, Size)) {
     consumeError(File.discard());
     return errorCodeToError(EC);
   }
 
   // Mmap it.
   std::error_code EC;
-  auto MappedFile = std::make_unique<fs::mapped_file_region>(
-      fs::convertFDToNativeFile(File.FD), fs::mapped_file_region::readwrite,
-      Size, 0, EC);
+  fs::mapped_file_region MappedFile =
+      fs::mapped_file_region(fs::convertFDToNativeFile(File.FD),
+                             fs::mapped_file_region::readwrite, Size, 0, EC);
 
   // mmap(2) can fail if the underlying filesystem does not support it.
   // If that happens, we fall back to in-memory buffer as the last resort.
   if (EC) {
     consumeError(File.discard());
     return createInMemoryBuffer(Path, Size, Mode);
   }
 

llvm/lib/Support/Unix/Path.inc

#endif // #if defined (__APPLE__)
   Mapping = ::mmap(nullptr, Size, prot, flags, FD, Offset);
   if (Mapping == MAP_FAILED)
     return std::error_code(errno, std::generic_category());
   return std::error_code();
 }
 
 mapped_file_region::mapped_file_region(int fd, mapmode mode, size_t length,
                                        uint64_t offset, std::error_code &ec)
-    : Size(length), Mapping(), Mode(mode) {
+    : Size(length), Mode(mode) {
   (void)Mode;
   ec = init(fd, offset, mode);
   if (ec)
-    Mapping = nullptr;
+    copyFrom(mapped_file_region());
 }
 
-mapped_file_region::~mapped_file_region() {
+void mapped_file_region::unmapImpl() {
   if (Mapping)
     ::munmap(Mapping, Size);
 }
 
 int mapped_file_region::alignment() {
   return Process::getPageSizeEstimate();
 }
 

llvm/lib/Support/Windows/Path.inc

std::error_code mapped_file_region::init(sys::fs::file_t OrigFileHandle,
   }
 
   return std::error_code();
 }
 
 mapped_file_region::mapped_file_region(sys::fs::file_t fd, mapmode mode,
                                        size_t length, uint64_t offset,
                                        std::error_code &ec)
-    : Size(length), Mapping() {
+    : Size(length) {
   ec = init(fd, offset, mode);
   if (ec)
-    Mapping = 0;
+    copyFrom(mapped_file_region());
 }
 
 static bool hasFlushBufferKernelBug() {
   static bool Ret{GetWindowsOSVersion() < llvm::VersionTuple(10, 0, 0, 17763)};
   return Ret;
 }
 
 static bool isEXE(StringRef Magic) {
   static const char PEMagic[] = {'P', 'E', '\0', '\0'};
   if (Magic.startswith(StringRef("MZ")) && Magic.size() >= 0x3c + 4) {
     uint32_t off = read32le(Magic.data() + 0x3c);
     // PE/COFF file, either EXE or DLL.
     if (Magic.substr(off).startswith(StringRef(PEMagic, sizeof(PEMagic))))
       return true;
   }
   return false;
 }
 
-mapped_file_region::~mapped_file_region() {
+void mapped_file_region::unmapImpl() {
   if (Mapping) {
 
     bool Exe = isEXE(StringRef((char *)Mapping, Size));
 
     ::UnmapViewOfFile(Mapping);
 
     if (Mode == mapmode::readwrite && Exe && hasFlushBufferKernelBug()) {
       // There is a Windows kernel bug, the exact trigger conditions of which

llvm/unittests/Support/Path.cpp

ASSERT_NO_ERROR(
       fs::createTemporaryFile("prefix", "temp", FileDescriptor, TempPath));
   unsigned Size = 4096;
   ASSERT_NO_ERROR(
       fs::resize_file_before_mapping_readwrite(FileDescriptor, Size));
 
   // Map in temp file and add some content
   std::error_code EC;
   StringRef Val("hello there");
+  fs::mapped_file_region MaybeMFR;
+  EXPECT_FALSE(MaybeMFR);
   {
     fs::mapped_file_region mfr(fs::convertFDToNativeFile(FileDescriptor),
                                fs::mapped_file_region::readwrite, Size, 0, EC);
     ASSERT_NO_ERROR(EC);
     std::copy(Val.begin(), Val.end(), mfr.data());
     // Explicitly add a 0.
     mfr.data()[Val.size()] = 0;
-    // Unmap temp file
+
+    // Move it out of the scope and confirm mfr is reset.
+    MaybeMFR = std::move(mfr);
+    EXPECT_FALSE(mfr);
+#if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
+    EXPECT_DEATH(mfr.data(), "Mapping failed but used anyway!");
+    EXPECT_DEATH(mfr.size(), "Mapping failed but used anyway!");
+#endif
   }
+
+  // Check that the moved-to region is still valid.
+  EXPECT_EQ(Val, StringRef(MaybeMFR.data()));
+  EXPECT_EQ(Size, MaybeMFR.size());
+
+  // Unmap temp file.
+  MaybeMFR.unmap();
+
   ASSERT_EQ(close(FileDescriptor), 0);
 
   // Map it back in read-only
   {
     int FD;
     EC = fs::openFileForRead(Twine(TempPath), FD);
     ASSERT_NO_ERROR(EC);
     fs::mapped_file_region mfr(fs::convertFDToNativeFile(FD),