[llvm] [Support] Always call FlushFileBuffers() when unmapping memory on Windows (PR #78597)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lld

Author: Martin Storsjö (mstorsjo)

<details>
<summary>Changes</summary>

This reverts the functional parts of commit
3b9b4d2156673edda50584086fbfb0d66460b4d2.

The conditional call to FlushFileBuffers was added to work around a rare kernel bug, see acd8791c2619f2afc0347c1bff073b32fbffb5d6 and 3b9b4d2156673edda50584086fbfb0d66460b4d2.

However, closing a memory mapping that has been written to, can also need a call to FlushFileBuffers in other cases. When operating on a VirtualBox Shared Folder file system, the output file ends up unwritten/unflushed (with the right size but all zeros), if written via a memory mapping, then the file is renamed before the output handle is closed (this is what is done in lld-link).

The sequence to trigger the issue on the VirtualBox Shared Folders is this, summarized:

    file = CreateFile()
    mapping = CreateFileMapping(file)
    mem = MapViewOfFile()
    CloseHandle(mapping)
    write(mem)
    UnmapViewOfFile(mem)
    SetFileInformationByHandle(file, FileRenameInfo)
    CloseHandle(file)

With this sequence, the output file always ends up with all zeros. See https://github.com/mstorsjo/llvm-mingw/issues/393#issuecomment-1895640756 for a full reproduction example.

The issue can seem elusive, as we conditionally do the call to FlushFileBuffers() to work around the rare kernel bug, whenever running on versions of Windows older than 10.0.0.17763.

This fixes downstream bug
https://github.com/mstorsjo/llvm-mingw/issues/393.

---
Full diff: https://github.com/llvm/llvm-project/pull/78597.diff


1 Files Affected:

- (modified) llvm/lib/Support/Windows/Path.inc (+6-20) 


``````````diff
diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc
index 2bf68b7972e746f..204dc412972d63d 100644
--- a/llvm/lib/Support/Windows/Path.inc
+++ b/llvm/lib/Support/Windows/Path.inc
@@ -936,30 +936,11 @@ mapped_file_region::mapped_file_region(sys::fs::file_t fd, mapmode mode,
     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.starts_with(StringRef("MZ")) && Magic.size() >= 0x3c + 4) {
-    uint32_t off = read32le(Magic.data() + 0x3c);
-    // PE/COFF file, either EXE or DLL.
-    if (Magic.substr(off).starts_with(StringRef(PEMagic, sizeof(PEMagic))))
-      return true;
-  }
-  return false;
-}
-
 void mapped_file_region::unmapImpl() {
   if (Mapping) {
-
-    bool Exe = isEXE(StringRef((char *)Mapping, Size));
-
     ::UnmapViewOfFile(Mapping);
 
-    if (Mode == mapmode::readwrite && Exe && hasFlushBufferKernelBug()) {
+    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
@@ -967,6 +948,11 @@ void mapped_file_region::unmapImpl() {
       // sufficient to ensure that this bug is not triggered.
       // The bug only occurs when writing an executable and executing it right
       // after, under high I/O pressure.
+      //
+      // Separately, on VirtualBox Shared Folder mounts, writes via memory maps
+      // always end up unflushed (regardless of version of Windows, unless flushed
+      // with this explicit call, if they are renamed with
+      // SetFileInformationByHandle(FileRenameInfo) before closing the output handle.
       ::FlushFileBuffers(FileHandle);
     }
 

``````````

</details>


https://github.com/llvm/llvm-project/pull/78597