Handle some fs::remove failures

[jfb]

Summary

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

[vsapsai]

clang/lib/Frontend/CompilerInstance.cpp

648–650

Does the same logic as in ASTUnit.cpp apply here? I.e. if we failed to rename a file and emitted a message about it, should we also have a message about the failure to remove a file?

1447–1449

Why are you interrupting the loop when cannot remove a file? Don't know which option is the right one, just want to know your reasons.

clang/lib/Frontend/PrecompiledPreamble.cpp

111–118

Clangd uses PrecompiledPreamble but not TemporaryFiles as far as I can tell. report_fatal_error can be really disruptive for clangd, so it would be good to get an opinion from somebody working on it if this change would impact them.

clang/lib/Serialization/GlobalModuleIndex.cpp

935–938

Don't have a strong opinion but it looks odd that here in createStringError you are using string concatenation and a few lines lower %s.

llvm/include/llvm/Support/FileUtilities.h

55–61

For this change opinion of LLDB developers can be useful as it changes existing FileRemover behavior.

llvm/lib/Support/GraphWriter.cpp

102

Please add an extra space before \"

llvm/lib/Support/LockFileManager.cpp

58

Do you plan to keep using report_fatal_error in LockFileManager? It should help with discovering problems with modules but making it a fatal error forever seems a little bit scary.

214

Do you need to start the string with a space to separate a filename and "also"?

298–300

Here too, extra space before \"

[jfb]

• Return llvm::Error from ASTUnit::Save
• Update per comments.

[jkorous]

clang/lib/Frontend/PrecompiledPreamble.cpp

111–118

Since client code in general might have different error handling/reporting strategy and can't do much about failure in destructor here, I'd consider just some kind of logging or assert here.

[jfb]

clang/lib/Frontend/CompilerInstance.cpp

648–650

I've updated ASTUnit to be a bit better, and removed the logging here.

1447–1449

The loops already bail when EC is set, and here I figure if we can't remove the base file we shouldn't try to remove its corresponding timestamp.

clang/lib/Serialization/GlobalModuleIndex.cpp

935–938

Yeah this part of createStringError bugs me... It only takes const char* for %s, and here I have a Twine. I changed it to %s.

llvm/lib/Support/LockFileManager.cpp

58

For some of the other destructors I was thinking that failing to remove a file could be non-fatal, but for lock files we can't really do much if they stay around. I think those are probably better as fatal than any other one. That, or return Expected, but combining with Optional<std::pair<std::string, int>> is... ew...

[vsapsai]

clang/lib/Frontend/ASTUnit.cpp

2318–2319

Will Out.error() still work after Out.clear_error()?

clang/lib/Frontend/CompilerInstance.cpp

1447–1449

I was thinking about using continue instead of break. After some thinking I believe continue is better because this way we'll try to remove all stale module files that we can. Otherwise one bad file can prevent the module cache pruning from working.

And I agree with your logic about not removing the timestamp when cannot remove the base file.

llvm/include/llvm/Support/Error.h

1173 ↗

(On Diff #212899)

I might be missing something but should this function be static in the header?

llvm/lib/Support/LockFileManager.cpp

58

OK, I agree that a failure to remove a lock file is bad enough to justify report_fatal_error. We have some recovery code around locking (see unsafeRemoveLockFile) but we can try this approach until we have evidence it causes problems.

[abrachet]

I haven't updated all ignored instances of fs::remove, I therefore can't mark it LLVM_NODISCARD for now.

I think I remember reading that casting a [[nodiscard]] functions return to void was broken on some compilers, ie still warns. You cast fs::remove to void a lot here which is a great way to show intent (I wish we mentioned this in the style guide) but if I'm not just making this up, it might not be worth the trouble to give fs::remove the nodiscard attribute. Maybe something to look into if you ever do this.

[jfb]

• Return llvm::Error from ASTUnit::Save
• Update per comments.
• Address Volodymyr's comments.

clang/lib/Frontend/CompilerInstance.cpp

1447–1449

continue will hit File != FileEnd && !EC, so it's equivalent to break. Your thinking makes sense, but I think we'd want to do it in a separate patch.

clang/lib/Frontend/PrecompiledPreamble.cpp

111–118

I changed it to dbgs().

[bruno]

Thanks for working on this JF!

clang/lib/Frontend/PrecompiledPreamble.cpp

111–118

This probably means we won't see the message in a non-debug build, right?

[jfb]

clang/lib/Frontend/PrecompiledPreamble.cpp

111–118

Yeah, so it'll be diagnosable but won't affect release builds (which I understand to be the concern expressed w.r.t. clangd).

[vsapsai]

I have no other comments but for the fatal error in FileRemover I'd like to loop in Jonas as he was touching module cache in LLDB fairly recently.

clang/lib/Frontend/CompilerInstance.cpp

1447–1449

OK, I see now. Thanks for explanation.

[JDevlieghere]

Thanks for looping me in, @vsapsai!

llvm/include/llvm/Support/FileUtilities.h

55–61

Aborting isn't acceptable for LLDB. Can we turn this into an assert instead?

[jfb]

• Don't report_fatal_error in code that can be called from LLDB

[jfb]

Updated.

[vsapsai]

Looks good to me. If in some situations abort on error turns out to be too aggressive, we can change it later.

[bruno]

LGTM with one minor change.

clang/lib/Frontend/PrecompiledPreamble.cpp

38

pch-preamble is more accurate here.

clang/include/clang/Basic/DiagnosticFrontendKinds.td

 //==--- DiagnosticFrontendKinds.td - frontend diagnostics -----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 class BackendInfo : CatBackend, ShowInSystemHeader;
 
 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">;
 def err_fe_error_backend : Error<"error in backend: %0">, DefaultFatal;
 
 def err_fe_inline_asm : Error<"%0">, CatInlineAsm;
 def warn_fe_inline_asm : Warning<"%0">, CatInlineAsm, InGroup<BackendInlineAsm>;
 def note_fe_inline_asm : Note<"%0">, CatInlineAsm;

clang/include/clang/Frontend/ASTUnit.h

 #include "clang/AST/ASTContext.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/FileSystemOptions.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/LangOptions.h"
 #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"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallVector.h"
 #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>
 #include <memory>
 #include <string>
 #include <utility>
 #include <vector>
 
void CodeComplete(StringRef File, unsigned Line, unsigned Column,
                     std::shared_ptr<PCHContainerOperations> PCHContainerOps,
                     DiagnosticsEngine &Diag, LangOptions &LangOpts,
                     SourceManager &SourceMgr, FileManager &FileMgr,
                     SmallVectorImpl<StoredDiagnostic> &StoredDiagnostics,
                     SmallVectorImpl<const llvm::MemoryBuffer *> &OwnedBuffers);
 
   /// 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.
   ///
   /// \returns True if an error occurred, false otherwise.
   bool serialize(raw_ostream &OS);
 };
 
 } // namespace clang
 
 #endif // LLVM_CLANG_FRONTEND_ASTUNIT_H

clang/include/clang/Frontend/PrecompiledPreamble.h

 
 #ifndef LLVM_CLANG_FRONTEND_PRECOMPILED_PREAMBLE_H
 #define LLVM_CLANG_FRONTEND_PRECOMPILED_PREAMBLE_H
 
 #include "clang/Lex/Lexer.h"
 #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>
 #include <system_error>
 #include <type_traits>
 
 namespace llvm {
 class MemoryBuffer;
public:
 
     TempPCHFile(const TempPCHFile &) = delete;
     ~TempPCHFile();
 
     /// A path where temporary file is stored.
     llvm::StringRef getFilePath() const;
 
   private:
-    void RemoveFileIfPresent();
+    llvm::Error RemoveFileIfPresent();
 
   private:
     llvm::Optional<std::string> FilePath;
   };
 
   class InMemoryPreamble {
   public:
     std::string Data;

clang/lib/Frontend/ASTUnit.cpp

void ASTUnit::CodeComplete(
   if (Act->BeginSourceFile(*Clang.get(), Clang->getFrontendOpts().Inputs[0])) {
     if (llvm::Error Err = Act->Execute()) {
       consumeError(std::move(Err)); // FIXME this drops errors on the floor.
     }
     Act->EndSourceFile();
   }
 }
 
-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.
   SmallString<128> TempPath;
   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?
   llvm::raw_fd_ostream Out(fd, /*shouldClose=*/true);
 
   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,
                           SmallVectorImpl<char> &Buffer,
                           Sema &S,
                           bool hasErrors,
                           raw_ostream &OS) {
   Writer.WriteAST(S, std::string(), nullptr, "", hasErrors);

clang/lib/Frontend/CompilerInstance.cpp

 void CompilerInstance::addOutputFile(OutputFile &&OutFile) {
   OutputFiles.push_back(std::move(OutFile));
 }
 
 void CompilerInstance::clearOutputFiles(bool EraseFiles) {
   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);
 
         // If '-working-directory' was passed, the output filename should be
         // relative to that.
         FileMgr->FixupRelativePath(NewOutFile);
         if (std::error_code ec =
                 llvm::sys::fs::rename(OF.TempFilename, NewOutFile)) {
           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();
 }
 
 std::unique_ptr<raw_pwrite_stream>
 CompilerInstance::createDefaultOutputFile(bool Binary, StringRef InFile,
                                           StringRef Extension) {
for (llvm::sys::fs::directory_iterator File(Dir->path(), EC), FileEnd;
       // If the file has been used recently enough, leave it there.
       time_t FileAccessTime = StatBuf.st_atime;
       if (CurrentTime - FileAccessTime <=
               time_t(HSOpts.ModuleCachePruneAfter)) {
         continue;
       }
 
       // 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;
   }
 }
 
 void CompilerInstance::createModuleManager() {
   if (!ModuleManager) {
     if (!hasASTContext())
       createASTContext();
 

clang/lib/Frontend/DependencyFile.cpp

for (unsigned i = 0, e = NativePath.size(); i != e; ++i) {
     } else if (NativePath[i] == '$') // $ is escaped by $$.
       OS << '$';
     OS << NativePath[i];
   }
 }
 
 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;
   }
 
   std::error_code EC;
   llvm::raw_fd_ostream OS(OutputFile, EC, llvm::sys::fs::OF_Text);
   if (EC) {
     Diags.Report(diag::err_fe_error_opening) << OutputFile << EC.message();
     return;

clang/lib/Frontend/PrecompiledPreamble.cpp

 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Serialization/ASTWriter.h"
 #include "llvm/ADT/StringExtras.h"
 #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"
 #include "llvm/Support/Process.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include <limits>
 #include <utility>
 
+#define DEBUG_TYPE "pch"
+
 using namespace clang;
 
 namespace {
 
 StringRef getInMemoryPreamblePath() {
 #if defined(LLVM_ON_UNIX)
   return "/__clang_tmp/___clang_inmemory_preamble___";
 #elif defined(_WIN32)

 
 public:
   ~TemporaryFiles();
 
   /// Adds \p File to a set of tracked files.
   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;
   llvm::StringSet<> Files;
 };
 
 TemporaryFiles &TemporaryFiles::getInstance() {
   static TemporaryFiles Instance;
   return Instance;
 }
 
 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) {
   llvm::MutexGuard Guard(Mutex);
   auto IsInserted = Files.insert(File).second;
   (void)IsInserted;
   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 {
 public:
   PrecompilePreambleAction(std::string *InMemStorage,
                            PreambleCallbacks &Callbacks)
       : InMemStorage(InMemStorage), Callbacks(Callbacks) {}
 

 
 PrecompiledPreamble::TempPCHFile::TempPCHFile(TempPCHFile &&Other) {
   FilePath = std::move(Other.FilePath);
   Other.FilePath = None;
 }
 
 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 {
   assert(FilePath && "TempPCHFile doesn't have a FilePath. Had it been moved?");
   return *FilePath;
 }
 
 PrecompiledPreamble::PCHStorage::PCHStorage(TempPCHFile File)

clang/lib/Rewrite/Rewriter.cpp

~AtomicallyMovedFile() {
     // Close (will also flush) theFileStream.
     FileStream->close();
     if (std::error_code ec = llvm::sys::fs::rename(TempFilename, Filename)) {
       AllWritten = false;
       Diagnostics.Report(clang::diag::err_unable_to_rename_temp)
         << 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);
     }
   }
 
   bool ok() { return (bool)FileStream; }
   raw_ostream &getStream() { return *FileStream; }
 
 private:
   DiagnosticsEngine &Diagnostics;

clang/lib/Serialization/GlobalModuleIndex.cpp

GlobalModuleIndex::writeIndex(FileManager &FileMgr,
 
   // Write the index.
   Out.write(OutputBuffer.data(), OutputBuffer.size());
   Out.close();
   if (Out.has_error())
     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());
   }
 
   return llvm::Error::success();
 }
 
 namespace {

clang/tools/driver/cc1gen_reproducer_main.cpp

int cc1gen_reproducer_main(ArrayRef<const char *> Argv, const char *Argv0,
   // Emit the information about the reproduce files to stdout.
   int Result = 1;
   if (Report) {
     printReproducerInformation(llvm::outs(), InvocationInfo, *Report);
     Result = 0;
   }
 
   // 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;
 }

clang/tools/libclang/CIndex.cpp

 
 static CXSaveError clang_saveTranslationUnit_Impl(CXTranslationUnit TU,
                                                   const char *FileName,
                                                   unsigned options) {
   CIndexer *CXXIdx = TU->CIdx;
   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,
                               unsigned options) {
   LOG_FUNC_SECTION {
     *Log << TU << ' ' << FileName;
   }
 

clang/unittests/CrossTU/CrossTranslationUnitTest.cpp

ASSERT_FALSE(llvm::sys::fs::createTemporaryFile("input", "cpp", SourceFD,
                                                     SourceFileName));
     llvm::ToolOutputFile SourceFile(SourceFileName, SourceFD);
     SourceFile.os() << SourceText;
     SourceFile.os().flush();
     EXPECT_TRUE(llvm::sys::fs::exists(SourceFileName));
 
     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.
     llvm::Expected<const FunctionDecl *> NewFDorError = handleExpected(
         CTU.getCrossTUDefinition(FD, "", IndexFileName, false),
         []() { return nullptr; }, [](IndexError &) {});
 
     if (NewFDorError) {

clang/unittests/Frontend/ASTUnitTest.cpp

if (!AST)
     FAIL() << "failed to create ASTUnit";
 
   EXPECT_FALSE(AST->getASTContext().getPrintingPolicy().UseVoidForZeroParams);
 
   llvm::SmallString<256> ASTFileName;
   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));
 
   std::unique_ptr<ASTUnit> AU = ASTUnit::LoadFromASTFile(
       ASTFileName.str(), PCHContainerOps->getRawReader(),
       ASTUnit::LoadEverything, Diags, FileSystemOptions(),
       /*UseDebugInfo=*/false);
 

llvm/include/llvm/Support/FileUtilities.h

 //===- llvm/Support/FileUtilities.h - File System Utilities -----*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file defines a family of utility functions which are useful for doing
 // various things with files.
 //
 //===----------------------------------------------------------------------===//
 
 #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
   /// the files match, 1 if they are different, and 2 if there is a file error.
   /// This function allows you to specify an absolute and relative FP error that
   /// is allowed to exist.  If you specify a string to fill in for the error
   /// option, it will set the string to an error message if an error occurs, or
   /// if the files are different.
public:
 
     explicit FileRemover(const Twine& filename, bool deleteIt = true)
       : DeleteIt(deleteIt) {
       filename.toVector(Filename);
     }
 
     ~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());
+        }
       }
     }
 
     /// setFile - Give ownership of the file to the FileRemover so it will
     /// be removed when the object is destroyed.  If the FileRemover already
     /// 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();
       filename.toVector(Filename);
       DeleteIt = deleteIt;
     }
 
     /// releaseFile - Take ownership of the file away from the FileRemover so it
     /// will not be removed when the object is destroyed.
     void releaseFile() { DeleteIt = false; }
   };
 } // End llvm namespace
 
 #endif

llvm/lib/Support/GraphWriter.cpp

 static bool ExecGraphViewer(StringRef ExecPath, std::vector<StringRef> &args,
                             StringRef Filename, bool wait,
                             std::string &ErrMsg) {
   if (wait) {
     if (sys::ExecuteAndWait(ExecPath, args, None, {}, 0, 0, &ErrMsg)) {
       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);
     errs() << "Remember to erase graph file: " << Filename << "\n";
   }
   return false;
 }
 

llvm/lib/Support/LockFileManager.cpp

 
 using namespace llvm;
 
 /// Attempt to read the lock file with the given name, if it exists.
 ///
 /// \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();
 
   StringRef Hostname;
   StringRef PIDStr;
   std::tie(Hostname, PIDStr) = getToken(MB.getBuffer(), " ");
   PIDStr = PIDStr.substr(PIDStr.find_first_not_of(" "));
   int PID;
   if (!PIDStr.getAsInteger(10, PID)) {
     auto Owner = std::make_pair(std::string(Hostname), PID);
     if (processStillExecuting(Owner.first, Owner.second))
       return Owner;
   }
 
   // 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;
 }
 
 static std::error_code getHostID(SmallVectorImpl<char> &HostID) {
   HostID.clear();
 
 #if USE_OSX_GETHOSTUUID
   // On OS X, use the more stable hardware UUID instead of hostname.
public:
   }
 
   ~RemoveUniqueLockFileOnSignal() {
     if (!RemoveImmediately) {
       // Leave the signal handler enabled. It will be removed when the lock is
       // 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);
   }
 
   void lockAcquired() { RemoveImmediately = false; }
 };
 
 } // end anonymous namespace
 

 #endif
     Out.close();
 
     if (Out.has_error()) {
       // We failed to write out PID, so report the error, remove the
       // 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;
     }
   }
 
   // Clean up the unique file on signal, which also releases the lock if it is
   // held since the .lock symlink will point to a nonexistent file.
   RemoveUniqueLockFileOnSignal RemoveUniqueFile(UniqueLockFileName);
 
if (EC != errc::file_exists) {
       OSS << LockFileName.str() << " to " << UniqueLockFileName.str();
       setError(EC, OSS.str());
       return;
     }
 
     // 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;
     }
 
     if (!sys::fs::exists(LockFileName)) {
       // The previous owner released the lock file before we could read it.
       // Try to get ownership again.
       continue;
     }
std::string LockFileManager::getErrorMessage() const {
   return "";
 }
 
 LockFileManager::~LockFileManager() {
   if (getState() != LFS_Owned)
     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);
 }
 
 LockFileManager::WaitForUnlockResult LockFileManager::waitForUnlock() {
   if (getState() != LFS_Shared)
     return Res_Success;

llvm/lib/Support/Path.cpp

#ifdef _WIN32
   // If we can't rename, discard the temporary file.
   if (RenameEC)
     setDeleteDisposition(H, true);
 #else
   std::error_code RenameEC = fs::rename(TmpName, Name);
   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
 
   if (!RenameEC)
     TmpName = "";
 
   if (close(FD) == -1) {

llvm/lib/Support/ToolOutputFile.cpp

ToolOutputFile::CleanupInstaller::CleanupInstaller(StringRef Filename)
   // Arrange for the file to be deleted if the process is killed.
   if (Filename != "-")
     sys::RemoveFileOnSignal(Filename);
 }
 
 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.
   if (Filename != "-")
     sys::DontRemoveFileOnSignal(Filename);
 }
 
 ToolOutputFile::ToolOutputFile(StringRef Filename, std::error_code &EC,