[llvm] r334630 - LTO: Keep file handles open for memory mapped files.
Peter Collingbourne via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 13 11:03:14 PDT 2018
Author: pcc
Date: Wed Jun 13 11:03:14 2018
New Revision: 334630
URL: http://llvm.org/viewvc/llvm-project?rev=334630&view=rev
Log:
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
Modified:
llvm/trunk/include/llvm/Support/FileSystem.h
llvm/trunk/include/llvm/Support/LockFileManager.h
llvm/trunk/lib/LTO/Caching.cpp
llvm/trunk/lib/Support/LockFileManager.cpp
llvm/trunk/lib/Support/Path.cpp
llvm/trunk/lib/Support/Unix/Path.inc
llvm/trunk/lib/Support/Windows/Path.inc
Modified: llvm/trunk/include/llvm/Support/FileSystem.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/FileSystem.h?rev=334630&r1=334629&r2=334630&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Support/FileSystem.h (original)
+++ llvm/trunk/include/llvm/Support/FileSystem.h Wed Jun 13 11:03:14 2018
@@ -1043,7 +1043,9 @@ private:
/// Platform-specific mapping state.
size_t Size;
void *Mapping;
- int FD;
+#ifdef _WIN32
+ void *FileHandle;
+#endif
mapmode Mode;
std::error_code init(int FD, uint64_t Offset, mapmode Mode);
Modified: llvm/trunk/include/llvm/Support/LockFileManager.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/LockFileManager.h?rev=334630&r1=334629&r2=334630&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Support/LockFileManager.h (original)
+++ llvm/trunk/include/llvm/Support/LockFileManager.h Wed Jun 13 11:03:14 2018
@@ -11,7 +11,6 @@
#include "llvm/ADT/Optional.h"
#include "llvm/ADT/SmallString.h"
-#include "llvm/Support/FileSystem.h"
#include <system_error>
#include <utility> // for std::pair
@@ -54,7 +53,7 @@ public:
private:
SmallString<128> FileName;
SmallString<128> LockFileName;
- Optional<sys::fs::TempFile> UniqueLockFile;
+ SmallString<128> UniqueLockFileName;
Optional<std::pair<std::string, int> > Owner;
std::error_code ErrorCode;
Modified: llvm/trunk/lib/LTO/Caching.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/LTO/Caching.cpp?rev=334630&r1=334629&r2=334630&view=diff
==============================================================================
--- llvm/trunk/lib/LTO/Caching.cpp (original)
+++ llvm/trunk/lib/LTO/Caching.cpp Wed Jun 13 11:03:14 2018
@@ -40,7 +40,14 @@ Expected<NativeObjectCache> lto::localCa
return AddStreamFn();
}
- if (MBOrErr.getError() != errc::no_such_file_or_directory)
+ // On Windows we can fail to open a cache file with a permission denied
+ // error. This generally means that another process has requested to delete
+ // the file while it is still open, but it could also mean that another
+ // process has opened the file without the sharing permissions we need.
+ // Since the file is probably being deleted we handle it in the same way as
+ // if the file did not exist at all.
+ if (MBOrErr.getError() != errc::no_such_file_or_directory &&
+ MBOrErr.getError() != errc::permission_denied)
report_fatal_error(Twine("Failed to open cache file ") + EntryPath +
": " + MBOrErr.getError().message() + "\n");
Modified: llvm/trunk/lib/Support/LockFileManager.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/LockFileManager.cpp?rev=334630&r1=334629&r2=334630&view=diff
==============================================================================
--- llvm/trunk/lib/Support/LockFileManager.cpp (original)
+++ llvm/trunk/lib/Support/LockFileManager.cpp Wed Jun 13 11:03:14 2018
@@ -9,10 +9,8 @@
#include "llvm/Support/LockFileManager.h"
#include "llvm/ADT/None.h"
-#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringExtras.h"
-#include "llvm/Config/llvm-config.h"
#include "llvm/Support/Errc.h"
#include "llvm/Support/ErrorOr.h"
#include "llvm/Support/FileSystem.h"
@@ -123,6 +121,39 @@ bool LockFileManager::processStillExecut
return true;
}
+namespace {
+
+/// An RAII helper object ensure that the unique lock file is removed.
+///
+/// Ensures that if there is an error or a signal before we finish acquiring the
+/// lock, the unique file will be removed. And if we successfully take the lock,
+/// the signal handler is left in place so that signals while the lock is held
+/// will remove the unique lock file. The caller should ensure there is a
+/// matching call to sys::DontRemoveFileOnSignal when the lock is released.
+class RemoveUniqueLockFileOnSignal {
+ StringRef Filename;
+ bool RemoveImmediately;
+public:
+ RemoveUniqueLockFileOnSignal(StringRef Name)
+ : Filename(Name), RemoveImmediately(true) {
+ sys::RemoveFileOnSignal(Filename, nullptr);
+ }
+
+ ~RemoveUniqueLockFileOnSignal() {
+ if (!RemoveImmediately) {
+ // Leave the signal handler enabled. It will be removed when the lock is
+ // released.
+ return;
+ }
+ sys::fs::remove(Filename);
+ sys::DontRemoveFileOnSignal(Filename);
+ }
+
+ void lockAcquired() { RemoveImmediately = false; }
+};
+
+} // end anonymous namespace
+
LockFileManager::LockFileManager(StringRef FileName)
{
this->FileName = FileName;
@@ -141,22 +172,16 @@ LockFileManager::LockFileManager(StringR
return;
// Create a lock file that is unique to this instance.
- Expected<sys::fs::TempFile> Temp =
- sys::fs::TempFile::create(LockFileName + "-%%%%%%%%");
- if (!Temp) {
- std::error_code EC = errorToErrorCode(Temp.takeError());
- std::string S("failed to create unique file with prefix ");
- S.append(LockFileName.str());
+ UniqueLockFileName = LockFileName;
+ UniqueLockFileName += "-%%%%%%%%";
+ int UniqueLockFileID;
+ if (std::error_code EC = sys::fs::createUniqueFile(
+ UniqueLockFileName, UniqueLockFileID, UniqueLockFileName)) {
+ std::string S("failed to create unique file ");
+ S.append(UniqueLockFileName.str());
setError(EC, S);
return;
}
- UniqueLockFile = std::move(*Temp);
-
- // Make sure we discard the temporary file on exit.
- auto RemoveTempFile = llvm::make_scope_exit([&]() {
- if (Error E = UniqueLockFile->discard())
- setError(errorToErrorCode(std::move(E)));
- });
// Write our process ID to our unique lock file.
{
@@ -166,46 +191,54 @@ LockFileManager::LockFileManager(StringR
return;
}
- raw_fd_ostream Out(UniqueLockFile->FD, /*shouldClose=*/false);
+ raw_fd_ostream Out(UniqueLockFileID, /*shouldClose=*/true);
Out << HostID << ' ';
#if LLVM_ON_UNIX
Out << getpid();
#else
Out << "1";
#endif
- Out.flush();
+ Out.close();
if (Out.has_error()) {
// We failed to write out PID, so report the error, remove the
// unique lock file, and fail.
std::string S("failed to write to ");
- S.append(UniqueLockFile->TmpName);
+ S.append(UniqueLockFileName.str());
setError(Out.error(), S);
+ sys::fs::remove(UniqueLockFileName);
return;
}
}
+ // Clean up the unique file on signal, which also releases the lock if it is
+ // held since the .lock symlink will point to a nonexistent file.
+ RemoveUniqueLockFileOnSignal RemoveUniqueFile(UniqueLockFileName);
+
while (true) {
// Create a link from the lock file name. If this succeeds, we're done.
std::error_code EC =
- sys::fs::create_link(UniqueLockFile->TmpName, LockFileName);
+ sys::fs::create_link(UniqueLockFileName, LockFileName);
if (!EC) {
- RemoveTempFile.release();
+ RemoveUniqueFile.lockAcquired();
return;
}
if (EC != errc::file_exists) {
std::string S("failed to create link ");
raw_string_ostream OSS(S);
- OSS << LockFileName.str() << " to " << UniqueLockFile->TmpName;
+ OSS << LockFileName.str() << " to " << UniqueLockFileName.str();
setError(EC, OSS.str());
return;
}
// Someone else managed to create the lock file first. Read the process ID
// from the lock file.
- if ((Owner = readLockFile(LockFileName)))
- return; // RemoveTempFile will delete out our unique lock file.
+ if ((Owner = readLockFile(LockFileName))) {
+ // Wipe out our unique lock file (it's useless now)
+ sys::fs::remove(UniqueLockFileName);
+ return;
+ }
if (!sys::fs::exists(LockFileName)) {
// The previous owner released the lock file before we could read it.
@@ -217,7 +250,7 @@ LockFileManager::LockFileManager(StringR
// ownership.
if ((EC = sys::fs::remove(LockFileName))) {
std::string S("failed to remove lockfile ");
- S.append(LockFileName.str());
+ S.append(UniqueLockFileName.str());
setError(EC, S);
return;
}
@@ -252,7 +285,10 @@ LockFileManager::~LockFileManager() {
// Since we own the lock, remove the lock file and our own unique lock file.
sys::fs::remove(LockFileName);
- consumeError(UniqueLockFile->discard());
+ sys::fs::remove(UniqueLockFileName);
+ // The unique file is now gone, so remove it from the signal handler. This
+ // matches a sys::RemoveFileOnSignal() in LockFileManager().
+ sys::DontRemoveFileOnSignal(UniqueLockFileName);
}
LockFileManager::WaitForUnlockResult LockFileManager::waitForUnlock() {
Modified: llvm/trunk/lib/Support/Path.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Path.cpp?rev=334630&r1=334629&r2=334630&view=diff
==============================================================================
--- llvm/trunk/lib/Support/Path.cpp (original)
+++ llvm/trunk/lib/Support/Path.cpp Wed Jun 13 11:03:14 2018
@@ -1129,12 +1129,13 @@ Error TempFile::keep(const Twine &Name)
// Always try to close and rename.
#ifdef _WIN32
// If we cant't cancel the delete don't rename.
- std::error_code RenameEC = cancelDeleteOnClose(FD);
+ auto H = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
+ std::error_code RenameEC = setDeleteDisposition(H, false);
if (!RenameEC)
RenameEC = rename_fd(FD, Name);
// If we can't rename, discard the temporary file.
if (RenameEC)
- removeFD(FD);
+ setDeleteDisposition(H, true);
#else
std::error_code RenameEC = fs::rename(TmpName, Name);
// If we can't rename, discard the temporary file.
@@ -1160,7 +1161,8 @@ Error TempFile::keep() {
Done = true;
#ifdef _WIN32
- if (std::error_code EC = cancelDeleteOnClose(FD))
+ auto H = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
+ if (std::error_code EC = setDeleteDisposition(H, false))
return errorCodeToError(EC);
#else
sys::DontRemoveFileOnSignal(TmpName);
Modified: llvm/trunk/lib/Support/Unix/Path.inc
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Unix/Path.inc?rev=334630&r1=334629&r2=334630&view=diff
==============================================================================
--- llvm/trunk/lib/Support/Unix/Path.inc (original)
+++ llvm/trunk/lib/Support/Unix/Path.inc Wed Jun 13 11:03:14 2018
@@ -634,8 +634,7 @@ std::error_code mapped_file_region::init
mapped_file_region::mapped_file_region(int fd, mapmode mode, size_t length,
uint64_t offset, std::error_code &ec)
- : Size(length), Mapping(), FD(fd), Mode(mode) {
- (void)FD;
+ : Size(length), Mapping(), Mode(mode) {
(void)Mode;
ec = init(fd, offset, mode);
if (ec)
Modified: llvm/trunk/lib/Support/Windows/Path.inc
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/Windows/Path.inc?rev=334630&r1=334629&r2=334630&view=diff
==============================================================================
--- llvm/trunk/lib/Support/Windows/Path.inc (original)
+++ llvm/trunk/lib/Support/Windows/Path.inc Wed Jun 13 11:03:14 2018
@@ -404,56 +404,6 @@ static std::error_code setDeleteDisposit
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 setLastModificationAndAc
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 @@ std::error_code mapped_file_region::init
}
HANDLE FileMappingHandle =
- ::CreateFileMappingW(FileHandle, 0, flprotect,
+ ::CreateFileMappingW(OrigFileHandle, 0, flprotect,
Hi_32(Size),
Lo_32(Size),
0);
@@ -878,9 +827,20 @@ std::error_code mapped_file_region::init
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 @@ mapped_file_region::~mapped_file_region(
// 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 @@ static std::error_code nativeFileToFd(Ex
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 @@ Expected<file_t> openNativeFile(const Tw
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 @@ Expected<file_t> openNativeFile(const Tw
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;
}
More information about the llvm-commits
mailing list