LTO: Keep file handles open for memory mapped files.
On Windows we've observed that if you open a file, write to it, map it into
memory and close the file handle, the contents of the memory mapping can
sometimes be incorrect. That was what we did when adding an entry to the
ThinLTO cache using the TempFile and MemoryBuffer classes, and it was causing
intermittent build failures on Chromium's ThinLTO bots on Windows. More
details are in the associated Chromium bug (crbug.com/786127).
We can prevent this from happening by keeping a handle to the file open while
the mapping is active. So this patch changes the mapped_file_region class to
duplicate the file handle when mapping the file and close it upon unmapping it.
One gotcha is that the file handle that we keep open must not have been
created with FILE_FLAG_DELETE_ON_CLOSE, as otherwise the operating system
will prevent other processes from opening the file. We can achieve this
by avoiding the use of FILE_FLAG_DELETE_ON_CLOSE altogether. Instead,
we use SetFileInformationByHandle with FileDispositionInfo to manage the
delete-on-close bit. This lets us remove the hack that we used to use to
clear the delete-on-close bit on a file opened with FILE_FLAG_DELETE_ON_CLOSE.
A downside of using SetFileInformationByHandle/FileDispositionInfo as
opposed to FILE_FLAG_DELETE_ON_CLOSE is that it prevents us from using
CreateFile to open the file while the flag is set, even within the same
process. This doesn't seem to matter for almost every client of TempFile,
except for LockFileManager, which calls sys::fs::create_link to create a
hard link from the lock file, and in the process of doing so tries to open
the file. To prevent this change from breaking LockFileManager I changed it
to stop using TempFile by effectively reverting r318550.
Differential Revision: https://reviews.llvm.org/D48051
llvm-svn: 334630
diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc
index 7ef7468..577d571 100644
--- a/llvm/lib/Support/Windows/Path.inc
+++ b/llvm/lib/Support/Windows/Path.inc
@@ -404,56 +404,6 @@
return std::error_code();
}
-static std::error_code removeFD(int FD) {
- HANDLE Handle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
- return setDeleteDisposition(Handle, true);
-}
-
-/// In order to handle temporary files we want the following properties
-///
-/// * The temporary file is deleted on crashes
-/// * We can use (read, rename, etc) the temporary file.
-/// * We can cancel the delete to keep the file.
-///
-/// Using FILE_DISPOSITION_INFO with DeleteFile=true will create a file that is
-/// deleted on close, but it has a few problems:
-///
-/// * The file cannot be used. An attempt to open or rename the file will fail.
-/// This makes the temporary file almost useless, as it cannot be part of
-/// any other CreateFileW call in the current or in another process.
-/// * It is not atomic. A crash just after CreateFileW or just after canceling
-/// the delete will leave the file on disk.
-///
-/// Using FILE_FLAG_DELETE_ON_CLOSE solves the first issues and the first part
-/// of the second one, but there is no way to cancel it in place. What works is
-/// to create a second handle to prevent the deletion, close the first one and
-/// then clear DeleteFile with SetFileInformationByHandle. This requires
-/// changing the handle and file descriptor the caller uses.
-static std::error_code cancelDeleteOnClose(int &FD) {
- HANDLE Handle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
- SmallVector<wchar_t, MAX_PATH> Name;
- if (std::error_code EC = realPathFromHandle(Handle, Name))
- return EC;
- HANDLE NewHandle =
- ::CreateFileW(Name.data(), GENERIC_READ | GENERIC_WRITE | DELETE,
- FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
- NULL, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
- if (NewHandle == INVALID_HANDLE_VALUE)
- return mapWindowsError(::GetLastError());
- if (close(FD))
- return mapWindowsError(::GetLastError());
-
- if (std::error_code EC = setDeleteDisposition(NewHandle, false))
- return EC;
-
- FD = ::_open_osfhandle(intptr_t(NewHandle), 0);
- if (FD == -1) {
- ::CloseHandle(NewHandle);
- return mapWindowsError(ERROR_INVALID_HANDLE);
- }
- return std::error_code();
-}
-
static std::error_code rename_internal(HANDLE FromHandle, const Twine &To,
bool ReplaceIfExists) {
SmallVector<wchar_t, 0> ToWide;
@@ -826,10 +776,9 @@
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)
+ HANDLE OrigFileHandle = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
+ if (OrigFileHandle == INVALID_HANDLE_VALUE)
return make_error_code(errc::bad_file_descriptor);
DWORD flprotect;
@@ -840,7 +789,7 @@
}
HANDLE FileMappingHandle =
- ::CreateFileMappingW(FileHandle, 0, flprotect,
+ ::CreateFileMappingW(OrigFileHandle, 0, flprotect,
Hi_32(Size),
Lo_32(Size),
0);
@@ -878,9 +827,20 @@
Size = mbi.RegionSize;
}
- // Close all the handles except for the view. It will keep the other handles
- // alive.
+ // Close the file mapping handle, as it's kept alive by the file mapping. But
+ // neither the file mapping nor the file mapping handle keep the file handle
+ // alive, so we need to keep a reference to the file in case all other handles
+ // are closed and the file is deleted, which may cause invalid data to be read
+ // from the file.
::CloseHandle(FileMappingHandle);
+ if (!::DuplicateHandle(::GetCurrentProcess(), OrigFileHandle,
+ ::GetCurrentProcess(), &FileHandle, 0, 0,
+ DUPLICATE_SAME_ACCESS)) {
+ std::error_code ec = mapWindowsError(GetLastError());
+ ::UnmapViewOfFile(Mapping);
+ return ec;
+ }
+
return std::error_code();
}
@@ -902,10 +862,10 @@
// 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);
+ ::FlushFileBuffers(FileHandle);
}
+
+ ::CloseHandle(FileHandle);
}
}
@@ -1056,16 +1016,6 @@
return std::error_code();
}
-static DWORD nativeOpenFlags(OpenFlags Flags) {
- DWORD Result = 0;
- if (Flags & OF_Delete)
- Result |= FILE_FLAG_DELETE_ON_CLOSE;
-
- if (Result == 0)
- Result = FILE_ATTRIBUTE_NORMAL;
- return Result;
-}
-
static DWORD nativeDisposition(CreationDisposition Disp, OpenFlags Flags) {
// This is a compatibility hack. Really we should respect the creation
// disposition, but a lot of old code relied on the implicit assumption that
@@ -1142,7 +1092,6 @@
assert((!(Disp == CD_CreateNew) || !(Flags & OF_Append)) &&
"Cannot specify both 'CreateNew' and 'Append' file creation flags!");
- DWORD NativeFlags = nativeOpenFlags(Flags);
DWORD NativeDisp = nativeDisposition(Disp, Flags);
DWORD NativeAccess = nativeAccess(Access, Flags);
@@ -1152,9 +1101,15 @@
file_t Result;
std::error_code EC = openNativeFileInternal(
- Name, Result, NativeDisp, NativeAccess, NativeFlags, Inherit);
+ Name, Result, NativeDisp, NativeAccess, FILE_ATTRIBUTE_NORMAL, Inherit);
if (EC)
return errorCodeToError(EC);
+ if (Flags & OF_Delete) {
+ if ((EC = setDeleteDisposition(Result, true))) {
+ ::CloseHandle(Result);
+ return errorCodeToError(EC);
+ }
+ }
return Result;
}