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

[Martin Storsjö via llvm-commits]

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

>From 4d4272d07f5705d80af0cb59e82e3010647b1043 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] Avoid a VirtualBox shared folders mmap bug

In acd8791c2619f2afc0347c1bff073b32fbffb5d6, a call to
FlushFileBuffers was added to work around a rare kernel bug. In
3b9b4d2156673edda50584086fbfb0d66460b4d2, the scope of that
workaround was limited, for performance reasons, as the flushes
are quite expensive.

On VirtualBox shared folders, closing a memory mapping that has
been written to, also needs to be explicitly flushed, if renaming
the output file before it is closed. Contrary to the kernel bug,
this always happens on such mounts. In these cases, the output
ends up as a file of the right size, but the contents are all
zeros.

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 for a full
reproduction example.

To avoid this issue, call FlushFileBuffers() when the file may
reside on a VitualBox shared folder. As the flushes are expensive,
only do them when the output isn't on a local file system.

The issue with VirtualBox shared folders could also be fixed
by calling FlushViewOfFile before UnmapViewOfFile, and doing that
could be slightly less expensive than FlushFileBuffers.

Empirically, the difference between the two is very small though,
and as it's not easy to verify whether switching FlushFileBuffers
to FlushViewOfFile helps with the rare kernel bug, keep using
FlushFileBuffers for both cases, for code simplicity.

This fixes downstream bug
https://github.com/mstorsjo/llvm-mingw/issues/393.
---
 llvm/lib/Support/Windows/Path.inc | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc
index 2bf68b7972e746f..9f758729e627185 100644
--- a/llvm/lib/Support/Windows/Path.inc
+++ b/llvm/lib/Support/Windows/Path.inc
@@ -959,7 +959,8 @@ void mapped_file_region::unmapImpl() {
 
     ::UnmapViewOfFile(Mapping);
 
-    if (Mode == mapmode::readwrite && Exe && hasFlushBufferKernelBug()) {
+    if (Mode == mapmode::readwrite) {
+      bool DoFlush = Exe && hasFlushBufferKernelBug();
       // 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,7 +968,27 @@ 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.
-      ::FlushFileBuffers(FileHandle);
+      if (!DoFlush) {
+        // 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.
+        //
+        // As the flushing is quite expensive, use a heuristic to limit the
+        // cases where we do the flushing. Only do the flushing if we aren't
+        // sure we are on a local file system.
+        bool IsLocal = false;
+        SmallVector<wchar_t, 128> FinalPath;
+        if (!realPathFromHandle(FileHandle, FinalPath)) {
+          // Not checking the return value here - if the check fails, assume the
+          // file isn't local.
+          is_local_internal(FinalPath, IsLocal);
+        }
+        DoFlush = !IsLocal;
+      }
+      if (DoFlush)
+        ::FlushFileBuffers(FileHandle);
     }
 
     ::CloseHandle(FileHandle);