[llvm] 7daa182 - Fix tmp files being left on Windows builds.
Amy Huang via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 1 17:14:13 PDT 2021
Author: Amy Huang
Date: 2021-06-01T17:09:08-07:00
New Revision: 7daa18215905c831e130c7542f17619e9d936dfc
URL: https://github.com/llvm/llvm-project/commit/7daa18215905c831e130c7542f17619e9d936dfc
DIFF: https://github.com/llvm/llvm-project/commit/7daa18215905c831e130c7542f17619e9d936dfc.diff
LOG: Fix tmp files being left on Windows builds.
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
Added:
Modified:
clang/include/clang/Frontend/CompilerInstance.h
clang/lib/Frontend/CompilerInstance.cpp
llvm/lib/Support/Path.cpp
llvm/lib/Support/Windows/Path.inc
Removed:
################################################################################
diff --git a/clang/include/clang/Frontend/CompilerInstance.h b/clang/include/clang/Frontend/CompilerInstance.h
index 5e879d672d801..9d6dd8fa1006f 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 <cassert>
#include <list>
#include <memory>
@@ -165,11 +166,10 @@ class CompilerInstance : public ModuleLoader {
/// failed.
struct OutputFile {
std::string Filename;
- std::string TempFilename;
+ Optional<llvm::sys::fs::TempFile> File;
- OutputFile(std::string filename, std::string tempFilename)
- : Filename(std::move(filename)), TempFilename(std::move(tempFilename)) {
- }
+ OutputFile(std::string filename, Optional<llvm::sys::fs::TempFile> 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 5df40c742d469..b1b983d620653 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<llvm::sys::fs::TempFile> Temp;
if (UseTemporary) {
// Create a temporary file.
// Insert -%%%%%%%% before the extension (if any), and because some tools
@@ -821,25 +827,36 @@ 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<llvm::sys::fs::TempFile> 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 (E) {
+ consumeError(std::move(E));
+ } else {
+ Temp = std::move(ExpectedFile.get());
+ TempPath = Temp->TmpName;
- if (!EC) {
- OS.reset(new llvm::raw_fd_ostream(fd, /*shouldClose=*/true));
- OSFile = TempFile = std::string(TempPath.str());
+ OS.reset(new llvm::raw_fd_ostream(Temp->FD, /*shouldClose=*/false,
+ Binary ? llvm::sys::fs::OF_None
+ : llvm::sys::fs::OF_Text));
+ OSFile = std::string(TempPath.str());
}
// 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
@@ -863,7 +880,7 @@ CompilerInstance::createOutputFileImpl(StringRef OutputPath, bool Binary,
// 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 005c27c3a42af..d549b0011b1f4 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<HANDLE>(_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 19b9f9ef7cbce..daa69bf04b44e 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<HANDLE>(_get_osfhandle(FromFD));
- return rename_handle(FromHandle, To);
-}
-
std::error_code rename(const Twine &From, const Twine &To) {
// Convert to utf-16.
SmallVector<wchar_t, 128> WideFrom;
More information about the llvm-commits
mailing list