Revert "Modify the localCache API to require an explicit commit on CachedFile… (#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

@@ -24,32 +24,15 @@ class MemoryBuffer;
 /// This class wraps an output stream for a file. Most clients should just be
 /// able to return an instance of this base class from the stream callback, but
 /// if a client needs to perform some action after the stream is written to,
-/// that can be done by deriving from this class and overriding the destructor
-/// or the commit() method.
+/// that can be done by deriving from this class and overriding the destructor.
 class CachedFileStream {
 public:
   CachedFileStream(std::unique_ptr<raw_pwrite_stream> OS,
                    std::string OSPath = "")
       : OS(std::move(OS)), ObjectPathName(OSPath) {}
-
-  /// Must be called exactly once after the writes to OS have been completed
-  /// but before the CachedFileStream object is destroyed.
-  virtual Error commit() {
-    if (Committed)
-      return createStringError(make_error_code(std::errc::invalid_argument),
-                               Twine("CacheStream already committed."));
-    Committed = true;
-
-    return Error::success();
-  }
-
-  bool Committed = false;
   std::unique_ptr<raw_pwrite_stream> OS;
   std::string ObjectPathName;
-  virtual ~CachedFileStream() {
-    if (!Committed)
-      report_fatal_error("CachedFileStream was not committed.\n");
-  }
+  virtual ~CachedFileStream() = default;
 };
 
 /// This type defines the callback to add a file that is generated on the fly.

llvm/lib/CGData/CodeGenData.cpp

@@ -233,9 +233,6 @@ void saveModuleForTwoRounds(const Module &TheModule, unsigned Task,
 
   WriteBitcodeToFile(TheModule, *Stream->OS,
                      /*ShouldPreserveUseListOrder=*/true);
-
-  if (Error Err = Stream->commit())
-    report_fatal_error(std::move(Err));
 }
 
 std::unique_ptr<Module> loadModuleForTwoRounds(BitcodeModule &OrigModule,

llvm/lib/Debuginfod/Debuginfod.cpp

@@ -188,11 +188,6 @@ class StreamedHTTPResponseHandler : public HTTPResponseHandler {
 public:
   StreamedHTTPResponseHandler(CreateStreamFn CreateStream, HTTPClient &Client)
       : CreateStream(CreateStream), Client(Client) {}
-
-  /// Must be called exactly once after the writes have been completed
-  /// but before the StreamedHTTPResponseHandler object is destroyed.
-  Error commit();
-
   virtual ~StreamedHTTPResponseHandler() = default;
 
   Error handleBodyChunk(StringRef BodyChunk) override;
@@ -215,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;
@@ -309,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

@@ -460,9 +460,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

@@ -88,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 {
-        Error E = CachedFileStream::commit();
-        if (E)
-          return E;
+      ~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();
@@ -101,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
@@ -117,14 +114,11 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
         // AddBuffer a copy of the bytes we wrote in that case. We do this
         // instead of just using the existing file, because the pruner might
         // delete the file before we get a chance to use it.
-        E = TempFile.keep(ObjectPathName);
+        Error E = TempFile.keep(ObjectPathName);
         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);
@@ -137,10 +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();
       }
     };
 

llvm/tools/gold/gold-plugin.cpp

@@ -1119,9 +1119,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

@@ -448,9 +448,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;

llvm/unittests/Support/CMakeLists.txt

@@ -18,7 +18,6 @@ add_llvm_unittest(SupportTests
   BranchProbabilityTest.cpp
   CachePruningTest.cpp
   CrashRecoveryTest.cpp
-  Caching.cpp
   Casting.cpp
   CheckedArithmeticTest.cpp
   Chrono.cpp