Index: llvm/trunk/include/llvm/Support/FileSystem.h =================================================================== --- llvm/trunk/include/llvm/Support/FileSystem.h +++ llvm/trunk/include/llvm/Support/FileSystem.h @@ -1043,7 +1043,9 @@ /// 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); Index: llvm/trunk/include/llvm/Support/LockFileManager.h =================================================================== --- llvm/trunk/include/llvm/Support/LockFileManager.h +++ llvm/trunk/include/llvm/Support/LockFileManager.h @@ -11,7 +11,6 @@ #include "llvm/ADT/Optional.h" #include "llvm/ADT/SmallString.h" -#include "llvm/Support/FileSystem.h" #include #include // for std::pair @@ -54,7 +53,7 @@ private: SmallString<128> FileName; SmallString<128> LockFileName; - Optional UniqueLockFile; + SmallString<128> UniqueLockFileName; Optional > Owner; std::error_code ErrorCode; Index: llvm/trunk/lib/LTO/Caching.cpp =================================================================== --- llvm/trunk/lib/LTO/Caching.cpp +++ llvm/trunk/lib/LTO/Caching.cpp @@ -40,7 +40,14 @@ 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"); Index: llvm/trunk/lib/Support/LockFileManager.cpp =================================================================== --- llvm/trunk/lib/Support/LockFileManager.cpp +++ llvm/trunk/lib/Support/LockFileManager.cpp @@ -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 @@ 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 @@ return; // Create a lock file that is unique to this instance. - Expected 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 @@ 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 @@ // 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 @@ // 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() { Index: llvm/trunk/lib/Support/Path.cpp =================================================================== --- llvm/trunk/lib/Support/Path.cpp +++ llvm/trunk/lib/Support/Path.cpp @@ -1129,12 +1129,13 @@ // 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(_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 @@ Done = true; #ifdef _WIN32 - if (std::error_code EC = cancelDeleteOnClose(FD)) + auto H = reinterpret_cast(_get_osfhandle(FD)); + if (std::error_code EC = setDeleteDisposition(H, false)) return errorCodeToError(EC); #else sys::DontRemoveFileOnSignal(TmpName); Index: llvm/trunk/lib/Support/Unix/Path.inc =================================================================== --- llvm/trunk/lib/Support/Unix/Path.inc +++ llvm/trunk/lib/Support/Unix/Path.inc @@ -634,8 +634,7 @@ 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) Index: llvm/trunk/lib/Support/Windows/Path.inc =================================================================== --- llvm/trunk/lib/Support/Windows/Path.inc +++ llvm/trunk/lib/Support/Windows/Path.inc @@ -404,56 +404,6 @@ return std::error_code(); } -static std::error_code removeFD(int FD) { - HANDLE Handle = reinterpret_cast(_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(_get_osfhandle(FD)); - SmallVector 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 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(_get_osfhandle(FD)); - if (FileHandle == INVALID_HANDLE_VALUE) + HANDLE OrigFileHandle = reinterpret_cast(_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(_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; }