Modify the localCache API to require an explicit commit on CachedFile… #115331
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
lldb
debuginfo
LTO
|
@llvm/pr-subscribers-lto @llvm/pr-subscribers-lldb Author: None (anjenner) Changes…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. Full diff: https://github.com/llvm/llvm-project/pull/115331.diff 7 Files Affected:
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;
|
anjenner
force-pushed
the
explicit-commit-in-CachedFileStream
branch
from
67167aa to
d16492d
Compare
anjenner
force-pushed
the
explicit-commit-in-CachedFileStream
branch
from
51cc7b4 to
7782900
Compare
…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.
|
Ping for code review please - thank you. |
kyulee-com
reviewed
Dec 2, 2024
llvm/lib/Support/Caching.cpp
Outdated
| // allowing to replace report_fatal_errors with a return Error. | ||
| Error commit() override { | ||
| if (Committed) | ||
| return Error::success(); |
There was a problem hiding this comment.
Intuitively, this doesn't seem correct. Shouldn't we fail if we attempt to commit more than once, rather than just returning a success?
Additionally, I'm curious about what happens if we use this CacheStream after a commit has been called. I suspect it will fail, but do we provide a proper error message in this case? It would be ideal to have a test case to cover these scenarios.
There was a problem hiding this comment.
Fair enough. My thinking was that commit() would be idempotent and calling it a second time would be harmless. But I understand the potential for confusion so I'll make it fail instead. After commit() has been called, the OS member will have been reset() and I think will cause the any calling code to do a pure virtual function call if it tries to write to it. I'll have a think about how we could better there and make a testcase. Probably setting OS to a raw_fd_ostream with an fd of zero instead of a default-constructed raw_pwrite_ostream would give a better error message.
There was a problem hiding this comment.
I added a unit test and also tested what happens if the stream is written after a commit. It aborts the process with an assert failure:
/usr/include/c++/11/bits/unique_ptr.h:407: typename std::add_lvalue_reference<_Tp>::type std::unique_ptr<_Tp, _Dp>::operator*() const [with _Tp = llvm::raw_pwrite_stream; _Dp = std::default_deletellvm::raw_pwrite_stream; typename std::add_lvalue_reference<_Tp>::type = llvm::raw_pwrite_stream&]: Assertion 'get() != pointer()' failed.
This is the same thing that will happen anywhere else in LLVM if an output stream is used after its reset() method has been called. This seems reasonable to me - it will be very obvious if a CacheStream is used after commit, so any such bugs will be found and fixed quickly. Providing a proper error message is of limited value since this should never happen unless there is a bug in the code that is calling the caching API - end users of the compiler won't see it. If such an error message is desirable then it should be implemented at the raw_ostream level so that so it appears for other "write after reset" bugs, so it is outside of the scope of this PR.
llvm/lib/Support/Caching.cpp
Outdated
| ~CacheStream() { | ||
| // In Debug builds, try to track down places where commit() was not | ||
| // called before destruction. | ||
| assert(Committed); |
There was a problem hiding this comment.
While I understand the intent here, it's unusual to have different behavior in debug and release modes. The release mode operation, where commit() is essentially optional because it's handled by the destructor, seems reasonable to me. For consistency, I would prefer to remove this assert, although you can use it for testing purposes. Are you aiming to use commit() optionally to make error messages more explicit?
Alternatively, if we require commit() to be used instead of relying on the destructor, I would suggest explicitly failing or emitting a warning if it hasn't been committed beforehand.
There was a problem hiding this comment.
The advantage of explicit commit is that it allows proper error handling rather than aborting the entire process if the cache write fails - usually an undesirable behavior. My aim here was to quietly fall back to the previous behavior in if commit() was not called in release mode but help developers track down such places in debug mode (which worked perfectly - I found just such a case that way). I was thinking of leaving it there to help people who have code that uses localCache in branches that have not yet landed in mainline LLVM. However, I agree that it would be annoying to switch from release to debug to track down an unrelated problem and have to fix a missing commit() at that point. I think on balance explicitly failing here is the better plan - I'll modify my patch to do this but I could probably be talked into allowing the commit() to be optional if anyone has strong feelings about this.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
I'm a little concerned about the added complexity of the cache interfaces. At the very least, the required usage of the commit() function should be clearly documented probably in the main Caching.h header, however, it is a bit confusing right now because there is no requirement to use that in the base class (comment on that below). There should also be comments on the other commit() method declarations.
| << "error message: " << ASSERT_NO_ERROR_ec.message() << "\n"; \ | ||
| GTEST_FATAL_FAILURE_(MessageStorage.c_str()); \ | ||
| } else { \ | ||
| } |
There was a problem hiding this comment.
This is a standard defensive programming technique with multiline macros to cause a compile error when someone writes "ASSERT_NO_ERROR(...) else { ... }" by mistake.
| @@ -0,0 +1,116 @@ | |||
| //===- Caching.cpp --------------------------------------------------------===// | |||
There was a problem hiding this comment.
Can you test the error when there is no commit?
There was a problem hiding this comment.
Yes - I have added a test in my latest commit.
llvm/include/llvm/Support/Caching.h
Outdated
| @@ -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(); } | |||
There was a problem hiding this comment.
If the expectation is that commit() should now always be called, should there be some error detection in the base class?
There was a problem hiding this comment.
I have moved the error detection to the base class.
anjenner
force-pushed
the
explicit-commit-in-CachedFileStream
branch
from
de5effe to
46660c1
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/14226 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/155/builds/7284 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/186/builds/7106 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/72/builds/8923 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/145/builds/5513 Here is the relevant piece of the build log for the reference |
vitalybuka
added a commit
that referenced
this pull request
Mar 7, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/146/builds/2436 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/64/builds/2433 Here is the relevant piece of the build log for the reference |
|
@anjenner are you looking at the remaining reported buildbot failures? E.g. the unit testing failure below. Please fix or revert this change.
|
|
@anjenner The unittest you added at
Can you take a look and make this test more reliable? |
dyung
added a commit
that referenced
this pull request
Mar 8, 2025
…chedFile… (#115331)" This reverts commit ce9e1d3. 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
jph-13
pushed a commit
to jph-13/llvm-project
that referenced
this pull request
Mar 21, 2025
llvm#115331) …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.
jph-13
pushed a commit
to jph-13/llvm-project
that referenced
this pull request
Mar 21, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/38/builds/2638 Here is the relevant piece of the build log for the reference |
justinkb
pushed a commit
to justinkb/llvm-project
that referenced
this pull request
Apr 14, 2025
…chedFile… (llvm#115331)" This reverts commit ce9e1d3. 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
justinkb
pushed a commit
to justinkb/llvm-project
that referenced
this pull request
Apr 14, 2025
…chedFile… (llvm#115331)" This reverts commit ce9e1d3. 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
anjenner
pushed a commit
to anjenner/llvm-project
that referenced
this pull request
Apr 17, 2025
|
New PR for this: #136121 . |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/98/builds/1285 Here is the relevant piece of the build log for the reference |
anjenner
added a commit
that referenced
this pull request
Apr 22, 2025
#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 #115331 . I have incorporated a change to the testcase to make it more reliable on Windows, as well as two follow-up changes (df79000 and b0baa1d) that were also reverted when 115331 was reverted. --------- Co-authored-by: Augie Fackler <[email protected]> Co-authored-by: Vitaly Buka <[email protected]>
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
May 6, 2025
… 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 llvm/llvm-project#115331 . I have incorporated a change to the testcase to make it more reliable on Windows, as well as two follow-up changes (llvm/llvm-project@df79000 and llvm/llvm-project@b0baa1d) that were also reverted when 115331 was reverted. --------- Co-authored-by: Augie Fackler <[email protected]> Co-authored-by: Vitaly Buka <[email protected]>
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
llvm#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 llvm#115331 . I have incorporated a change to the testcase to make it more reliable on Windows, as well as two follow-up changes (llvm@df79000 and llvm@b0baa1d) that were also reverted when 115331 was reverted. --------- Co-authored-by: Augie Fackler <[email protected]> Co-authored-by: Vitaly Buka <[email protected]>
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
llvm#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 llvm#115331 . I have incorporated a change to the testcase to make it more reliable on Windows, as well as two follow-up changes (llvm@df79000 and llvm@b0baa1d) that were also reverted when 115331 was reverted. --------- Co-authored-by: Augie Fackler <[email protected]> Co-authored-by: Vitaly Buka <[email protected]>
IanWood1
pushed a commit
to IanWood1/llvm-project
that referenced
this pull request
May 6, 2025
llvm#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 llvm#115331 . I have incorporated a change to the testcase to make it more reliable on Windows, as well as two follow-up changes (llvm@df79000 and llvm@b0baa1d) that were also reverted when 115331 was reverted. --------- Co-authored-by: Augie Fackler <[email protected]> Co-authored-by: Vitaly Buka <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…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.