https://github.com/anjenner updated
https://github.com/llvm/llvm-project/pull/115331
>From 3fdba46d41ad9668a114766fe892af497f121cd5 Mon Sep 17 00:00:00 2001
From: Andrew Jenner <andrew.jen...@amd.com>
Date: Thu, 7 Nov 2024 10:47:42 -0500
Subject: [PATCH 1/6] 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 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..120bcd9da02ed 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 4c785117ae8ef..6ff889d3a8cad 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 bdf4ff8960bc8..4bfa8751a4364 100644
--- a/llvm/lib/LTO/LTOBackend.cpp
+++ b/llvm/lib/LTO/LTOBackend.cpp
@@ -434,6 +434,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..2ecdf53701030 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 6d0021c85f20f..d7c3e07b84efa 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 d4f022ef021a4..270510a013193 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 77829009d4ff1fe8326e87330b19dd6430384448 Mon Sep 17 00:00:00 2001
From: Andrew Jenner <andrew.jen...@amd.com>
Date: Tue, 19 Nov 2024 09:26:02 -0500
Subject: [PATCH 2/6] 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 88dcdfd1f931a..c0f0053af4304 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,
>From 8f9985601087a046845032db8d5d1dbdc0dc64de Mon Sep 17 00:00:00 2001
From: Andrew Jenner <andrew.jen...@amd.com>
Date: Thu, 5 Dec 2024 10:56:20 -0500
Subject: [PATCH 3/6] Address review feedback.
---
llvm/lib/Support/Caching.cpp | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/llvm/lib/Support/Caching.cpp b/llvm/lib/Support/Caching.cpp
index 2ecdf53701030..15df5deae1d44 100644
--- a/llvm/lib/Support/Caching.cpp
+++ b/llvm/lib/Support/Caching.cpp
@@ -91,7 +91,8 @@ Expected<FileCache> llvm::localCache(const Twine
&CacheNameRef,
Error commit() override {
if (Committed)
- return Error::success();
+ return createStringError(std::errc::invalid_argument,
+ Twine("CacheStream already committed."));
Committed = true;
// Make sure the stream is closed before committing it.
@@ -145,15 +146,8 @@ Expected<FileCache> llvm::localCache(const Twine
&CacheNameRef,
}
~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))));
+ if (!Committed)
+ report_fatal_error(Twine("CacheStream was not committed.\n");
}
};
>From 46b18d8fc2a5f8683cde113cc1812b7f7468040c Mon Sep 17 00:00:00 2001
From: Andrew Jenner <andrew.jen...@amd.com>
Date: Tue, 11 Feb 2025 07:51:30 -0500
Subject: [PATCH 4/6] Fix build errors.
---
llvm/lib/Support/Caching.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/llvm/lib/Support/Caching.cpp b/llvm/lib/Support/Caching.cpp
index 15df5deae1d44..9d94c4fba0f14 100644
--- a/llvm/lib/Support/Caching.cpp
+++ b/llvm/lib/Support/Caching.cpp
@@ -91,7 +91,7 @@ Expected<FileCache> llvm::localCache(const Twine
&CacheNameRef,
Error commit() override {
if (Committed)
- return createStringError(std::errc::invalid_argument,
+ return
createStringError(make_error_code(std::errc::invalid_argument),
Twine("CacheStream already committed."));
Committed = true;
@@ -147,7 +147,7 @@ Expected<FileCache> llvm::localCache(const Twine
&CacheNameRef,
~CacheStream() {
if (!Committed)
- report_fatal_error(Twine("CacheStream was not committed.\n");
+ report_fatal_error("CacheStream was not committed.\n");
}
};
>From 987f307fd361190a4801de3161db9c2f5dd7156d Mon Sep 17 00:00:00 2001
From: Andrew Jenner <andrew.jen...@amd.com>
Date: Thu, 27 Feb 2025 06:52:23 -0500
Subject: [PATCH 5/6] Add unit test for Caching functionality.
---
llvm/unittests/Support/CMakeLists.txt | 1 +
llvm/unittests/Support/Caching.cpp | 116 ++++++++++++++++++++++++++
2 files changed, 117 insertions(+)
create mode 100644 llvm/unittests/Support/Caching.cpp
diff --git a/llvm/unittests/Support/CMakeLists.txt
b/llvm/unittests/Support/CMakeLists.txt
index 6de8165826442..c78e6a370beac 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..55e762cb50908
--- /dev/null
+++ b/llvm/unittests/Support/Caching.cpp
@@ -0,0 +1,116 @@
+//===- 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()));
+}
>From c8ed684c6e285fe365e8b3073d8a169cd6934cd3 Mon Sep 17 00:00:00 2001
From: Andrew Jenner <andrew.jen...@amd.com>
Date: Thu, 27 Feb 2025 10:05:33 -0500
Subject: [PATCH 6/6] Placate code formatting check.
---
llvm/unittests/Support/Caching.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/llvm/unittests/Support/Caching.cpp
b/llvm/unittests/Support/Caching.cpp
index 55e762cb50908..fb63b0c1a9762 100644
--- a/llvm/unittests/Support/Caching.cpp
+++ b/llvm/unittests/Support/Caching.cpp
@@ -109,7 +109,8 @@ TEST(Caching, WriteAfterCommit) {
(*CFS->OS).write(data, sizeof(data));
ASSERT_THAT_ERROR(CFS->commit(), Succeeded());
- EXPECT_DEATH({ (*CFS->OS).write(data, sizeof(data)); }, "")
+ EXPECT_DEATH(
+ { (*CFS->OS).write(data, sizeof(data)); }, "")
<< "Write after commit did not cause abort";
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
