jfb updated this revision to Diff 213497.
jfb marked 8 inline comments as done.
jfb added a comment.
- Return llvm::Error from ASTUnit::Save
- Update per comments.
- Address Volodymyr's comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D65545/new/
https://reviews.llvm.org/D65545
Files:
clang/include/clang/Basic/DiagnosticFrontendKinds.td
clang/include/clang/Frontend/ASTUnit.h
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
clang/tools/libclang/CIndex.cpp
clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
clang/unittests/Frontend/ASTUnitTest.cpp
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,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.
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,16 @@
/// \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 +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);
Index: llvm/lib/Support/GraphWriter.cpp
===================================================================
--- llvm/lib/Support/GraphWriter.cpp
+++ 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);
Index: llvm/include/llvm/Support/FileUtilities.h
===================================================================
--- llvm/include/llvm/Support/FileUtilities.h
+++ llvm/include/llvm/Support/FileUtilities.h
@@ -49,8 +49,9 @@
~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 +60,9 @@
/// 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: clang/unittests/Frontend/ASTUnitTest.cpp
===================================================================
--- clang/unittests/Frontend/ASTUnitTest.cpp
+++ 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));
Index: clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
===================================================================
--- clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
+++ clang/unittests/CrossTU/CrossTranslationUnitTest.cpp
@@ -72,7 +72,8 @@
std::unique_ptr<ASTUnit> 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.
Index: clang/tools/libclang/CIndex.cpp
===================================================================
--- clang/tools/libclang/CIndex.cpp
+++ 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,
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,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());
}
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
@@ -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 <limits>
#include <utility>
+#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<false> 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 {
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
@@ -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;
}
}
Index: clang/lib/Frontend/ASTUnit.cpp
===================================================================
--- clang/lib/Frontend/ASTUnit.cpp
+++ 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,19 @@
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,
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/Frontend/ASTUnit.h
===================================================================
--- clang/include/clang/Frontend/ASTUnit.h
+++ 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 <cassert>
#include <cstddef>
#include <cstdint>
@@ -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.
///
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
