diff --git a/lldb/source/Core/DataFileCache.cpp b/lldb/source/Core/DataFileCache.cpp index ef0e07a8b0342..9109269711231 100644 --- a/lldb/source/Core/DataFileCache.cpp +++ b/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); diff --git a/llvm/include/llvm/Support/Caching.h b/llvm/include/llvm/Support/Caching.h index cf45145619d95..9a82921e6ffc7 100644 --- a/llvm/include/llvm/Support/Caching.h +++ b/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 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 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. diff --git a/llvm/lib/CGData/CodeGenData.cpp b/llvm/lib/CGData/CodeGenData.cpp index 02de528c4d007..7b9c584d64867 100644 --- a/llvm/lib/CGData/CodeGenData.cpp +++ b/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 loadModuleForTwoRounds(BitcodeModule &OrigModule, diff --git a/llvm/lib/Debuginfod/Debuginfod.cpp b/llvm/lib/Debuginfod/Debuginfod.cpp index 4c785117ae8ef..e570982e357e3 100644 --- a/llvm/lib/Debuginfod/Debuginfod.cpp +++ b/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 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) diff --git a/llvm/lib/LTO/LTOBackend.cpp b/llvm/lib/LTO/LTOBackend.cpp index b38252a3272e8..888e4172b4799 100644 --- a/llvm/lib/LTO/LTOBackend.cpp +++ b/llvm/lib/LTO/LTOBackend.cpp @@ -455,6 +455,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, diff --git a/llvm/lib/Support/Caching.cpp b/llvm/lib/Support/Caching.cpp index 66e540efaca97..40a5c44771b65 100644 --- a/llvm/lib/Support/Caching.cpp +++ b/llvm/lib/Support/Caching.cpp @@ -88,9 +88,10 @@ Expected 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 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 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 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(); } }; diff --git a/llvm/tools/gold/gold-plugin.cpp b/llvm/tools/gold/gold-plugin.cpp index ae965e6f486aa..b42bfbed3b873 100644 --- a/llvm/tools/gold/gold-plugin.cpp +++ b/llvm/tools/gold/gold-plugin.cpp @@ -1119,7 +1119,9 @@ static std::vector, bool>> runLTO() { auto AddBuffer = [&](size_t Task, const Twine &moduleName, std::unique_ptr 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; diff --git a/llvm/tools/llvm-lto2/llvm-lto2.cpp b/llvm/tools/llvm-lto2/llvm-lto2.cpp index 3ad2a3ecb6752..d78a3e0420543 100644 --- a/llvm/tools/llvm-lto2/llvm-lto2.cpp +++ b/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 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; diff --git a/llvm/unittests/Support/CMakeLists.txt b/llvm/unittests/Support/CMakeLists.txt index 6de8165826442..c78e6a370beac 100644 --- a/llvm/unittests/Support/CMakeLists.txt +++ b/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 diff --git a/llvm/unittests/Support/Caching.cpp b/llvm/unittests/Support/Caching.cpp new file mode 100644 index 0000000000000..c46d47fd6eecb --- /dev/null +++ b/llvm/unittests/Support/Caching.cpp @@ -0,0 +1,163 @@ +//===- 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> TempDir; + sys::path::system_temp_directory(true, TempDir); + SmallString<256> CacheDir; + sys::path::append(CacheDir, TempDir, "llvm_test_cache"); + + sys::fs::remove_directories(CacheDir.str()); + + std::unique_ptr CachedBuffer; + auto AddBuffer = [&CachedBuffer](unsigned Task, const Twine &ModuleName, + std::unique_ptr 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> TempDir; + sys::path::system_temp_directory(true, TempDir); + SmallString<256> CacheDir; + sys::path::append(CacheDir, TempDir, "llvm_test_cache"); + + sys::fs::remove_directories(CacheDir.str()); + + std::unique_ptr CachedBuffer; + auto AddBuffer = [&CachedBuffer](unsigned Task, const Twine &ModuleName, + std::unique_ptr 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> TempDir; + sys::path::system_temp_directory(true, TempDir); + SmallString<256> CacheDir; + sys::path::append(CacheDir, TempDir, "llvm_test_cache"); + + sys::fs::remove_directories(CacheDir.str()); + + std::unique_ptr CachedBuffer; + auto AddBuffer = [&CachedBuffer](unsigned Task, const Twine &ModuleName, + std::unique_ptr 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())); +}