From 9d070b2f4889887f9ce497592ef01df7b9601a1c Mon Sep 17 00:00:00 2001 From: Amy Huang Date: Wed, 2 Jun 2021 09:49:12 -0700 Subject: [PATCH] Recommit "Fix tmp files being left on Windows builds." with a fix for incorrect std::string use. (Also remove redundant call to RemoveFileOnSignal.) Clang writes object files by first writing to a .tmp file and then renaming to the final .obj name. On Windows, if a compile is killed partway through the .tmp files don't get deleted. Currently it seems like RemoveFileOnSignal takes care of deleting the tmp files on Linux, but on Windows we need to call setDeleteDisposition on tmp files so that they are deleted when closed. This patch switches to using TempFile to create the .tmp files we write when creating object files, since it uses setDeleteDisposition on Windows. This change applies to both Linux and Windows for consistency. Differential Revision: https://reviews.llvm.org/D102876 This reverts commit 20797b129f844d4b12ffb2b12cf33baa2d42985c. --- clang/include/clang/Frontend/CompilerInstance.h | 8 +-- clang/lib/Frontend/CompilerInstance.cpp | 77 ++++++++++++++----------- llvm/lib/Support/Path.cpp | 2 +- llvm/lib/Support/Windows/Path.inc | 5 -- 4 files changed, 49 insertions(+), 43 deletions(-) diff --git a/clang/include/clang/Frontend/CompilerInstance.h b/clang/include/clang/Frontend/CompilerInstance.h index 5e879d6..9d6dd8f 100644 --- a/clang/include/clang/Frontend/CompilerInstance.h +++ b/clang/include/clang/Frontend/CompilerInstance.h @@ -22,6 +22,7 @@ #include "llvm/ADT/IntrusiveRefCntPtr.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/BuryPointer.h" +#include "llvm/Support/FileSystem.h" #include #include #include @@ -165,11 +166,10 @@ class CompilerInstance : public ModuleLoader { /// failed. struct OutputFile { std::string Filename; - std::string TempFilename; + Optional File; - OutputFile(std::string filename, std::string tempFilename) - : Filename(std::move(filename)), TempFilename(std::move(tempFilename)) { - } + OutputFile(std::string filename, Optional file) + : Filename(std::move(filename)), File(std::move(file)) {} }; /// The list of active output files. diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp index 5df40c7..32e7fdf 100644 --- a/clang/lib/Frontend/CompilerInstance.cpp +++ b/clang/lib/Frontend/CompilerInstance.cpp @@ -703,31 +703,37 @@ void CompilerInstance::createSema(TranslationUnitKind TUKind, // Output Files void CompilerInstance::clearOutputFiles(bool EraseFiles) { + // Ignore errors that occur when trying to discard the temp file. for (OutputFile &OF : OutputFiles) { if (EraseFiles) { - if (!OF.TempFilename.empty()) { - llvm::sys::fs::remove(OF.TempFilename); - continue; - } + if (OF.File) + consumeError(OF.File->discard()); if (!OF.Filename.empty()) llvm::sys::fs::remove(OF.Filename); continue; } - if (OF.TempFilename.empty()) + if (!OF.File) continue; + if (OF.File->TmpName.empty()) { + consumeError(OF.File->discard()); + continue; + } + // If '-working-directory' was passed, the output filename should be // relative to that. SmallString<128> NewOutFile(OF.Filename); FileMgr->FixupRelativePath(NewOutFile); - std::error_code EC = llvm::sys::fs::rename(OF.TempFilename, NewOutFile); - if (!EC) + + llvm::Error E = OF.File->keep(NewOutFile); + if (!E) continue; + getDiagnostics().Report(diag::err_unable_to_rename_temp) - << OF.TempFilename << OF.Filename << EC.message(); + << OF.File->TmpName << OF.Filename << std::move(E); - llvm::sys::fs::remove(OF.TempFilename); + llvm::sys::fs::remove(OF.File->TmpName); } OutputFiles.clear(); if (DeleteBuiltModules) { @@ -809,7 +815,7 @@ CompilerInstance::createOutputFileImpl(StringRef OutputPath, bool Binary, } } - std::string TempFile; + Optional Temp; if (UseTemporary) { // Create a temporary file. // Insert -%%%%%%%% before the extension (if any), and because some tools @@ -821,25 +827,34 @@ CompilerInstance::createOutputFileImpl(StringRef OutputPath, bool Binary, TempPath += "-%%%%%%%%"; TempPath += OutputExtension; TempPath += ".tmp"; - int fd; - std::error_code EC = llvm::sys::fs::createUniqueFile( - TempPath, fd, TempPath, - Binary ? llvm::sys::fs::OF_None : llvm::sys::fs::OF_Text); - - if (CreateMissingDirectories && - EC == llvm::errc::no_such_file_or_directory) { - StringRef Parent = llvm::sys::path::parent_path(OutputPath); - EC = llvm::sys::fs::create_directories(Parent); - if (!EC) { - EC = llvm::sys::fs::createUniqueFile(TempPath, fd, TempPath, - Binary ? llvm::sys::fs::OF_None - : llvm::sys::fs::OF_Text); - } - } + Expected ExpectedFile = + llvm::sys::fs::TempFile::create(TempPath); + + llvm::Error E = handleErrors( + ExpectedFile.takeError(), [&](const llvm::ECError &E) -> llvm::Error { + std::error_code EC = E.convertToErrorCode(); + if (CreateMissingDirectories && + EC == llvm::errc::no_such_file_or_directory) { + StringRef Parent = llvm::sys::path::parent_path(OutputPath); + EC = llvm::sys::fs::create_directories(Parent); + if (!EC) { + ExpectedFile = llvm::sys::fs::TempFile::create(TempPath); + if (!ExpectedFile) + return llvm::errorCodeToError( + llvm::errc::no_such_file_or_directory); + } + } + return llvm::errorCodeToError(EC); + }); - if (!EC) { - OS.reset(new llvm::raw_fd_ostream(fd, /*shouldClose=*/true)); - OSFile = TempFile = std::string(TempPath.str()); + if (E) { + consumeError(std::move(E)); + } else { + Temp = std::move(ExpectedFile.get()); + OS.reset(new llvm::raw_fd_ostream(Temp->FD, /*shouldClose=*/false, + Binary ? llvm::sys::fs::OF_None + : llvm::sys::fs::OF_Text)); + OSFile = Temp->TmpName; } // If we failed to create the temporary, fallback to writing to the file // directly. This handles the corner case where we cannot write to the @@ -856,14 +871,10 @@ CompilerInstance::createOutputFileImpl(StringRef OutputPath, bool Binary, return llvm::errorCodeToError(EC); } - // Make sure the out stream file gets removed if we crash. - if (RemoveFileOnSignal) - llvm::sys::RemoveFileOnSignal(*OSFile); - // Add the output file -- but don't try to remove "-", since this means we are // using stdin. OutputFiles.emplace_back(((OutputPath != "-") ? OutputPath : "").str(), - std::move(TempFile)); + std::move(Temp)); if (!Binary || OS->supportsSeeking()) return std::move(OS); diff --git a/llvm/lib/Support/Path.cpp b/llvm/lib/Support/Path.cpp index 005c27c..d549b00 100644 --- a/llvm/lib/Support/Path.cpp +++ b/llvm/lib/Support/Path.cpp @@ -1229,7 +1229,7 @@ Error TempFile::keep(const Twine &Name) { auto H = reinterpret_cast(_get_osfhandle(FD)); std::error_code RenameEC = setDeleteDisposition(H, false); if (!RenameEC) { - RenameEC = rename_fd(FD, Name); + RenameEC = rename_handle(H, Name); // If rename failed because it's cross-device, copy instead if (RenameEC == std::error_code(ERROR_NOT_SAME_DEVICE, std::system_category())) { diff --git a/llvm/lib/Support/Windows/Path.inc b/llvm/lib/Support/Windows/Path.inc index 19b9f9e..daa69bf 100644 --- a/llvm/lib/Support/Windows/Path.inc +++ b/llvm/lib/Support/Windows/Path.inc @@ -560,11 +560,6 @@ static std::error_code rename_handle(HANDLE FromHandle, const Twine &To) { return errc::permission_denied; } -static std::error_code rename_fd(int FromFD, const Twine &To) { - HANDLE FromHandle = reinterpret_cast(_get_osfhandle(FromFD)); - return rename_handle(FromHandle, To); -} - std::error_code rename(const Twine &From, const Twine &To) { // Convert to utf-16. SmallVector WideFrom; -- 2.7.4