[llvm] 0db6488 - Support: Add move semantics to mapped_file_region
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 9 18:00:20 PDT 2021
Author: Duncan P. N. Exon Smith
Date: 2021-04-09T17:56:26-07:00
New Revision: 0db6488a7704a322ae05f20ef3466bed3eb1bb98
URL: https://github.com/llvm/llvm-project/commit/0db6488a7704a322ae05f20ef3466bed3eb1bb98
DIFF: https://github.com/llvm/llvm-project/commit/0db6488a7704a322ae05f20ef3466bed3eb1bb98.diff
LOG: Support: Add move semantics to mapped_file_region
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.
Differential Revision: https://reviews.llvm.org/D100159
Added:
Modified:
llvm/include/llvm/Support/FileSystem.h
llvm/lib/Support/FileOutputBuffer.cpp
llvm/lib/Support/Unix/Path.inc
llvm/lib/Support/Windows/Path.inc
llvm/unittests/Support/Path.cpp
Removed:
################################################################################
diff --git a/llvm/include/llvm/Support/FileSystem.h b/llvm/include/llvm/Support/FileSystem.h
index da3627721ebd1..526a97f752249 100644
--- a/llvm/include/llvm/Support/FileSystem.h
+++ b/llvm/include/llvm/Support/FileSystem.h
@@ -1253,25 +1253,57 @@ class mapped_file_region {
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
index 6d2125802f257..4b4406c4c9f4b 100644
--- a/llvm/lib/Support/FileOutputBuffer.cpp
+++ b/llvm/lib/Support/FileOutputBuffer.cpp
@@ -33,21 +33,20 @@ namespace {
// 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);
@@ -56,7 +55,7 @@ class OnDiskBuffer : public FileOutputBuffer {
~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 @@ class OnDiskBuffer : public FileOutputBuffer {
}
private:
- std::unique_ptr<fs::mapped_file_region> Buffer;
+ fs::mapped_file_region Buffer;
fs::TempFile Temp;
};
@@ -139,9 +138,9 @@ createOnDiskBuffer(StringRef Path, size_t Size, unsigned Mode) {
// 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.
diff --git a/llvm/lib/Support/Unix/Path.inc b/llvm/lib/Support/Unix/Path.inc
index 28a0bfa873763..56ed17e364e89 100644
--- a/llvm/lib/Support/Unix/Path.inc
+++ b/llvm/lib/Support/Unix/Path.inc
@@ -846,14 +846,14 @@ std::error_code mapped_file_region::init(int FD, uint64_t Offset,
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
index 72c0bf2cf8aa6..19b9f9ef7cbce 100644
--- a/llvm/lib/Support/Windows/Path.inc
+++ b/llvm/lib/Support/Windows/Path.inc
@@ -903,10 +903,10 @@ std::error_code mapped_file_region::init(sys::fs::file_t OrigFileHandle,
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 @@ static bool isEXE(StringRef Magic) {
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
index 6615d2f2806f3..33f2c712f5ca6 100644
--- a/llvm/unittests/Support/Path.cpp
+++ b/llvm/unittests/Support/Path.cpp
@@ -1341,6 +1341,8 @@ TEST_F(FileSystemTest, FileMapping) {
// 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 @@ TEST_F(FileSystemTest, FileMapping) {
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
More information about the llvm-commits
mailing list