akhuang created this revision.
Herald added subscribers: dexonsmith, hiraditya.
akhuang requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.
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 uses llvm::sys::fs::TempFile to create these files on Windows.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D102876
Files:
clang/include/clang/Frontend/CompilerInstance.h
clang/lib/Frontend/CompilerInstance.cpp
llvm/include/llvm/Support/FileSystem.h
llvm/lib/Support/Path.cpp
llvm/lib/Support/Windows/Path.inc
Index: llvm/lib/Support/Windows/Path.inc
===================================================================
--- llvm/lib/Support/Windows/Path.inc
+++ llvm/lib/Support/Windows/Path.inc
@@ -435,6 +435,7 @@
if (!SetFileInformationByHandle(Handle, FileDispositionInfo, &Disposition,
sizeof(Disposition)))
return mapWindowsError(::GetLastError());
+
return std::error_code();
}
@@ -560,11 +561,6 @@
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;
@@ -1153,6 +1149,7 @@
return EC;
}
ResultFile = H;
+
return std::error_code();
}
Index: llvm/lib/Support/Path.cpp
===================================================================
--- llvm/lib/Support/Path.cpp
+++ llvm/lib/Support/Path.cpp
@@ -1182,14 +1182,16 @@
namespace llvm {
namespace sys {
namespace fs {
-TempFile::TempFile(StringRef Name, int FD)
- : TmpName(std::string(Name)), FD(FD) {}
+TempFile::TempFile(StringRef Name, int FD, file_t Handle = nullptr)
+ : TmpName(std::string(Name)), FD(FD), Handle(Handle) {}
TempFile::TempFile(TempFile &&Other) { *this = std::move(Other); }
TempFile &TempFile::operator=(TempFile &&Other) {
TmpName = std::move(Other.TmpName);
FD = Other.FD;
+ Handle = Other.Handle;
Other.Done = true;
Other.FD = -1;
+ Other.Handle = 0;
return *this;
}
@@ -1197,13 +1199,18 @@
Error TempFile::discard() {
Done = true;
- if (FD != -1 && close(FD) == -1) {
+ if (!Handle && FD != -1 && close(FD) == -1) {
std::error_code EC = std::error_code(errno, std::generic_category());
return errorCodeToError(EC);
}
FD = -1;
#ifdef _WIN32
+ if (Handle && !::CloseHandle(Handle)) {
+ std::error_code EC = mapWindowsError(::GetLastError());
+ return errorCodeToError(EC);
+ }
+
// On windows closing will remove the file.
TmpName = "";
return Error::success();
@@ -1226,10 +1233,10 @@
// Always try to close and rename.
#ifdef _WIN32
// If we can't cancel the delete don't rename.
- auto H = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
+ HANDLE H = (Handle) ? Handle : 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())) {
@@ -1256,12 +1263,20 @@
if (!RenameEC)
TmpName = "";
- if (close(FD) == -1) {
- std::error_code EC(errno, std::generic_category());
- return errorCodeToError(EC);
+ if (!Handle) {
+ if (close(FD) == -1) {
+ std::error_code EC(errno, std::generic_category());
+ return errorCodeToError(EC);
+ }
+ } else {
+#ifdef _WIN32
+ if (!::CloseHandle(Handle)) {
+ std::error_code EC = mapWindowsError(::GetLastError());
+ return errorCodeToError(EC);
+ }
+#endif
}
FD = -1;
-
return errorCodeToError(RenameEC);
}
@@ -1270,16 +1285,21 @@
Done = true;
#ifdef _WIN32
- auto H = reinterpret_cast<HANDLE>(_get_osfhandle(FD));
+ auto H = (Handle) ? Handle : reinterpret_cast<HANDLE>(_get_osfhandle(FD));
if (std::error_code EC = setDeleteDisposition(H, false))
return errorCodeToError(EC);
+
+ if (Handle && !::CloseHandle(Handle)) {
+ std::error_code EC = mapWindowsError(::GetLastError());
+ return errorCodeToError(EC);
+ }
#else
sys::DontRemoveFileOnSignal(TmpName);
#endif
TmpName = "";
- if (close(FD) == -1) {
+ if (!Handle && close(FD) == -1) {
std::error_code EC(errno, std::generic_category());
return errorCodeToError(EC);
}
@@ -1295,14 +1315,25 @@
createUniqueFile(Model, FD, ResultPath, OF_Delete, Mode))
return errorCodeToError(EC);
- TempFile Ret(ResultPath, FD);
#ifndef _WIN32
+ TempFile Ret(ResultPath, FD);
if (sys::RemoveFileOnSignal(ResultPath)) {
// Make sure we delete the file when RemoveFileOnSignal fails.
consumeError(Ret.discard());
std::error_code EC(errc::operation_not_permitted);
return errorCodeToError(EC);
}
+#else
+ file_t FileHandle = convertFDToNativeFile(FD);
+ file_t NewFileHandle;
+ if (!::DuplicateHandle(::GetCurrentProcess(), FileHandle,
+ ::GetCurrentProcess(), &NewFileHandle, 0, 0,
+ DUPLICATE_SAME_ACCESS)) {
+ std::error_code EC = mapWindowsError(GetLastError());
+ return errorCodeToError(EC);
+ }
+
+ TempFile Ret(ResultPath, FD, NewFileHandle);
#endif
return std::move(Ret);
}
Index: llvm/include/llvm/Support/FileSystem.h
===================================================================
--- llvm/include/llvm/Support/FileSystem.h
+++ llvm/include/llvm/Support/FileSystem.h
@@ -851,7 +851,7 @@
/// properly handle errors in a destructor.
class TempFile {
bool Done = false;
- TempFile(StringRef Name, int FD);
+ TempFile(StringRef Name, int FD, file_t Handle);
public:
/// This creates a temporary file with createUniqueFile and schedules it for
@@ -867,6 +867,9 @@
// The open file descriptor.
int FD = -1;
+ // An open handle, for Windows only.
+ void *Handle = nullptr;
+
// Keep this with the given name.
Error keep(const Twine &Name);
Index: clang/lib/Frontend/CompilerInstance.cpp
===================================================================
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -704,6 +704,10 @@
void CompilerInstance::clearOutputFiles(bool EraseFiles) {
for (OutputFile &OF : OutputFiles) {
if (EraseFiles) {
+ if (OF.TempFile) {
+ if (llvm::Error E = OF.TempFile->discard())
+ consumeError(std::move(E));
+ }
if (!OF.TempFilename.empty()) {
llvm::sys::fs::remove(OF.TempFilename);
continue;
@@ -720,11 +724,22 @@
// relative to that.
SmallString<128> NewOutFile(OF.Filename);
FileMgr->FixupRelativePath(NewOutFile);
- std::error_code EC = llvm::sys::fs::rename(OF.TempFilename, NewOutFile);
- if (!EC)
+
+ std::string ErrorMsg;
+ if (OF.TempFile) {
+ if (llvm::Error E = OF.TempFile->keep(NewOutFile))
+ ErrorMsg = toString(std::move(E));
+ } else {
+ if (std::error_code EC =
+ llvm::sys::fs::rename(OF.TempFilename, NewOutFile))
+ ErrorMsg = EC.message();
+ }
+
+ if (ErrorMsg.empty())
continue;
+
getDiagnostics().Report(diag::err_unable_to_rename_temp)
- << OF.TempFilename << OF.Filename << EC.message();
+ << OF.TempFilename << OF.Filename << ErrorMsg;
llvm::sys::fs::remove(OF.TempFilename);
}
@@ -808,7 +823,8 @@
}
}
- std::string TempFile;
+ std::string TempFileName;
+ Optional<llvm::sys::fs::TempFile> Temp;
if (UseTemporary) {
// Create a temporary file.
// Insert -%%%%%%%% before the extension (if any), and because some tools
@@ -821,9 +837,46 @@
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);
+ std::error_code EC;
+#if _WIN32
+ // On Windows, use llvm::sys::fs::TempFile to write the output file so
+ // that it is deleted if the program crashes.
+ llvm::sys::fs::perms Perms = llvm::sys::fs::all_read |
+ llvm::sys::fs::all_write |
+ llvm::sys::fs::all_exe;
+ Expected<llvm::sys::fs::TempFile> ExpectedFile =
+ llvm::sys::fs::TempFile::create(TempPath, Perms);
+
+ 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, Perms);
+ if (!ExpectedFile)
+ return llvm::errorCodeToError(
+ llvm::errc::no_such_file_or_directory);
+ }
+ }
+ return llvm::errorCodeToError(EC);
+ });
+
+ if (!E) {
+ Temp = std::move(ExpectedFile.get());
+ fd = Temp->FD;
+ TempPath = Temp->TmpName;
+ } else {
+ consumeError(std::move(E));
+ // Set EC to something because we need an error code here.
+ EC = llvm::errc::no_such_file_or_directory;
+ }
+#else
+ EC = 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) {
@@ -835,10 +888,10 @@
: llvm::sys::fs::OF_Text);
}
}
-
+#endif
if (!EC) {
OS.reset(new llvm::raw_fd_ostream(fd, /*shouldClose=*/true));
- OSFile = TempFile = std::string(TempPath.str());
+ OSFile = TempFileName = 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
@@ -862,7 +915,7 @@
// 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(TempFileName), std::move(Temp));
if (!Binary || OS->supportsSeeking())
return std::move(OS);
Index: clang/include/clang/Frontend/CompilerInstance.h
===================================================================
--- clang/include/clang/Frontend/CompilerInstance.h
+++ 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>
@@ -166,10 +167,12 @@
struct OutputFile {
std::string Filename;
std::string TempFilename;
+ Optional<llvm::sys::fs::TempFile> TempFile;
- OutputFile(std::string filename, std::string tempFilename)
- : Filename(std::move(filename)), TempFilename(std::move(tempFilename)) {
- }
+ OutputFile(std::string filename, std::string tempFilename,
+ Optional<llvm::sys::fs::TempFile> tempFile)
+ : Filename(std::move(filename)), TempFilename(std::move(tempFilename)),
+ TempFile(std::move(tempFile)) {}
};
/// The list of active output files.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
