[Lldb-commits] [lldb] [llvm] Modify the localCache API to require an explicit commit on CachedFile… (PR #115331)
via lldb-commits
lldb-commits at lists.llvm.org
Thu Nov 14 06:26:37 PST 2024
https://github.com/anjenner updated https://github.com/llvm/llvm-project/pull/115331
>From d16492dc12891cf15e41225abff890462c9f8460 Mon Sep 17 00:00:00 2001
From: Andrew Jenner <Andrew.Jenner at amd.com>
Date: Thu, 7 Nov 2024 10:47:42 -0500
Subject: [PATCH] Modify the localCache API to require an explicit commit on
CachedFileStream.
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.
---
lldb/source/Core/DataFileCache.cpp | 5 ++++
llvm/include/llvm/Support/Caching.h | 1 +
llvm/lib/Debuginfod/Debuginfod.cpp | 9 +++++++
llvm/lib/LTO/LTOBackend.cpp | 3 +++
llvm/lib/Support/Caching.cpp | 40 +++++++++++++++++++++--------
llvm/tools/gold/gold-plugin.cpp | 4 ++-
llvm/tools/llvm-lto2/llvm-lto2.cpp | 4 ++-
7 files changed, 53 insertions(+), 13 deletions(-)
diff --git a/lldb/source/Core/DataFileCache.cpp b/lldb/source/Core/DataFileCache.cpp
index ef0e07a8b03420d..910926971123174 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 cf45145619d95b8..120bcd9da02edea 100644
--- a/llvm/include/llvm/Support/Caching.h
+++ b/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;
diff --git a/llvm/lib/Debuginfod/Debuginfod.cpp b/llvm/lib/Debuginfod/Debuginfod.cpp
index 4c785117ae8ef77..6ff889d3a8cad2b 100644
--- a/llvm/lib/Debuginfod/Debuginfod.cpp
+++ b/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)
diff --git a/llvm/lib/LTO/LTOBackend.cpp b/llvm/lib/LTO/LTOBackend.cpp
index ad332d25d9c0824..862a6820569f7ad 100644
--- a/llvm/lib/LTO/LTOBackend.cpp
+++ b/llvm/lib/LTO/LTOBackend.cpp
@@ -432,6 +432,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 66e540efaca972d..2ecdf53701030d1 100644
--- a/llvm/lib/Support/Caching.cpp
+++ b/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))));
}
};
diff --git a/llvm/tools/gold/gold-plugin.cpp b/llvm/tools/gold/gold-plugin.cpp
index 6d0021c85f20fb7..d7c3e07b84efa82 100644
--- a/llvm/tools/gold/gold-plugin.cpp
+++ b/llvm/tools/gold/gold-plugin.cpp
@@ -1103,7 +1103,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;
diff --git a/llvm/tools/llvm-lto2/llvm-lto2.cpp b/llvm/tools/llvm-lto2/llvm-lto2.cpp
index d4f022ef021a44e..270510a01319342 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<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;
More information about the lldb-commits
mailing list