[llvm] c3f815b - Modify the localCache API to require an explicit commit on CachedFile… (#136121)
via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 22 01:45:20 PDT 2025
Author: anjenner
Date: 2025-04-22T09:45:15+01:00
New Revision: c3f815ba82defc84244a9688fd2578da513340fb
URL: https://github.com/llvm/llvm-project/commit/c3f815ba82defc84244a9688fd2578da513340fb
DIFF: https://github.com/llvm/llvm-project/commit/c3f815ba82defc84244a9688fd2578da513340fb.diff
LOG: Modify the localCache API to require an explicit commit on CachedFile… (#136121)
…Stream.
CachedFileStream has previously performed the commit step in its
destructor, but this means its only recourse for error handling is
report_fatal_error. Modify this to add an explicit commit() method, and
call this in the appropriate places with appropriate error handling for
the location.
Currently the destructor of CacheStream gives an assert failure in Debug
builds if commit() was not called. This will help track down any
remaining uses of the API that assume the old destructior behaviour. In
Release builds we fall back to the previous behaviour and call
report_fatal_error if the commit fails.
This is version 2 of this PR, superseding reverted PR
https://github.com/llvm/llvm-project/pull/115331 . I have incorporated a
change to the testcase to make it more reliable on Windows, as well as
two follow-up changes
(https://github.com/llvm/llvm-project/commit/df79000896101acc9b8d7435e59f767b36c00ac8
and
https://github.com/llvm/llvm-project/commit/b0baa1d8bd68a2ce2f7c5f2b62333e410e9122a1)
that were also reverted when 115331 was reverted.
---------
Co-authored-by: Augie Fackler <augie at google.com>
Co-authored-by: Vitaly Buka <vitalybuka at google.com>
Added:
llvm/unittests/Support/Caching.cpp
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:
################################################################################
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<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.
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<Module> loadModuleForTwoRounds(BitcodeModule &OrigModule,
diff --git a/llvm/lib/Debuginfod/Debuginfod.cpp b/llvm/lib/Debuginfod/Debuginfod.cpp
index 4c785117ae8ef..db316a1b37b3b 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<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 1c764a0188eda..dd9197efa7718 100644
--- a/llvm/lib/LTO/LTOBackend.cpp
+++ b/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,
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<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();
}
};
diff --git a/llvm/tools/gold/gold-plugin.cpp b/llvm/tools/gold/gold-plugin.cpp
index ae965e6f486aa..256933d3f53f9 100644
--- a/llvm/tools/gold/gold-plugin.cpp
+++ b/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;
diff --git a/llvm/tools/llvm-lto2/llvm-lto2.cpp b/llvm/tools/llvm-lto2/llvm-lto2.cpp
index 33626c1fd9433..01cbbaae32a5c 100644
--- a/llvm/tools/llvm-lto2/llvm-lto2.cpp
+++ b/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;
diff --git a/llvm/unittests/Support/CMakeLists.txt b/llvm/unittests/Support/CMakeLists.txt
index 4a12a928af119..14c1915f4e2ee 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..581675deac066
--- /dev/null
+++ b/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()));
+}
More information about the llvm-commits
mailing list