Support: Add mapped_file_region::sync(), equivalent to msync

[dexonsmith]

Summary

Add mapped_file_region::sync(), equivalent to POSIX msync,
synchronizing written content to disk without unmapping the region.
Asserts if the mode is not mapped_file_region::readwrite.

Note that I don't have access to a Windows machine, so I can't
easily run those unit tests.

[rnk]

@amccarth, can you confirm that these are the right APIs to use here? Do we need both FlushViewOfFile and FlushFileBuffers?

[dexonsmith]

ping / rebase

[amccarth]

In D95494#2534572, @rnk wrote:

@amccarth, can you confirm that these are the right APIs to use here? Do we need both FlushViewOfFile and FlushFileBuffers?

Yes, those are the right APIs. The first initiates the flush asynchronously. The second blocks until the flush is complete. That's necessary if it's important to match the behavior of the corresponding Posix implementation, which passes the MS_SYNC flag.

The only difference I see is that that Posix version will mark the last write time on the file to be updated, but the Windows version _may_ not. I doubt that's important, but, if it is, then the Windows version would also have to call SetFileTime. I don't know how often LLVM code will sync, so I would avoid an extra operation if it's not necessary.

On Windows, you rarely need to use FlushViewOfFile because (1) the system will keep multiple _views_ coherent across threads and processes and (2) everything will be flushed to disk when the view is closed. FlushViewOfFile is useful when you need to ensure the bits on disk are updated because you expect another thread or process to access the file through something other than another view into the same file mapping.

llvm/unittests/Support/Path.cpp

1377

I'm mildly concerned that there are assertions that will prevent the temporary file from being deleted if/when the test fails. Accumulation of orphaned temp files can slow down tests, eat up disk quotas, etc.

There is a fs::TempFile class that will delete the file when it goes out of scope. Or, perhaps most of these ASSERT_s could be EXPECT_s.

[dexonsmith]

In D95494#2561067, @amccarth wrote:

In D95494#2534572, @rnk wrote:

@amccarth, can you confirm that these are the right APIs to use here? Do we need both FlushViewOfFile and FlushFileBuffers?

Yes, those are the right APIs. The first initiates the flush asynchronously. The second blocks until the flush is complete. That's necessary if it's important to match the behavior of the corresponding Posix implementation, which passes the MS_SYNC flag.

The only difference I see is that that Posix version will mark the last write time on the file to be updated, but the Windows version _may_ not. I doubt that's important, but, if it is, then the Windows version would also have to call SetFileTime. I don't know how often LLVM code will sync, so I would avoid an extra operation if it's not necessary.

On Windows, you rarely need to use FlushViewOfFile because (1) the system will keep multiple _views_ coherent across threads and processes and (2) everything will be flushed to disk when the view is closed. FlushViewOfFile is useful when you need to ensure the bits on disk are updated because you expect another thread or process to access the file through something other than another view into the same file mapping.

Ah, interesting. The use case I have in mind is optionally keeping a just-written-to mapped_file_region alive as a file-backed memory buffer. Effectively, allowing a caller to use f2 instead of f1:

std::unique_ptr<MemoryBuffer> f1(int FD, SmallVectorImpl<char> &&Data) {
  raw_fd_ostream file(FD, ...);
  file << Data;
  return std::make_unique<SmallVectorMemoryBuffer>(std::move(Data));
}
std::unique_ptr<MemoryBuffer> f2(int FD, SmallVectorImpl<char> &&Data) {
  auto MFR = std::make_unique<mapped_file_region>(FD, ...);
  memcpy(MFR.data(), ...)
  SmallVector<char, 0>().swap(Data);
  return std::make_unique<MappedFileRegionMemoryBuffer>(std::move(MFR));
}

I think we'd just want it to sync as much say destruction a file stream or destruction a mapped_file_region.

What do you recommend? (I'll also document clearly what the guarantees are we end up with...)

llvm/unittests/Support/Path.cpp

1377

Ah, thanks; I was just copy/pasting from an adjacent test, but I can at least clean mine up.

[amccarth]

I think we'd just want it to sync as much say destruction a file stream or destruction a mapped_file_region.

Then I think this change would be fine.

But I'm not sure the test is testing that.

On Windows, there are three kinds of objects at play: the file, the mapping, and the view. I don't know the Posix model well enough to know how this corresponds. I also don't know all the relevant details behind fs::mapped_file_region and MemoryBuffer.

On Windows, two views derived from the same file mapping will be coherent--even across processes--without user syncing. Furthermore, all views ultimately backed by the same (non-remote) file will also be coherent. In the test, the second "user" of the file is a MemoryBuffer. Since MemoryBuffer is also opening the file for memory mapping (I believe), it seems its views are going to be coherent regardless of whether you do a manual sync. Thus the test might always pass, regardless of whether your function is correct.

What's not necessarily coherent is regular file i/o with a file that's also using memory mapped i/o. That's the case where you would definitely need a manual sync. This is rare in my experience since Microsoft recommends opening a file in exclusive mode if you're going to use memory mapped i/o.

Although the docs say you can close views and mappings in any order, I've only seen it done in strictly reverse order. And I'm not sure you can close the file itself while mappings still exist. I assume that would just decrement a reference count and the file would remain open until the last mapping object goes away. Compounding this uncertainty is that the test calls close on the file descriptor rather than the file object itself.

So my recommendations are (1) that the test be written so that one of the accesses is via memory mapped i/o and the other is through file i/o, and (2) that the test be shown to fail without the msync call.

[raghavmedicherla]

I have accidentally committed this change without review. I have reverted it. My apologies!
revert link:
https://github.com/llvm/llvm-project/commit/5d3cf8267f90c3715d77f9bf7f97077b65791cb3

llvm/include/llvm/Support/FileSystem.h

public:
   mapped_file_region(sys::fs::file_t fd, mapmode mode, size_t length, uint64_t offset,
                      std::error_code &ec);
 
   ~mapped_file_region();
 
   size_t size() const;
   char *data() const;
 
+  /// Write changes to disk and synchronize. Equivalent to POSIX msync.
+  std::error_code sync() const;
+
   /// Get a const view of the data. Modifying this memory has undefined
   /// behavior.
   const char *const_data() const;
 
   /// \returns The minimum alignment offset must be.
   static int alignment();
 };
 

llvm/lib/Support/Unix/Path.inc

 #endif
 
 using namespace llvm;
 
 namespace llvm {
 namespace sys  {
 namespace fs {
 
 const file_t kInvalidFile = -1;
 
 #if defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__) ||     \
     defined(__minix) || defined(__FreeBSD_kernel__) || defined(__linux__) ||   \
     defined(__CYGWIN__) || defined(__DragonFly__) || defined(_AIX) || defined(__GNU__)
 static int
 test_dir(char ret[PATH_MAX], const char *dir, const char *bin)
 {
   struct stat sb;
if (sys::fs::exists(curproc)) {
     }
   }
   // If we don't have procfs mounted, fall back to argv[0]
   if (getprogpath(exe_path, argv0) != NULL)
     return exe_path;
 #elif defined(__linux__) || defined(__CYGWIN__) || defined(__gnu_hurd__)
   char exe_path[PATH_MAX];
   const char *aPath = "/proc/self/exe";
   if (sys::fs::exists(aPath)) {
     // /proc is not always mounted under Linux (chroot for example).
     ssize_t len = readlink(aPath, exe_path, sizeof(exe_path));
     if (len < 0)
       return "";
 
     // Null terminate the string for realpath. readlink never null
     // terminates its output.
     len = std::min(len, ssize_t(sizeof(exe_path) - 1));
#elif defined(HAVE_DLFCN_H) && defined(HAVE_DLADDR)
   if (realpath(DLInfo.dli_fname, link_path))
     return link_path;
 #else
 #error GetMainExecutable is not implemented on this host yet.
 #endif
   return "";
 }
 
 TimePoint<> basic_file_status::getLastAccessedTime() const {
   return toTimePoint(fs_st_atime, fs_st_atime_nsec);
 }
 
 TimePoint<> basic_file_status::getLastModificationTime() const {
   return toTimePoint(fs_st_mtime, fs_st_mtime_nsec);
 }
 
 UniqueID file_status::getUniqueID() const {
   return UniqueID(fs_st_dev, fs_st_ino);
 }
 
 uint32_t file_status::getLinkCount() const {
   return fs_st_nlinks;
 }
 
 ErrorOr<space_info> disk_space(const Twine &Path) {
   struct STATVFS Vfs;
   if (::STATVFS(const_cast<char *>(Path.str().c_str()), &Vfs))
     return std::error_code(errno, std::generic_category());
   auto FrSize = STATVFS_F_FRSIZE(Vfs);
   space_info SpaceInfo;
   SpaceInfo.capacity = static_cast<uint64_t>(Vfs.f_blocks) * FrSize;
   SpaceInfo.free = static_cast<uint64_t>(Vfs.f_bfree) * FrSize;
   SpaceInfo.available = static_cast<uint64_t>(Vfs.f_bavail) * FrSize;
   return SpaceInfo;
 }
 
 std::error_code current_path(SmallVectorImpl<char> &result) {
   result.clear();
 
   const char *pwd = ::getenv("PWD");
   llvm::sys::fs::file_status PWDStatus, DotStatus;
   if (pwd && llvm::sys::path::is_absolute(pwd) &&
       !llvm::sys::fs::status(pwd, PWDStatus) &&
       !llvm::sys::fs::status(".", DotStatus) &&
       PWDStatus.getUniqueID() == DotStatus.getUniqueID()) {
     result.append(pwd, pwd + strlen(pwd));
     return std::error_code();
   }
 
   result.reserve(PATH_MAX);
 
   while (true) {
     if (::getcwd(result.data(), result.capacity()) == nullptr) {
       // See if there was a real error.
       if (errno != ENOMEM)
         return std::error_code(errno, std::generic_category());
       // Otherwise there just wasn't enough space.
       result.reserve(result.capacity() * 2);
     } else
       break;
   }
 
   result.set_size(strlen(result.data()));
   return std::error_code();
 }
 
 std::error_code set_current_path(const Twine &path) {
   SmallString<128> path_storage;
   StringRef p = path.toNullTerminatedStringRef(path_storage);
 
   if (::chdir(p.begin()) == -1)
     return std::error_code(errno, std::generic_category());
 
   return std::error_code();
 }
 
 std::error_code create_directory(const Twine &path, bool IgnoreExisting,
                                  perms Perms) {
   SmallString<128> path_storage;
   StringRef p = path.toNullTerminatedStringRef(path_storage);
 
   if (::mkdir(p.begin(), Perms) == -1) {
     if (errno != EEXIST || !IgnoreExisting)
       return std::error_code(errno, std::generic_category());
   }
 
   return std::error_code();
if (Mapping)
     ::munmap(Mapping, Size);
 }
 
 size_t mapped_file_region::size() const {
   assert(Mapping && "Mapping failed but used anyway!");
   return Size;
 }
 
+std::error_code mapped_file_region::sync() const {
+  assert(Mapping && "Mapping failed but used anyway!");
+  assert(Mode == readwrite && "sync only valid for readwrite regions!");
+  if (::msync(Mapping, Size, MS_SYNC))
+    return std::error_code(errno, std::generic_category());
+  return std::error_code();
+}
+
 char *mapped_file_region::data() const {
   assert(Mapping && "Mapping failed but used anyway!");
   return reinterpret_cast<char*>(Mapping);
 }
 
 const char *mapped_file_region::const_data() const {
   assert(Mapping && "Mapping failed but used anyway!");
   return reinterpret_cast<const char*>(Mapping);

llvm/lib/Support/Windows/Path.inc

mapped_file_region::~mapped_file_region() {
   }
 }
 
 size_t mapped_file_region::size() const {
   assert(Mapping && "Mapping failed but used anyway!");
   return Size;
 }
 
+std::error_code mapped_file_region::sync() const {
+  assert(Mapping && "Mapping failed but used anyway!");
+  assert(Mode == readwrite && "sync only valid for readwrite regions!");
+  if (!::FlushViewOfFile(Mapping, Size))
+    return mapWindowsError(GetLastError());
+  if (!::FlushFileBuffers(FileHandle))
+    return mapWindowsError(GetLastError());
+  return std::error_code();
+}
+
 char *mapped_file_region::data() const {
   assert(Mapping && "Mapping failed but used anyway!");
   return reinterpret_cast<char*>(Mapping);
 }
 
 const char *mapped_file_region::const_data() const {
   assert(Mapping && "Mapping failed but used anyway!");
   return reinterpret_cast<const char*>(Mapping);

llvm/unittests/Support/Path.cpp

StringRef Val("hello there");
     std::copy(Val.begin(), Val.end(), mfr.data());
     // Explicitly add a 0.
     mfr.data()[Val.size()] = 0;
     // Unmap temp file
   }
   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),
                                fs::mapped_file_region::readonly, Size, 0, EC);
     ASSERT_NO_ERROR(EC);
 
     // Verify content
     EXPECT_EQ(StringRef(mfr.const_data()), Val);
 
     // Unmap temp file
     fs::mapped_file_region m(fs::convertFDToNativeFile(FD),
                              fs::mapped_file_region::readonly, Size, 0, EC);
     ASSERT_NO_ERROR(EC);
     ASSERT_EQ(close(FD), 0);
   }
   ASSERT_NO_ERROR(fs::remove(TempPath));
 }
 
+TEST_F(FileSystemTest, FileMappingSync) {
+  // Create a temp file.
+  int FD;
+  SmallString<64> TempPath;
+  ASSERT_NO_ERROR(fs::createTemporaryFile("prefix", "temp", FD, TempPath));
+  StringRef Content("hello there");
+  ASSERT_NO_ERROR(fs::resize_file_before_mapping_readwrite(FD, Content.size()));
+
+  {
+    // Map in the file and write some content.
+    std::error_code EC;
+    fs::mapped_file_region mfr(fs::convertFDToNativeFile(FD),
+                               fs::mapped_file_region::readwrite,
+                               Content.size(), 0, EC);
+    ASSERT_NO_ERROR(EC);
+    ASSERT_EQ(close(FD), 0);
+    std::copy(Content.begin(), Content.end(), mfr.data());
+
+    // Synchronize and check the contents before unmapping.
+    mfr.sync();
+    auto Buffer = MemoryBuffer::getFile(TempPath);
+    ASSERT_TRUE((bool)Buffer);
+    ASSERT_EQ(Content, Buffer->get()->getBuffer());
+  }
+  ASSERT_NO_ERROR(fs::remove(TempPath));
+}
+
 TEST(Support, NormalizePath) {
   //                           Input,        Expected Win, Expected Posix
   using TestTuple = std::tuple<const char *, const char *, const char *>;
   std::vector<TestTuple> Tests;
   Tests.emplace_back("a", "a", "a");
   Tests.emplace_back("a/b", "a\\b", "a/b");
   Tests.emplace_back("a\\b", "a\\b", "a/b");
   Tests.emplace_back("a\\\\b", "a\\\\b", "a//b");