jfb created this revision.
jfb added reviewers: Bigcheese, bruno, arphaman, vsapsai.
Herald added subscribers: llvm-commits, cfe-commits, dexonsmith, jkorous,
hiraditya.
Herald added projects: clang, LLVM.
We have data showing that some modules builds fail in rare cases. We're
therefore interested in handling sources of failure better, especially when it
comes to modules. This patch takes us a small step closer to this by handling
the return code of fs::remove in code that seems like it should. I haven't
updated all ignored instances of fs::remove, I therefore can't mark it
LLVM_NODISCARD for now.
This previous patch helps propagate errors: https://reviews.llvm.org/D63518
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D65545
Files:
clang/include/clang/Basic/DiagnosticFrontendKinds.td
clang/include/clang/Frontend/PrecompiledPreamble.h
clang/lib/Frontend/ASTUnit.cpp
clang/lib/Frontend/CompilerInstance.cpp
clang/lib/Frontend/DependencyFile.cpp
clang/lib/Frontend/PrecompiledPreamble.cpp
clang/lib/Rewrite/Rewriter.cpp
clang/lib/Serialization/GlobalModuleIndex.cpp
clang/tools/driver/cc1gen_reproducer_main.cpp
llvm/include/llvm/Support/Error.h
llvm/include/llvm/Support/FileUtilities.h
llvm/lib/Support/GraphWriter.cpp
llvm/lib/Support/LockFileManager.cpp
llvm/lib/Support/Path.cpp
llvm/lib/Support/ToolOutputFile.cpp
Index: llvm/lib/Support/ToolOutputFile.cpp
===================================================================
--- llvm/lib/Support/ToolOutputFile.cpp
+++ llvm/lib/Support/ToolOutputFile.cpp
@@ -25,7 +25,8 @@
ToolOutputFile::CleanupInstaller::~CleanupInstaller() {
// Delete the file if the client hasn't told us not to.
if (!Keep && Filename != "-")
- sys::fs::remove(Filename);
+ if (std::error_code EC = sys::fs::remove(Filename))
+ report_fatal_error("Failed removing file \"" + Filename + "\": " + EC.message());
// Ok, the file is successfully written and closed, or deleted. There's no
// further need to clean it up on signals.
Index: llvm/lib/Support/Path.cpp
===================================================================
--- llvm/lib/Support/Path.cpp
+++ llvm/lib/Support/Path.cpp
@@ -1182,9 +1182,10 @@
if (RenameEC) {
// If we can't rename, try to copy to work around cross-device link issues.
RenameEC = sys::fs::copy_file(TmpName, Name);
- // If we can't rename or copy, discard the temporary file.
+ // If we can't rename or copy, discard the temporary file, ignoring any
+ // further failure.
if (RenameEC)
- remove(TmpName);
+ (void)remove(TmpName);
}
sys::DontRemoveFileOnSignal(TmpName);
#endif
Index: llvm/lib/Support/LockFileManager.cpp
===================================================================
--- llvm/lib/Support/LockFileManager.cpp
+++ llvm/lib/Support/LockFileManager.cpp
@@ -47,14 +47,15 @@
/// \param LockFileName The name of the lock file to read.
///
/// \returns The process ID of the process that owns this lock file
-Optional<std::pair<std::string, int> >
+Optional<std::pair<std::string, int>>
LockFileManager::readLockFile(StringRef LockFileName) {
// Read the owning host and PID out of the lock file. If it appears that the
// owning process is dead, the lock file is invalid.
ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr =
MemoryBuffer::getFile(LockFileName);
if (!MBOrErr) {
- sys::fs::remove(LockFileName);
+ if (std::error_code EC = sys::fs::remove(LockFileName))
+ report_fatal_error("Unable to remove invalid lock file \"" + LockFileName + "\": " + EC.message());
return None;
}
MemoryBuffer &MB = *MBOrErr.get();
@@ -71,7 +72,8 @@
}
// Delete the lock file. It's invalid anyway.
- sys::fs::remove(LockFileName);
+ if (std::error_code EC = sys::fs::remove(LockFileName))
+ report_fatal_error("Unable to remove invalid lock file \"" + LockFileName + "\": " + EC.message());
return None;
}
@@ -144,7 +146,8 @@
// released.
return;
}
- sys::fs::remove(Filename);
+ if (std::error_code EC = sys::fs::remove(Filename))
+ report_fatal_error("Unable to remove unique lock file on signal \"" + Filename + "\": " + EC.message());
sys::DontRemoveFileOnSignal(Filename);
}
@@ -204,8 +207,9 @@
// unique lock file, and fail.
std::string S("failed to write to ");
S.append(UniqueLockFileName.str());
+ if (std::error_code EC = sys::fs::remove(UniqueLockFileName))
+ S.append("also failed to remove the lockfile");
setError(Out.error(), S);
- sys::fs::remove(UniqueLockFileName);
return;
}
}
@@ -234,8 +238,12 @@
// Someone else managed to create the lock file first. Read the process ID
// from the lock file.
if ((Owner = readLockFile(LockFileName))) {
- // Wipe out our unique lock file (it's useless now)
- sys::fs::remove(UniqueLockFileName);
+ // Wipe out our unique lock file (it's useless now).
+ if ((EC = sys::fs::remove(UniqueLockFileName))) {
+ std::string S("failed to remove useless lockfile ");
+ S.append(UniqueLockFileName.str());
+ setError(EC, S);
+ }
return;
}
@@ -283,8 +291,10 @@
return;
// Since we own the lock, remove the lock file and our own unique lock file.
- sys::fs::remove(LockFileName);
- sys::fs::remove(UniqueLockFileName);
+ if (std::error_code EC = sys::fs::remove(LockFileName))
+ report_fatal_error("failed removing file\"" + LockFileName + "\": " + EC.message());
+ if (std::error_code EC = sys::fs::remove(UniqueLockFileName))
+ report_fatal_error("failed removing file\"" + UniqueLockFileName + "\": " + EC.message());
// The unique file is now gone, so remove it from the signal handler. This
// matches a sys::RemoveFileOnSignal() in LockFileManager().
sys::DontRemoveFileOnSignal(UniqueLockFileName);
Index: llvm/lib/Support/GraphWriter.cpp
===================================================================
--- llvm/lib/Support/GraphWriter.cpp
+++ llvm/lib/Support/GraphWriter.cpp
@@ -98,7 +98,8 @@
errs() << "Error: " << ErrMsg << "\n";
return true;
}
- sys::fs::remove(Filename);
+ if (std::error_code EC = sys::fs::remove(Filename))
+ errs() << "Failed removing file\"" << Filename << "\": " << EC.message() << '\n';
errs() << " done. \n";
} else {
sys::ExecuteNoWait(ExecPath, args, None, {}, 0, &ErrMsg);
Index: llvm/include/llvm/Support/FileUtilities.h
===================================================================
--- llvm/include/llvm/Support/FileUtilities.h
+++ llvm/include/llvm/Support/FileUtilities.h
@@ -49,8 +49,8 @@
~FileRemover() {
if (DeleteIt) {
- // Ignore problems deleting the file.
- sys::fs::remove(Filename);
+ if (std::error_code EC = sys::fs::remove(Filename))
+ report_fatal_error("failed removing file \"" + Filename + "\": " + EC.message());
}
}
@@ -59,8 +59,8 @@
/// had ownership of a file, remove it first.
void setFile(const Twine& filename, bool deleteIt = true) {
if (DeleteIt) {
- // Ignore problems deleting the file.
- sys::fs::remove(Filename);
+ if (std::error_code EC = sys::fs::remove(Filename))
+ report_fatal_error("failed removing file \"" + Filename + "\": " + EC.message());
}
Filename.clear();
Index: llvm/include/llvm/Support/Error.h
===================================================================
--- llvm/include/llvm/Support/Error.h
+++ llvm/include/llvm/Support/Error.h
@@ -1170,6 +1170,10 @@
Error createStringError(std::error_code EC, char const *Msg);
+static inline Error createStringError(std::error_code EC, const Twine &S) {
+ return createStringError(EC, S.str().c_str());
+}
+
template <typename... Ts>
inline Error createStringError(std::errc EC, char const *Fmt,
const Ts &... Vals) {
Index: clang/tools/driver/cc1gen_reproducer_main.cpp
===================================================================
--- clang/tools/driver/cc1gen_reproducer_main.cpp
+++ clang/tools/driver/cc1gen_reproducer_main.cpp
@@ -190,6 +190,11 @@
}
// Remove the input file.
- llvm::sys::fs::remove(Input);
+ if (std::error_code EC = llvm::sys::fs::remove(Input)) {
+ llvm::errs() << "error: failed to remove file " << Input << ": "
+ << EC.message() << "\n";
+ return 1;
+ }
+
return Result;
}
Index: clang/lib/Serialization/GlobalModuleIndex.cpp
===================================================================
--- clang/lib/Serialization/GlobalModuleIndex.cpp
+++ clang/lib/Serialization/GlobalModuleIndex.cpp
@@ -932,12 +932,13 @@
return llvm::createStringError(Out.error(), "failed writing to stream");
// Remove the old index file. It isn't relevant any more.
- llvm::sys::fs::remove(IndexPath);
+ if (std::error_code EC = llvm::sys::fs::remove(IndexPath))
+ return llvm::createStringError(EC, "failed removing \"" + IndexPath + "\"");
// Rename the newly-written index file to the proper name.
if (std::error_code Err = llvm::sys::fs::rename(IndexTmpPath, IndexPath)) {
// Remove the file on failure, don't check whether removal succeeded.
- llvm::sys::fs::remove(IndexTmpPath);
+ (void)llvm::sys::fs::remove(IndexTmpPath);
return llvm::createStringError(Err, "failed renaming file \"%s\" to \"%s\"",
IndexTmpPath.c_str(), IndexPath.c_str());
}
Index: clang/lib/Rewrite/Rewriter.cpp
===================================================================
--- clang/lib/Rewrite/Rewriter.cpp
+++ clang/lib/Rewrite/Rewriter.cpp
@@ -434,7 +434,7 @@
<< TempFilename << Filename << ec.message();
// If the remove fails, there's not a lot we can do - this is already an
// error.
- llvm::sys::fs::remove(TempFilename);
+ (void)llvm::sys::fs::remove(TempFilename);
}
}
Index: clang/lib/Frontend/PrecompiledPreamble.cpp
===================================================================
--- clang/lib/Frontend/PrecompiledPreamble.cpp
+++ clang/lib/Frontend/PrecompiledPreamble.cpp
@@ -92,7 +92,7 @@
void addFile(StringRef File);
/// Remove \p File from disk and from the set of tracked files.
- void removeFile(StringRef File);
+ llvm::Error removeFile(StringRef File);
private:
llvm::sys::SmartMutex<false> Mutex;
@@ -107,7 +107,8 @@
TemporaryFiles::~TemporaryFiles() {
llvm::MutexGuard Guard(Mutex);
for (const auto &File : Files)
- llvm::sys::fs::remove(File.getKey());
+ if (std::error_code EC = llvm::sys::fs::remove(File.getKey()))
+ llvm::report_fatal_error("failed removing file \"" + File.getKey() + "\": " + EC.message());
}
void TemporaryFiles::addFile(StringRef File) {
@@ -117,12 +118,14 @@
assert(IsInserted && "File has already been added");
}
-void TemporaryFiles::removeFile(StringRef File) {
+llvm::Error TemporaryFiles::removeFile(StringRef File) {
llvm::MutexGuard Guard(Mutex);
auto WasPresent = Files.erase(File);
(void)WasPresent;
assert(WasPresent && "File was not tracked");
- llvm::sys::fs::remove(File);
+ if (std::error_code EC = llvm::sys::fs::remove(File))
+ return llvm::createStringError(EC, "failed removing \"" + File + "\"");
+ return llvm::Error::success();
}
class PrecompilePreambleAction : public ASTFrontendAction {
@@ -572,20 +575,26 @@
PrecompiledPreamble::TempPCHFile &PrecompiledPreamble::TempPCHFile::
operator=(TempPCHFile &&Other) {
- RemoveFileIfPresent();
+ if (llvm::Error E = RemoveFileIfPresent())
+ llvm::report_fatal_error("failed assigning temporary PCH file: " + toString(std::move(E)));
FilePath = std::move(Other.FilePath);
Other.FilePath = None;
return *this;
}
-PrecompiledPreamble::TempPCHFile::~TempPCHFile() { RemoveFileIfPresent(); }
+PrecompiledPreamble::TempPCHFile::~TempPCHFile() {
+ if (llvm::Error E = RemoveFileIfPresent())
+ llvm::report_fatal_error("failed destroying temporary PCH file: " + toString(std::move(E)));
+}
-void PrecompiledPreamble::TempPCHFile::RemoveFileIfPresent() {
+llvm::Error PrecompiledPreamble::TempPCHFile::RemoveFileIfPresent() {
if (FilePath) {
- TemporaryFiles::getInstance().removeFile(*FilePath);
+ if (llvm::Error E = TemporaryFiles::getInstance().removeFile(*FilePath))
+ return E;
FilePath = None;
}
+ return llvm::Error::success();
}
llvm::StringRef PrecompiledPreamble::TempPCHFile::getFilePath() const {
Index: clang/lib/Frontend/DependencyFile.cpp
===================================================================
--- clang/lib/Frontend/DependencyFile.cpp
+++ clang/lib/Frontend/DependencyFile.cpp
@@ -311,7 +311,8 @@
void DependencyFileGenerator::outputDependencyFile(DiagnosticsEngine &Diags) {
if (SeenMissingHeader) {
- llvm::sys::fs::remove(OutputFile);
+ if (std::error_code EC = llvm::sys::fs::remove(OutputFile))
+ Diags.Report(diag::err_fe_error_removing) << OutputFile << EC.message();
return;
}
Index: clang/lib/Frontend/CompilerInstance.cpp
===================================================================
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -630,7 +630,9 @@
for (OutputFile &OF : OutputFiles) {
if (!OF.TempFilename.empty()) {
if (EraseFiles) {
- llvm::sys::fs::remove(OF.TempFilename);
+ if (std::error_code EC = llvm::sys::fs::remove(OF.TempFilename))
+ getDiagnostics().Report(diag::err_fe_error_removing)
+ << OF.TempFilename << EC.message();
} else {
SmallString<128> NewOutFile(OF.Filename);
@@ -642,16 +644,22 @@
getDiagnostics().Report(diag::err_unable_to_rename_temp)
<< OF.TempFilename << OF.Filename << ec.message();
- llvm::sys::fs::remove(OF.TempFilename);
+ if (std::error_code EC = llvm::sys::fs::remove(OF.TempFilename))
+ getDiagnostics().Report(diag::err_fe_error_removing)
+ << OF.TempFilename << EC.message();
}
}
} else if (!OF.Filename.empty() && EraseFiles)
- llvm::sys::fs::remove(OF.Filename);
+ if (std::error_code EC = llvm::sys::fs::remove(OF.Filename))
+ getDiagnostics().Report(diag::err_fe_error_removing)
+ << OF.Filename << EC.message();
}
OutputFiles.clear();
if (DeleteBuiltModules) {
for (auto &Module : BuiltModules)
- llvm::sys::fs::remove(Module.second);
+ if (std::error_code EC = llvm::sys::fs::remove(Module.second))
+ getDiagnostics().Report(diag::err_fe_error_removing)
+ << Module.second << EC.message();
BuiltModules.clear();
}
NonSeekStream.reset();
@@ -1433,18 +1441,20 @@
}
// Remove the file.
- llvm::sys::fs::remove(File->path());
+ if ((EC = llvm::sys::fs::remove(File->path())))
+ break;
- // Remove the timestamp file.
- std::string TimpestampFilename = File->path() + ".timestamp";
- llvm::sys::fs::remove(TimpestampFilename);
+ std::string TimestampFilename = File->path() + ".timestamp";
+ if ((EC = llvm::sys::fs::remove(TimestampFilename)))
+ break;
}
// If we removed all of the files in the directory, remove the directory
// itself.
if (llvm::sys::fs::directory_iterator(Dir->path(), EC) ==
llvm::sys::fs::directory_iterator() && !EC)
- llvm::sys::fs::remove(Dir->path());
+ if ((EC = llvm::sys::fs::remove(Dir->path())))
+ break;
}
}
Index: clang/lib/Frontend/ASTUnit.cpp
===================================================================
--- clang/lib/Frontend/ASTUnit.cpp
+++ clang/lib/Frontend/ASTUnit.cpp
@@ -2314,7 +2314,8 @@
}
if (llvm::sys::fs::rename(TempPath, File)) {
- llvm::sys::fs::remove(TempPath);
+ // Ignore failure to remove if we already fail to rename.
+ (void)llvm::sys::fs::remove(TempPath);
return true;
}
Index: clang/include/clang/Frontend/PrecompiledPreamble.h
===================================================================
--- clang/include/clang/Frontend/PrecompiledPreamble.h
+++ clang/include/clang/Frontend/PrecompiledPreamble.h
@@ -17,6 +17,7 @@
#include "clang/Lex/Preprocessor.h"
#include "llvm/ADT/IntrusiveRefCntPtr.h"
#include "llvm/Support/AlignOf.h"
+#include "llvm/Support/Error.h"
#include "llvm/Support/MD5.h"
#include <cstddef>
#include <memory>
@@ -156,7 +157,7 @@
llvm::StringRef getFilePath() const;
private:
- void RemoveFileIfPresent();
+ llvm::Error RemoveFileIfPresent();
private:
llvm::Optional<std::string> FilePath;
Index: clang/include/clang/Basic/DiagnosticFrontendKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticFrontendKinds.td
+++ clang/include/clang/Basic/DiagnosticFrontendKinds.td
@@ -10,6 +10,7 @@
let Component = "Frontend" in {
+def err_fe_error_removing : Error<"error removing '%0': %1">;
def err_fe_error_opening : Error<"error opening '%0': %1">;
def err_fe_error_reading : Error<"error reading '%0'">;
def err_fe_error_reading_stdin : Error<"error reading stdin: %0">;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
