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

Author: Salinas, David

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

@@ -30,6 +30,7 @@ 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,6 +188,7 @@ class StreamedHTTPResponseHandler : public HTTPResponseHandler {
 public:
   StreamedHTTPResponseHandler(CreateStreamFn CreateStream, HTTPClient &Client)
       : CreateStream(CreateStream), Client(Client) {}
+  Error commit();
   virtual ~StreamedHTTPResponseHandler() = default;
 
   Error handleBodyChunk(StringRef BodyChunk) override;
@@ -210,6 +211,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 +305,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

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

llvm/lib/Support/Caching.cpp

@@ -80,6 +80,7 @@ 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,
@@ -88,9 +89,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 {
+        if (Committed)
+          return Error::success();
+        Committed = true;
 
         // Make sure the stream is closed before committing it.
         OS.reset();
@@ -100,10 +102,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
@@ -118,7 +122,10 @@ 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 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 +138,22 @@ 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();
+      }
+
+      ~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

@@ -1119,7 +1119,9 @@ static std::vector<std::pair<SmallString<128>, bool>> runLTO() {
 
   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

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