diff --git a/llvm/include/llvm/Support/FileSystem.h b/llvm/include/llvm/Support/FileSystem.h --- a/llvm/include/llvm/Support/FileSystem.h +++ b/llvm/include/llvm/Support/FileSystem.h @@ -1253,25 +1253,57 @@ 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; diff --git a/llvm/lib/Support/FileOutputBuffer.cpp b/llvm/lib/Support/FileOutputBuffer.cpp --- a/llvm/lib/Support/FileOutputBuffer.cpp +++ b/llvm/lib/Support/FileOutputBuffer.cpp @@ -33,21 +33,20 @@ // with the temporary file on commit(). class OnDiskBuffer : public FileOutputBuffer { public: - OnDiskBuffer(StringRef Path, fs::TempFile Temp, - std::unique_ptr 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); @@ -56,7 +55,7 @@ ~OnDiskBuffer() override { // Close the mapping before deleting the temp file, so that the removal // succeeds. - Buffer.reset(); + Buffer.unmap(); consumeError(Temp.discard()); } @@ -67,7 +66,7 @@ } private: - std::unique_ptr Buffer; + fs::mapped_file_region Buffer; fs::TempFile Temp; }; @@ -139,9 +138,9 @@ // Mmap it. std::error_code EC; - auto MappedFile = std::make_unique( - 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. diff --git a/llvm/lib/Support/Unix/Path.inc b/llvm/lib/Support/Unix/Path.inc --- a/llvm/lib/Support/Unix/Path.inc +++ b/llvm/lib/Support/Unix/Path.inc @@ -846,14 +846,14 @@ 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); } diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc --- a/llvm/lib/Support/Windows/Path.inc +++ b/llvm/lib/Support/Windows/Path.inc @@ -903,10 +903,10 @@ 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() { @@ -925,7 +925,7 @@ return false; } -mapped_file_region::~mapped_file_region() { +void mapped_file_region::unmapImpl() { if (Mapping) { bool Exe = isEXE(StringRef((char *)Mapping, Size)); diff --git a/llvm/unittests/Support/Path.cpp b/llvm/unittests/Support/Path.cpp --- a/llvm/unittests/Support/Path.cpp +++ b/llvm/unittests/Support/Path.cpp @@ -1341,6 +1341,8 @@ // 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); @@ -1348,8 +1350,23 @@ 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