Modify the localCache API to require an explicit commit on CachedFile…

lldb/source/Core/DataFileCache.cpp

@@ -132,6 +132,11 @@ 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,15 +24,32 @@ 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.
+/// that can be done by deriving from this class and overriding the destructor
+/// or the commit() method.
 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() = default;
+  virtual ~CachedFileStream() {
+    if (!Committed)
+      report_fatal_error("CachedFileStream was not committed.\n");
+  }
 };
 
 /// This type defines the callback to add a file that is generated on the fly.

llvm/lib/CGData/CodeGenData.cpp

@@ -233,6 +233,9 @@ 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,6 +188,11 @@ 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;
@@ -210,6 +215,12 @@ 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;
@@ -298,6 +309,8 @@ 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,6 +460,9 @@ 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,9 +88,10 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
             AddBuffer(std::move(AddBuffer)), TempFile(std::move(TempFile)),
             ModuleName(ModuleName), Task(Task) {}
 
-      ~CacheStream() {
-        // TODO: Manually commit rather than using non-trivial destructor,
-        // allowing to replace report_fatal_errors with a return Error.
+      Error commit() override {
+        Error E = CachedFileStream::commit();
+        if (E)
+          return E;
 
         // Make sure the stream is closed before committing it.
         OS.reset();
@@ -100,10 +101,12 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
             MemoryBuffer::getOpenFile(
                 sys::fs::convertFDToNativeFile(TempFile.FD), ObjectPathName,
                 /*FileSize=*/-1, /*RequiresNullTerminator=*/false);
-        if (!MBOrErr)
-          report_fatal_error(Twine("Failed to open new cache file ") +
-                             TempFile.TmpName + ": " +
-                             MBOrErr.getError().message() + "\n");
+        if (!MBOrErr) {
+          std::error_code EC = MBOrErr.getError();
+          return createStringError(EC, Twine("Failed to open new cache file ") +
+                                           TempFile.TmpName + ": " +
+                                           EC.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
@@ -114,11 +117,14 @@ 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.
-        Error E = TempFile.keep(ObjectPathName);
+        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 errorCodeToError(EC);
+            return createStringError(
+                EC, Twine("Failed to rename temporary file ") +
+                        TempFile.TmpName + " to " + ObjectPathName + ": " +
+                        EC.message() + "\n");
 
           auto MBCopy = MemoryBuffer::getMemBufferCopy((*MBOrErr)->getBuffer(),
                                                        ObjectPathName);
@@ -131,11 +137,10 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
         });
 
         if (E)
-          report_fatal_error(Twine("Failed to rename temporary file ") +
-                             TempFile.TmpName + " to " + ObjectPathName + ": " +
-                             toString(std::move(E)) + "\n");
+          return E;
 
         AddBuffer(Task, ModuleName, std::move(*MBOrErr));
+        return Error::success();
       }
     };
 

llvm/tools/gold/gold-plugin.cpp

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

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

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

llvm/unittests/Support/CMakeLists.txt

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

llvm/unittests/Support/Caching.cpp

@@ -0,0 +1,157 @@
+//===- Caching.cpp --------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/Caching.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Testing/Support/Error.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+#define ASSERT_NO_ERROR(x)                                                     \
+  if (std::error_code ASSERT_NO_ERROR_ec = x) {                                \
+    SmallString<128> MessageStorage;                                           \
+    raw_svector_ostream Message(MessageStorage);                               \
+    Message << #x ": did not return errc::success.\n"                          \
+            << "error number: " << ASSERT_NO_ERROR_ec.value() << "\n"          \
+            << "error message: " << ASSERT_NO_ERROR_ec.message() << "\n";      \
+    GTEST_FATAL_FAILURE_(MessageStorage.c_str());                              \
+  } else {                                                                     \
+  }
+
+char data[] = "some data";
+
+TEST(Caching, Normal) {
+  SmallString<256> CacheDir;
+  sys::fs::createUniquePath("llvm_test_cache-%%%%%%", CacheDir, true);
+
+  sys::fs::remove_directories(CacheDir.str());
+
+  std::unique_ptr<MemoryBuffer> CachedBuffer;
+  auto AddBuffer = [&CachedBuffer](unsigned Task, const Twine &ModuleName,
+                                   std::unique_ptr<MemoryBuffer> M) {
+    CachedBuffer = std::move(M);
+  };
+  auto CacheOrErr =
+      localCache("LLVMTestCache", "LLVMTest", CacheDir, AddBuffer);
+  ASSERT_TRUE(bool(CacheOrErr));
+
+  FileCache &Cache = *CacheOrErr;
+
+  {
+    auto AddStreamOrErr = Cache(1, "foo", "");
+    ASSERT_TRUE(bool(AddStreamOrErr));
+
+    AddStreamFn &AddStream = *AddStreamOrErr;
+    ASSERT_TRUE(AddStream);
+
+    auto FileOrErr = AddStream(1, "");
+    ASSERT_TRUE(bool(FileOrErr));
+
+    CachedFileStream *CFS = FileOrErr->get();
+    (*CFS->OS).write(data, sizeof(data));
+    ASSERT_THAT_ERROR(CFS->commit(), Succeeded());
+  }
+
+  {
+    auto AddStreamOrErr = Cache(1, "foo", "");
+    ASSERT_TRUE(bool(AddStreamOrErr));
+
+    AddStreamFn &AddStream = *AddStreamOrErr;
+    ASSERT_FALSE(AddStream);
+
+    ASSERT_TRUE(CachedBuffer->getBufferSize() == sizeof(data));
+    StringRef readData = CachedBuffer->getBuffer();
+    ASSERT_TRUE(memcmp(data, readData.data(), sizeof(data)) == 0);
+  }
+
+  ASSERT_NO_ERROR(sys::fs::remove_directories(CacheDir.str()));
+}
+
+TEST(Caching, WriteAfterCommit) {
+  SmallString<256> CacheDir;
+  sys::fs::createUniquePath("llvm_test_cache-%%%%%%", CacheDir, true);
+
+  sys::fs::remove_directories(CacheDir.str());
+
+  std::unique_ptr<MemoryBuffer> CachedBuffer;
+  auto AddBuffer = [&CachedBuffer](unsigned Task, const Twine &ModuleName,
+                                   std::unique_ptr<MemoryBuffer> M) {
+    CachedBuffer = std::move(M);
+  };
+  auto CacheOrErr =
+      localCache("LLVMTestCache", "LLVMTest", CacheDir, AddBuffer);
+  ASSERT_TRUE(bool(CacheOrErr));
+
+  FileCache &Cache = *CacheOrErr;
+
+  auto AddStreamOrErr = Cache(1, "foo", "");
+  ASSERT_TRUE(bool(AddStreamOrErr));
+
+  AddStreamFn &AddStream = *AddStreamOrErr;
+  ASSERT_TRUE(AddStream);
+
+  auto FileOrErr = AddStream(1, "");
+  ASSERT_TRUE(bool(FileOrErr));
+
+  CachedFileStream *CFS = FileOrErr->get();
+  (*CFS->OS).write(data, sizeof(data));
+  ASSERT_THAT_ERROR(CFS->commit(), Succeeded());
+
+  EXPECT_DEATH(
+      { (*CFS->OS).write(data, sizeof(data)); }, "")
+      << "Write after commit did not cause abort";
+
+  ASSERT_NO_ERROR(sys::fs::remove_directories(CacheDir.str()));
+}
+
+TEST(Caching, NoCommit) {
+  SmallString<256> CacheDir;
+  sys::fs::createUniquePath("llvm_test_cache-%%%%%%", CacheDir, true);
+
+  sys::fs::remove_directories(CacheDir.str());
+
+  std::unique_ptr<MemoryBuffer> CachedBuffer;
+  auto AddBuffer = [&CachedBuffer](unsigned Task, const Twine &ModuleName,
+                                   std::unique_ptr<MemoryBuffer> M) {
+    CachedBuffer = std::move(M);
+  };
+  auto CacheOrErr =
+      localCache("LLVMTestCache", "LLVMTest", CacheDir, AddBuffer);
+  ASSERT_TRUE(bool(CacheOrErr));
+
+  FileCache &Cache = *CacheOrErr;
+
+  auto AddStreamOrErr = Cache(1, "foo", "");
+  ASSERT_TRUE(bool(AddStreamOrErr));
+
+  AddStreamFn &AddStream = *AddStreamOrErr;
+  ASSERT_TRUE(AddStream);
+
+  auto FileOrErr = AddStream(1, "");
+  ASSERT_TRUE(bool(FileOrErr));
+
+  CachedFileStream *CFS = FileOrErr->get();
+  (*CFS->OS).write(data, sizeof(data));
+  ASSERT_THAT_ERROR(CFS->commit(), Succeeded());
+
+  EXPECT_DEATH(
+      {
+        auto FileOrErr = AddStream(1, "");
+        ASSERT_TRUE(bool(FileOrErr));
+
+        CachedFileStream *CFS = FileOrErr->get();
+        (*CFS->OS).write(data, sizeof(data));
+      },
+      "")
+      << "destruction without commit did not cause error";
+
+  ASSERT_NO_ERROR(sys::fs::remove_directories(CacheDir.str()));
+}