Author: Douglas Yung Date: 2025-03-08T23:54:57Z New Revision: 1d763f383380ec629ee6d5053fd41322efcbc6bd
URL: https://github.com/llvm/llvm-project/commit/1d763f383380ec629ee6d5053fd41322efcbc6bd DIFF: https://github.com/llvm/llvm-project/commit/1d763f383380ec629ee6d5053fd41322efcbc6bd.diff LOG: Revert "Modify the localCache API to require an explicit commit on CachedFile… (#115331)" This reverts commit ce9e1d3c15ed6290f1cb07b482939976fa8115cd. 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 Added: Modified: lldb/source/Core/DataFileCache.cpp llvm/include/llvm/Support/Caching.h llvm/lib/CGData/CodeGenData.cpp llvm/lib/Debuginfod/Debuginfod.cpp llvm/lib/LTO/LTOBackend.cpp llvm/lib/Support/Caching.cpp llvm/tools/gold/gold-plugin.cpp llvm/tools/llvm-lto2/llvm-lto2.cpp llvm/unittests/Support/CMakeLists.txt Removed: llvm/unittests/Support/Caching.cpp ################################################################################ diff --git a/lldb/source/Core/DataFileCache.cpp b/lldb/source/Core/DataFileCache.cpp index 9109269711231..ef0e07a8b0342 100644 --- a/lldb/source/Core/DataFileCache.cpp +++ b/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); diff --git a/llvm/include/llvm/Support/Caching.h b/llvm/include/llvm/Support/Caching.h index 9a82921e6ffc7..cf45145619d95 100644 --- a/llvm/include/llvm/Support/Caching.h +++ b/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. diff --git a/llvm/lib/CGData/CodeGenData.cpp b/llvm/lib/CGData/CodeGenData.cpp index 7b9c584d64867..02de528c4d007 100644 --- a/llvm/lib/CGData/CodeGenData.cpp +++ b/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, diff --git a/llvm/lib/Debuginfod/Debuginfod.cpp b/llvm/lib/Debuginfod/Debuginfod.cpp index e570982e357e3..4c785117ae8ef 100644 --- a/llvm/lib/Debuginfod/Debuginfod.cpp +++ b/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) diff --git a/llvm/lib/LTO/LTOBackend.cpp b/llvm/lib/LTO/LTOBackend.cpp index 2da207ace91ee..139c39abf8e6b 100644 --- a/llvm/lib/LTO/LTOBackend.cpp +++ b/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, diff --git a/llvm/lib/Support/Caching.cpp b/llvm/lib/Support/Caching.cpp index 40a5c44771b65..66e540efaca97 100644 --- a/llvm/lib/Support/Caching.cpp +++ b/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(); } }; diff --git a/llvm/tools/gold/gold-plugin.cpp b/llvm/tools/gold/gold-plugin.cpp index b42bfbed3b873..ae965e6f486aa 100644 --- a/llvm/tools/gold/gold-plugin.cpp +++ b/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; diff --git a/llvm/tools/llvm-lto2/llvm-lto2.cpp b/llvm/tools/llvm-lto2/llvm-lto2.cpp index d78a3e0420543..3ad2a3ecb6752 100644 --- a/llvm/tools/llvm-lto2/llvm-lto2.cpp +++ b/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; diff --git a/llvm/unittests/Support/CMakeLists.txt b/llvm/unittests/Support/CMakeLists.txt index c78e6a370beac..6de8165826442 100644 --- a/llvm/unittests/Support/CMakeLists.txt +++ b/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 diff --git a/llvm/unittests/Support/Caching.cpp b/llvm/unittests/Support/Caching.cpp deleted file mode 100644 index c46d47fd6eecb..0000000000000 --- a/llvm/unittests/Support/Caching.cpp +++ /dev/null @@ -1,163 +0,0 @@ -//===- 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<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> 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<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> 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<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())); -} _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
