[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
Tue Nov 19 06:24:04 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 1/2] 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 ef0e07a8b03420..91092697112317 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 cf45145619d95b..120bcd9da02ede 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 4c785117ae8ef7..6ff889d3a8cad2 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 ad332d25d9c082..862a6820569f7a 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 66e540efaca972..2ecdf53701030d 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 6d0021c85f20fb..d7c3e07b84efa8 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 d4f022ef021a44..270510a0131934 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;
>From 51cc7b499ece6a82bd5c45cf4a99b12e1b3825af Mon Sep 17 00:00:00 2001
From: Andrew Jenner <Andrew.Jenner at amd.com>
Date: Tue, 19 Nov 2024 09:26:02 -0500
Subject: [PATCH 2/2] Fix another instance of CachedFileStream not being
committed.
---
llvm/lib/CGData/CodeGenData.cpp | 3 +++
1 file changed, 3 insertions(+)
diff --git a/llvm/lib/CGData/CodeGenData.cpp b/llvm/lib/CGData/CodeGenData.cpp
index 88dcdfd1f931a2..c0f0053af43048 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,
More information about the lldb-commits
mailing list