Index: llvm/include/llvm/Support/FileSystem.h =================================================================== --- llvm/include/llvm/Support/FileSystem.h +++ llvm/include/llvm/Support/FileSystem.h @@ -786,10 +786,14 @@ // Keep this with the temporary name. Error keep(); - // Delete the file. + // Close the file. This needs to be called after calling keep() unless keep() + // returned an error. + Error close(); + + // Delete and close the file. Error discard(); - // This checks that keep or delete was called. + // This checks that keep and close or discard was called. ~TempFile(); }; @@ -1043,7 +1047,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/lib/LTO/Caching.cpp =================================================================== --- llvm/lib/LTO/Caching.cpp +++ llvm/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"); @@ -63,16 +70,6 @@ // Make sure the stream is closed before committing it. OS.reset(); - // Open the file first to avoid racing with a cache pruner. - ErrorOr> MBOrErr = - MemoryBuffer::getOpenFile(TempFile.FD, TempFile.TmpName, - /*FileSize*/ -1, - /*RequiresNullTerminator*/ false); - if (!MBOrErr) - report_fatal_error(Twine("Failed to open new cache file ") + - TempFile.TmpName + ": " + - MBOrErr.getError().message() + "\n"); - // On POSIX systems, this will atomically replace the destination if // it already exists. We try to emulate this on Windows, but this may // fail with a permission denied error (for example, if the destination @@ -88,6 +85,15 @@ if (EC != errc::permission_denied) return errorCodeToError(EC); + ErrorOr> MBOrErr = + MemoryBuffer::getOpenFile(TempFile.FD, TempFile.TmpName, + /*FileSize*/ -1, + /*RequiresNullTerminator*/ false); + if (!MBOrErr) + report_fatal_error(Twine("Failed to map cache file ") + + TempFile.TmpName + ": " + + MBOrErr.getError().message() + "\n"); + auto MBCopy = MemoryBuffer::getMemBufferCopy((*MBOrErr)->getBuffer(), EntryPath); MBOrErr = std::move(MBCopy); @@ -103,6 +109,24 @@ TempFile.TmpName + " to " + EntryPath + ": " + toString(std::move(E)) + "\n"); + // We must map the file after renaming it because on Windows that + // replaces the original file handle (which was opened with + // delete-on-close) with a new file handle that was opened without + // delete-on-close, and if a file handle opened with delete-on-close is + // kept alive that prevents other processes from opening the file. + ErrorOr> MBOrErr = + MemoryBuffer::getOpenFile(TempFile.FD, EntryPath, + /*FileSize*/ -1, + /*RequiresNullTerminator*/ false); + if (!MBOrErr) + report_fatal_error(Twine("Failed to map cache file ") + + EntryPath + ": " + + MBOrErr.getError().message() + "\n"); + + if (Error E = TempFile.close()) + report_fatal_error(Twine("Failed to close ") + EntryPath + ": " + + toString(std::move(E)) + "\n"); + AddBuffer(Task, std::move(*MBOrErr)); } }; Index: llvm/lib/Object/ArchiveWriter.cpp =================================================================== --- llvm/lib/Object/ArchiveWriter.cpp +++ llvm/lib/Object/ArchiveWriter.cpp @@ -530,5 +530,7 @@ // closed before we attempt to rename. OldArchiveBuf.reset(); - return Temp->keep(ArcName); + if (Error E = Temp->keep(ArcName)) + return E; + return Temp->close(); } Index: llvm/lib/Support/FileOutputBuffer.cpp =================================================================== --- llvm/lib/Support/FileOutputBuffer.cpp +++ llvm/lib/Support/FileOutputBuffer.cpp @@ -51,7 +51,9 @@ Buffer.reset(); // Atomically replace the existing file with the new one. - return Temp.keep(FinalPath); + if (Error E = Temp.keep(FinalPath)) + return E; + return Temp.close(); } ~OnDiskBuffer() override { Index: llvm/lib/Support/Path.cpp =================================================================== --- llvm/lib/Support/Path.cpp +++ llvm/lib/Support/Path.cpp @@ -1094,10 +1094,11 @@ TmpName = std::move(Other.TmpName); FD = Other.FD; Other.Done = true; + Other.FD = -1; return *this; } -TempFile::~TempFile() { assert(Done); } +TempFile::~TempFile() { assert(Done && FD == -1); } Error TempFile::discard() { Done = true; @@ -1114,7 +1115,8 @@ if (!RemoveEC) TmpName = ""; - if (FD != -1 && close(FD) == -1) { + if (FD != -1 && ::close(FD) == -1) { + FD = -1; std::error_code EC = std::error_code(errno, std::generic_category()); return errorCodeToError(EC); } @@ -1143,14 +1145,12 @@ sys::DontRemoveFileOnSignal(TmpName); #endif - if (!RenameEC) + if (RenameEC) { + ::close(FD); + FD = -1; + } else { TmpName = ""; - - if (close(FD) == -1) { - std::error_code EC(errno, std::generic_category()); - return errorCodeToError(EC); } - FD = -1; return errorCodeToError(RenameEC); } @@ -1160,15 +1160,25 @@ Done = true; #ifdef _WIN32 - if (std::error_code EC = cancelDeleteOnClose(FD)) + if (std::error_code EC = cancelDeleteOnClose(FD)) { + ::close(FD); + FD = -1; return errorCodeToError(EC); + } #else sys::DontRemoveFileOnSignal(TmpName); #endif TmpName = ""; - if (close(FD) == -1) { + return Error::success(); +} + +Error TempFile::close() { + assert(Done && FD != -1); + + if (::close(FD) == -1) { + FD = -1; std::error_code EC(errno, std::generic_category()); return errorCodeToError(EC); } Index: llvm/lib/Support/Unix/Path.inc =================================================================== --- llvm/lib/Support/Unix/Path.inc +++ llvm/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/lib/Support/Windows/Path.inc =================================================================== --- llvm/lib/Support/Windows/Path.inc +++ llvm/lib/Support/Windows/Path.inc @@ -826,10 +826,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 +839,7 @@ } HANDLE FileMappingHandle = - ::CreateFileMappingW(FileHandle, 0, flprotect, + ::CreateFileMappingW(OrigFileHandle, 0, flprotect, Hi_32(Size), Lo_32(Size), 0); @@ -878,9 +877,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 due to what is believed to be a Windows kernel bug, 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 +912,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); } } Index: llvm/tools/bugpoint/BugDriver.cpp =================================================================== --- llvm/tools/bugpoint/BugDriver.cpp +++ llvm/tools/bugpoint/BugDriver.cpp @@ -34,8 +34,12 @@ DiscardTemp::~DiscardTemp() { if (SaveTemps) { - if (Error E = File.keep()) + if (Error E = File.keep()) { errs() << "Failed to keep temp file " << toString(std::move(E)) << '\n'; + return; + } + if (Error E = File.close()) + errs() << "Failed to close temp file " << toString(std::move(E)) << '\n'; return; } if (Error E = File.discard())