Revert "Modify the localCache API to require an explicit commit on CachedFile… (llvm#115331)"

This reverts commit ce9e1d3.

The unittest added in this commit seems to be flaky causing random failure on buildbots:
 - https://lab.llvm.org/buildbot/#/builders/46/builds/13235
 - https://lab.llvm.org/buildbot/#/builders/46/builds/13232
 - https://lab.llvm.org/buildbot/#/builders/46/builds/13228
 - https://lab.llvm.org/buildbot/#/builders/46/builds/13224
 - https://lab.llvm.org/buildbot/#/builders/46/builds/13220
 - https://lab.llvm.org/buildbot/#/builders/46/builds/13210
 - https://lab.llvm.org/buildbot/#/builders/46/builds/13208
 - https://lab.llvm.org/buildbot/#/builders/46/builds/13207
 - https://lab.llvm.org/buildbot/#/builders/46/builds/13202
 - https://lab.llvm.org/buildbot/#/builders/46/builds/13196
and
 - https://lab.llvm.org/buildbot/#/builders/180/builds/14266
 - https://lab.llvm.org/buildbot/#/builders/180/builds/14254
 - https://lab.llvm.org/buildbot/#/builders/180/builds/14250
 - https://lab.llvm.org/buildbot/#/builders/180/builds/14245
 - https://lab.llvm.org/buildbot/#/builders/180/builds/14244
 - https://lab.llvm.org/buildbot/#/builders/180/builds/14226

Author: dyung

lldb/source/Core/DataFileCache.cpp

@@ -132,11 +132,6 @@ bool DataFileCache::SetCachedData(llvm::StringRef key,
       if (file_or_err) {
         llvm::CachedFileStream *cfs = file_or_err->get();
         cfs->OS->write((const char *)data.data(), data.size());
-        if (llvm::Error err = cfs->commit()) {
-          Log *log = GetLog(LLDBLog::Modules);
-          LLDB_LOG_ERROR(log, std::move(err),
-                         "failed to commit to the cache for key: {0}");
-        }
         return true;
       } else {
         Log *log = GetLog(LLDBLog::Modules);

llvm/include/llvm/Support/Caching.h

@@ -30,7 +30,6 @@ class CachedFileStream {
   CachedFileStream(std::unique_ptr<raw_pwrite_stream> OS,
                    std::string OSPath = "")
       : OS(std::move(OS)), ObjectPathName(OSPath) {}
-  virtual Error commit() { return Error::success(); }
   std::unique_ptr<raw_pwrite_stream> OS;
   std::string ObjectPathName;
   virtual ~CachedFileStream() = default;

llvm/lib/Debuginfod/Debuginfod.cpp

@@ -188,7 +188,6 @@ class StreamedHTTPResponseHandler : public HTTPResponseHandler {
 public:
   StreamedHTTPResponseHandler(CreateStreamFn CreateStream, HTTPClient &Client)
       : CreateStream(CreateStream), Client(Client) {}
-  Error commit();
   virtual ~StreamedHTTPResponseHandler() = default;
 
   Error handleBodyChunk(StringRef BodyChunk) override;
@@ -211,12 +210,6 @@ Error StreamedHTTPResponseHandler::handleBodyChunk(StringRef BodyChunk) {
   return Error::success();
 }
 
-Error StreamedHTTPResponseHandler::commit() {
-  if (FileStream)
-    return FileStream->commit();
-  return Error::success();
-}
-
 // An over-accepting simplification of the HTTP RFC 7230 spec.
 static bool isHeader(StringRef S) {
   StringRef Name;
@@ -305,8 +298,6 @@ Expected<std::string> getCachedOrDownloadArtifact(
       Error Err = Client.perform(Request, Handler);
       if (Err)
         return std::move(Err);
-      if (Err = Handler.commit())
-        return std::move(Err);
 
       unsigned Code = Client.responseCode();
       if (Code && Code != 200)

llvm/lib/LTO/LTOBackend.cpp

@@ -421,9 +421,6 @@ static void codegen(const Config &Conf, TargetMachine *TM,
 
   if (DwoOut)
     DwoOut->keep();
-
-  if (Error Err = Stream->commit())
-    report_fatal_error(std::move(Err));
 }
 
 static void splitCodeGen(const Config &C, TargetMachine *TM,

llvm/lib/Support/Caching.cpp

@@ -80,7 +80,6 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
       sys::fs::TempFile TempFile;
       std::string ModuleName;
       unsigned Task;
-      bool Committed = false;
 
       CacheStream(std::unique_ptr<raw_pwrite_stream> OS, AddBufferFn AddBuffer,
                   sys::fs::TempFile TempFile, std::string EntryPath,
@@ -89,10 +88,9 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
             AddBuffer(std::move(AddBuffer)), TempFile(std::move(TempFile)),
             ModuleName(ModuleName), Task(Task) {}
 
-      Error commit() override {
-        if (Committed)
-          return Error::success();
-        Committed = true;
+      ~CacheStream() {
+        // TODO: Manually commit rather than using non-trivial destructor,
+        // allowing to replace report_fatal_errors with a return Error.
 
         // Make sure the stream is closed before committing it.
         OS.reset();
@@ -102,12 +100,10 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
             MemoryBuffer::getOpenFile(
                 sys::fs::convertFDToNativeFile(TempFile.FD), ObjectPathName,
                 /*FileSize=*/-1, /*RequiresNullTerminator=*/false);
-        if (!MBOrErr) {
-          std::error_code EC = MBOrErr.getError();
-          return createStringError(EC, Twine("Failed to open new cache file ") +
-                                           TempFile.TmpName + ": " +
-                                           EC.message() + "\n");
-        }
+        if (!MBOrErr)
+          report_fatal_error(Twine("Failed to open new cache file ") +
+                             TempFile.TmpName + ": " +
+                             MBOrErr.getError().message() + "\n");
 
         // On POSIX systems, this will atomically replace the destination if
         // it already exists. We try to emulate this on Windows, but this may
@@ -122,10 +118,7 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
         E = handleErrors(std::move(E), [&](const ECError &E) -> Error {
           std::error_code EC = E.convertToErrorCode();
           if (EC != errc::permission_denied)
-            return createStringError(
-                EC, Twine("Failed to rename temporary file ") +
-                        TempFile.TmpName + " to " + ObjectPathName + ": " +
-                        EC.message() + "\n");
+            return errorCodeToError(EC);
 
           auto MBCopy = MemoryBuffer::getMemBufferCopy((*MBOrErr)->getBuffer(),
                                                        ObjectPathName);
@@ -138,22 +131,11 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
         });
 
         if (E)
-          return E;
+          report_fatal_error(Twine("Failed to rename temporary file ") +
+                             TempFile.TmpName + " to " + ObjectPathName + ": " +
+                             toString(std::move(E)) + "\n");
 
         AddBuffer(Task, ModuleName, std::move(*MBOrErr));
-        return Error::success();
-      }
-
-      ~CacheStream() {
-        // In Debug builds, try to track down places where commit() was not
-        // called before destruction.
-        assert(Committed);
-        // In Release builds, fall back to the previous behaviour of committing
-        // during destruction and reporting errors with report_fatal_error.
-        if (Committed)
-          return;
-        if (Error Err = commit())
-          report_fatal_error(Twine(toString(std::move(Err))));
       }
     };
 

llvm/tools/gold/gold-plugin.cpp

@@ -1101,9 +1101,7 @@ static std::vector<std::pair<SmallString<128>, bool>> runLTO() {
 
   auto AddBuffer = [&](size_t Task, const Twine &moduleName,
                        std::unique_ptr<MemoryBuffer> MB) {
-    auto Stream = *AddStream(Task, ModuleName);
-    Stream->OS << MB->getBuffer();
-    check(Stream->commit(), "Failed to commit cache");
+    *AddStream(Task, moduleName)->OS << MB->getBuffer();
   };
 
   FileCache Cache;

llvm/tools/llvm-lto2/llvm-lto2.cpp

@@ -447,9 +447,7 @@ static int run(int argc, char **argv) {
 
   auto AddBuffer = [&](size_t Task, const Twine &ModuleName,
                        std::unique_ptr<MemoryBuffer> MB) {
-    auto Stream = AddStream(Task, ModuleName);
-    *Stream->OS << MB->getBuffer();
-    check(Stream->commit(), "Failed to commit cache");
+    *AddStream(Task, ModuleName)->OS << MB->getBuffer();
   };
 
   FileCache Cache;