[Lldb-commits] [lldb] 1d763f3 - Revert "Modify the localCache API to require an explicit commit on CachedFile… (#115331)"
Douglas Yung via lldb-commits
lldb-commits at lists.llvm.org
Sat Mar 8 15:59:18 PST 2025
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()));
-}
More information about the lldb-commits
mailing list