diff --git a/clang/include/clang/Basic/DiagnosticFrontendKinds.td b/clang/include/clang/Basic/DiagnosticFrontendKinds.td --- a/clang/include/clang/Basic/DiagnosticFrontendKinds.td +++ b/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">; diff --git a/clang/include/clang/Frontend/ASTUnit.h b/clang/include/clang/Frontend/ASTUnit.h --- a/clang/include/clang/Frontend/ASTUnit.h +++ b/clang/include/clang/Frontend/ASTUnit.h @@ -22,12 +22,12 @@ #include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" #include "clang/Basic/TargetOptions.h" +#include "clang/Frontend/PrecompiledPreamble.h" #include "clang/Lex/HeaderSearchOptions.h" #include "clang/Lex/ModuleLoader.h" #include "clang/Lex/PreprocessingRecord.h" #include "clang/Sema/CodeCompleteConsumer.h" #include "clang/Serialization/ASTBitCodes.h" -#include "clang/Frontend/PrecompiledPreamble.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/DenseMap.h" #include "llvm/ADT/IntrusiveRefCntPtr.h" @@ -38,6 +38,7 @@ #include "llvm/ADT/StringMap.h" #include "llvm/ADT/StringRef.h" #include "llvm/ADT/iterator_range.h" +#include "llvm/Support/Error.h" #include #include #include @@ -888,9 +889,8 @@ /// Save this translation unit to a file with the given name. /// - /// \returns true if there was a file error or false if the save was - /// successful. - bool Save(StringRef File); + /// \returns Errors arising from file operations. + llvm::Error Save(StringRef File); /// Serialize this translation unit with the given output stream. /// diff --git a/clang/include/clang/Frontend/PrecompiledPreamble.h b/clang/include/clang/Frontend/PrecompiledPreamble.h --- a/clang/include/clang/Frontend/PrecompiledPreamble.h +++ b/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 #include @@ -156,7 +157,7 @@ llvm::StringRef getFilePath() const; private: - void RemoveFileIfPresent(); + llvm::Error RemoveFileIfPresent(); private: llvm::Optional FilePath; diff --git a/clang/lib/Frontend/ASTUnit.cpp b/clang/lib/Frontend/ASTUnit.cpp --- a/clang/lib/Frontend/ASTUnit.cpp +++ b/clang/lib/Frontend/ASTUnit.cpp @@ -2290,9 +2290,10 @@ } } -bool ASTUnit::Save(StringRef File) { +llvm::Error ASTUnit::Save(StringRef File) { if (HadModuleLoaderFatalFailure) - return true; + return llvm::createStringError(std::errc::io_error, + "module loader had a fatal failure"); // Write to a temporary file and later rename it to the actual file, to avoid // possible race conditions. @@ -2300,8 +2301,10 @@ TempPath = File; TempPath += "-%%%%%%%%"; int fd; - if (llvm::sys::fs::createUniqueFile(TempPath, fd, TempPath)) - return true; + if (std::error_code EC = + llvm::sys::fs::createUniqueFile(TempPath, fd, TempPath)) + return llvm::createStringError(EC, "creating unique file \"%s\" for AST", + TempPath.c_str()); // FIXME: Can we somehow regenerate the stat cache here, or do we need to // unconditionally create a stat cache when we parse the file? @@ -2310,16 +2313,20 @@ serialize(Out); Out.close(); if (Out.has_error()) { + llvm::Error E = + llvm::createStringError(Out.error(), "AST serialization failed"); Out.clear_error(); - return true; + return E; } - if (llvm::sys::fs::rename(TempPath, File)) { - llvm::sys::fs::remove(TempPath); - return true; + if (std::error_code EC = llvm::sys::fs::rename(TempPath, File)) { + // Ignore failure to remove if we already fail to rename. + (void)llvm::sys::fs::remove(TempPath); + return llvm::createStringError(EC, "renaming \"%s\" to \"%s\" failed", + TempPath.c_str(), File.str().c_str()); } - return false; + return llvm::Error::success(); } static bool serializeUnit(ASTWriter &Writer, diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp --- a/clang/lib/Frontend/CompilerInstance.cpp +++ b/clang/lib/Frontend/CompilerInstance.cpp @@ -631,7 +631,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); @@ -643,16 +645,21 @@ getDiagnostics().Report(diag::err_unable_to_rename_temp) << OF.TempFilename << OF.Filename << ec.message(); - llvm::sys::fs::remove(OF.TempFilename); + // Ignore errors removing a file when renaming already failed. + (void)llvm::sys::fs::remove(OF.TempFilename); } } } 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(); @@ -1437,18 +1444,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; } } diff --git a/clang/lib/Frontend/DependencyFile.cpp b/clang/lib/Frontend/DependencyFile.cpp --- a/clang/lib/Frontend/DependencyFile.cpp +++ b/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; } diff --git a/clang/lib/Frontend/PrecompiledPreamble.cpp b/clang/lib/Frontend/PrecompiledPreamble.cpp --- a/clang/lib/Frontend/PrecompiledPreamble.cpp +++ b/clang/lib/Frontend/PrecompiledPreamble.cpp @@ -26,6 +26,7 @@ #include "llvm/ADT/StringSet.h" #include "llvm/Config/llvm-config.h" #include "llvm/Support/CrashRecoveryContext.h" +#include "llvm/Support/Debug.h" #include "llvm/Support/FileSystem.h" #include "llvm/Support/Mutex.h" #include "llvm/Support/MutexGuard.h" @@ -34,6 +35,8 @@ #include #include +#define DEBUG_TYPE "pch" + using namespace clang; namespace { @@ -93,7 +96,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 Mutex; @@ -108,7 +111,10 @@ 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_DEBUG(llvm::dbgs() << "failed removing file \"" << File.getKey() + << "\": " << EC.message()); + } } void TemporaryFiles::addFile(StringRef File) { @@ -118,12 +124,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 { @@ -575,20 +583,28 @@ 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 { diff --git a/clang/lib/Rewrite/Rewriter.cpp b/clang/lib/Rewrite/Rewriter.cpp --- a/clang/lib/Rewrite/Rewriter.cpp +++ b/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); } } diff --git a/clang/lib/Serialization/GlobalModuleIndex.cpp b/clang/lib/Serialization/GlobalModuleIndex.cpp --- a/clang/lib/Serialization/GlobalModuleIndex.cpp +++ b/clang/lib/Serialization/GlobalModuleIndex.cpp @@ -932,12 +932,14 @@ 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 \"%s\"", + IndexPath.c_str()); // 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()); } diff --git a/clang/tools/driver/cc1gen_reproducer_main.cpp b/clang/tools/driver/cc1gen_reproducer_main.cpp --- a/clang/tools/driver/cc1gen_reproducer_main.cpp +++ b/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; } diff --git a/clang/tools/libclang/CIndex.cpp b/clang/tools/libclang/CIndex.cpp --- a/clang/tools/libclang/CIndex.cpp +++ b/clang/tools/libclang/CIndex.cpp @@ -3978,8 +3978,10 @@ if (CXXIdx->isOptEnabled(CXGlobalOpt_ThreadBackgroundPriorityForIndexing)) setThreadBackgroundPriority(); - bool hadError = cxtu::getASTUnit(TU)->Save(FileName); - return hadError ? CXSaveError_Unknown : CXSaveError_None; + llvm::Error E = cxtu::getASTUnit(TU)->Save(FileName); + CXSaveError Res = E ? CXSaveError_Unknown : CXSaveError_None; + consumeError(std::move(E)); + return Res; } int clang_saveTranslationUnit(CXTranslationUnit TU, const char *FileName, diff --git a/clang/unittests/CrossTU/CrossTranslationUnitTest.cpp b/clang/unittests/CrossTU/CrossTranslationUnitTest.cpp --- a/clang/unittests/CrossTU/CrossTranslationUnitTest.cpp +++ b/clang/unittests/CrossTU/CrossTranslationUnitTest.cpp @@ -72,7 +72,8 @@ std::unique_ptr ASTWithDefinition = tooling::buildASTFromCode(SourceText, SourceFileName); - ASTWithDefinition->Save(ASTFileName.str()); + llvm::Error E = ASTWithDefinition->Save(ASTFileName.str()); + EXPECT_FALSE(E); EXPECT_TRUE(llvm::sys::fs::exists(ASTFileName)); // Load the definition from the AST file. diff --git a/clang/unittests/Frontend/ASTUnitTest.cpp b/clang/unittests/Frontend/ASTUnitTest.cpp --- a/clang/unittests/Frontend/ASTUnitTest.cpp +++ b/clang/unittests/Frontend/ASTUnitTest.cpp @@ -82,7 +82,8 @@ ASSERT_FALSE( llvm::sys::fs::createTemporaryFile("ast-unit", "ast", FD, ASTFileName)); ToolOutputFile ast_file(ASTFileName, FD); - AST->Save(ASTFileName.str()); + llvm::Error E = AST->Save(ASTFileName.str()); + EXPECT_FALSE(E); EXPECT_TRUE(llvm::sys::fs::exists(ASTFileName)); diff --git a/llvm/include/llvm/Support/FileUtilities.h b/llvm/include/llvm/Support/FileUtilities.h --- a/llvm/include/llvm/Support/FileUtilities.h +++ b/llvm/include/llvm/Support/FileUtilities.h @@ -14,9 +14,12 @@ #ifndef LLVM_SUPPORT_FILEUTILITIES_H #define LLVM_SUPPORT_FILEUTILITIES_H +#include "llvm/Support/Debug.h" #include "llvm/Support/FileSystem.h" #include "llvm/Support/Path.h" +#define DEBUG_TYPE "file-utilities" + namespace llvm { /// DiffFilesWithTolerance - Compare the two files specified, returning 0 if @@ -49,8 +52,12 @@ ~FileRemover() { if (DeleteIt) { - // Ignore problems deleting the file. - sys::fs::remove(Filename); + std::error_code EC = sys::fs::remove(Filename); + if (!EC) { + LLVM_DEBUG(llvm::dbgs() << "failed removing file \"" << Filename + << "\": " + EC.message() << '\n'; + abort()); + } } } @@ -59,8 +66,12 @@ /// 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); + std::error_code EC = sys::fs::remove(Filename); + if (!EC) { + LLVM_DEBUG(llvm::dbgs() << "failed removing file \"" << Filename + << "\": " + EC.message() << '\n'; + abort()); + } } Filename.clear(); diff --git a/llvm/lib/Support/GraphWriter.cpp b/llvm/lib/Support/GraphWriter.cpp --- a/llvm/lib/Support/GraphWriter.cpp +++ b/llvm/lib/Support/GraphWriter.cpp @@ -98,7 +98,9 @@ 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); diff --git a/llvm/lib/Support/LockFileManager.cpp b/llvm/lib/Support/LockFileManager.cpp --- a/llvm/lib/Support/LockFileManager.cpp +++ b/llvm/lib/Support/LockFileManager.cpp @@ -47,14 +47,16 @@ /// \param LockFileName The name of the lock file to read. /// /// \returns The process ID of the process that owns this lock file -Optional > +Optional> 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> 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 +73,9 @@ } // 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 +148,9 @@ // 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 +210,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 +241,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 +294,12 @@ 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); diff --git a/llvm/lib/Support/Path.cpp b/llvm/lib/Support/Path.cpp --- a/llvm/lib/Support/Path.cpp +++ b/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 diff --git a/llvm/lib/Support/ToolOutputFile.cpp b/llvm/lib/Support/ToolOutputFile.cpp --- a/llvm/lib/Support/ToolOutputFile.cpp +++ b/llvm/lib/Support/ToolOutputFile.cpp @@ -25,7 +25,9 @@ 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.