Call FlushFileBuffers on readwrite file mappings.

[zturner]

Summary

Due to a Windows kernel bug, calling FlushFileBuffers is necessary to ensure that subsequent processes can correctly read the data we write. More information is available in this connect issue. The Windows Kernel team is also aware and from my understanding they are investigating. Until we have more clear guidance about what versions of Windows this affects, I'm adding this call to FlushFileBuffers for all readwrite mappings on all versions of Windows.

[amccarth]

llvm/lib/Support/Windows/Path.inc

900 ↗

(On Diff #132872)

How about including the URL from the patch description directly in the comment?

[zturner]

llvm/lib/Support/Windows/Path.inc

900 ↗

(On Diff #132872)

I thought about it, but it would wrap past 80 characters (not a huge deal, admittedly). More importantly though, it doesn't contain any kind of workaround or acknowledgement from Microsoft other than "we did the same thing in link.exe". I was hoping that if / when we get more concrete information from someone on the kernel team that I could update it with that, since that would actually be some additional info besides what's in this comment. That said, if anyone feels strongly, I can add it here in the meantime (although it doesn't really say much that this comment doesn't already say).

[amccarth]

llvm/lib/Support/Windows/Path.inc

900 ↗

(On Diff #132872)

SGTM.

[rnk]

One concern I have is that this is a common pattern on Linux:

int fd = open(...);
void *mem = mmap(fd, ...);
close(fd);
// use mem

Are we reasonably confident that LLVM doesn't have this pattern anywhere in it? i.e. we never store the FD and then try to use it after closing it?

[zturner]

In D42925#998354, @rnk wrote:

One concern I have is that this is a common pattern on Linux:

int fd = open(...);
void *mem = mmap(fd, ...);
close(fd);
// use mem

Are we reasonably confident that LLVM doesn't have this pattern anywhere in it? i.e. we never store the FD and then try to use it after closing it?

We don't do that anywhere *at the moment* that I can find, but I suppose it's a concern for future proofing this code. Someone could come along and add code like this after the fact. Currently the only place where we open a mapped_file_region for readwrite is in OnDiskBuffer, which stores a TempFile and then renames it atomically to the final path. We could put the flush logic inside of TempFile::keep() so that it flushes it before renaming. That might be a better option, although it wouldn't catch the case where someone comes along later and tries to use a mapped_file_region for something else. Perhaps it's ok for the time being though?

[rnk]

In D42925#998357, @zturner wrote:

In D42925#998354, @rnk wrote:

One concern I have is that this is a common pattern on Linux:

int fd = open(...);
void *mem = mmap(fd, ...);
close(fd);
// use mem

Are we reasonably confident that LLVM doesn't have this pattern anywhere in it? i.e. we never store the FD and then try to use it after closing it?

We don't do that anywhere *at the moment* that I can find, but I suppose it's a concern for future proofing this code. Someone could come along and add code like this after the fact. Currently the only place where we open a mapped_file_region for readwrite is in OnDiskBuffer, which stores a TempFile and then renames it atomically to the final path. We could put the flush logic inside of TempFile::keep() so that it flushes it before renaming. That might be a better option, although it wouldn't catch the case where someone comes along later and tries to use a mapped_file_region for something else. Perhaps it's ok for the time being though?

It's probably fine. I tried to look for this pattern, but got lazy and punted it to you.

[thakis]

Did this land?

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

enum mapmode {
     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;
+  int FD;
+  mapmode Mode;
 
   std::error_code init(int 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;
 

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

std::error_code setLastModificationAndAccessTime(int FD, TimePoint<> Time) {
   HANDLE FileHandle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
   if (!SetFileTime(FileHandle, NULL, &FT, &FT))
     return mapWindowsError(::GetLastError());
   return std::error_code();
 }
 
 std::error_code mapped_file_region::init(int FD, uint64_t Offset,
                                          mapmode Mode) {
+  this->FD = FD;
+  this->Mode = Mode;
   HANDLE FileHandle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
   if (FileHandle == INVALID_HANDLE_VALUE)
     return make_error_code(errc::bad_file_descriptor);
 
   DWORD flprotect;
   switch (Mode) {
   case readonly:  flprotect = PAGE_READONLY; break;
   case readwrite: flprotect = PAGE_READWRITE; break;
mapped_file_region::mapped_file_region(int fd, mapmode mode, size_t length,
                                        uint64_t offset, std::error_code &ec)
     : Size(length), Mapping() {
   ec = init(fd, offset, mode);
   if (ec)
     Mapping = 0;
 }
 
 mapped_file_region::~mapped_file_region() {
-  if (Mapping)
+  if (Mapping) {
     ::UnmapViewOfFile(Mapping);
+
+    if (Mode == mapmode::readwrite) {
+      // There is a Windows kernel bug, the exact trigger conditions of which
+      // are not well understood.  When triggered, dirty pages are not properly
+      // flushed and subsequent process's attempts to read a file can return
+      // invalid data.  Calling FlushFileBuffers on the write handle is
+      // sufficient to ensure that this bug is not triggered.
+      HANDLE FileHandle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
+      if (FileHandle != INVALID_HANDLE_VALUE)
+        ::FlushFileBuffers(FileHandle);
+    }
+  }
 }
 
 size_t mapped_file_region::size() const {
   assert(Mapping && "Mapping failed but used anyway!");
   return Size;
 }
 
 char *mapped_file_region::data() const {