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

[Martin Storsjö via llvm-commits]

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

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.

>From 76f6dbe8b78214bf54ac7481970f4c28f417ed7f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?= <martin at martin.st>
Date: Thu, 18 Jan 2024 16:54:38 +0200
Subject: [PATCH] [Support] Always call FlushFileBuffers() when unmapping
 memory on Windows

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.
---
 llvm/lib/Support/Windows/Path.inc | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

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);
     }