Skip to content

Commit d16492d

Browse files
committed
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.
1 parent 8888352 commit d16492d

File tree

7 files changed

+53
-13
lines changed

7 files changed

+53
-13
lines changed

lldb/source/Core/DataFileCache.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,11 @@ bool DataFileCache::SetCachedData(llvm::StringRef key,
132132
if (file_or_err) {
133133
llvm::CachedFileStream *cfs = file_or_err->get();
134134
cfs->OS->write((const char *)data.data(), data.size());
135+
if (llvm::Error err = cfs->commit()) {
136+
Log *log = GetLog(LLDBLog::Modules);
137+
LLDB_LOG_ERROR(log, std::move(err),
138+
"failed to commit to the cache for key: {0}");
139+
}
135140
return true;
136141
} else {
137142
Log *log = GetLog(LLDBLog::Modules);

llvm/include/llvm/Support/Caching.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ class CachedFileStream {
3030
CachedFileStream(std::unique_ptr<raw_pwrite_stream> OS,
3131
std::string OSPath = "")
3232
: OS(std::move(OS)), ObjectPathName(OSPath) {}
33+
virtual Error commit() { return Error::success(); }
3334
std::unique_ptr<raw_pwrite_stream> OS;
3435
std::string ObjectPathName;
3536
virtual ~CachedFileStream() = default;

llvm/lib/Debuginfod/Debuginfod.cpp

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,6 +188,7 @@ class StreamedHTTPResponseHandler : public HTTPResponseHandler {
188188
public:
189189
StreamedHTTPResponseHandler(CreateStreamFn CreateStream, HTTPClient &Client)
190190
: CreateStream(CreateStream), Client(Client) {}
191+
Error commit();
191192
virtual ~StreamedHTTPResponseHandler() = default;
192193

193194
Error handleBodyChunk(StringRef BodyChunk) override;
@@ -210,6 +211,12 @@ Error StreamedHTTPResponseHandler::handleBodyChunk(StringRef BodyChunk) {
210211
return Error::success();
211212
}
212213

214+
Error StreamedHTTPResponseHandler::commit() {
215+
if (FileStream)
216+
return FileStream->commit();
217+
return Error::success();
218+
}
219+
213220
// An over-accepting simplification of the HTTP RFC 7230 spec.
214221
static bool isHeader(StringRef S) {
215222
StringRef Name;
@@ -298,6 +305,8 @@ Expected<std::string> getCachedOrDownloadArtifact(
298305
Error Err = Client.perform(Request, Handler);
299306
if (Err)
300307
return std::move(Err);
308+
if (Err = Handler.commit())
309+
return std::move(Err);
301310

302311
unsigned Code = Client.responseCode();
303312
if (Code && Code != 200)

llvm/lib/LTO/LTOBackend.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -432,6 +432,9 @@ static void codegen(const Config &Conf, TargetMachine *TM,
432432

433433
if (DwoOut)
434434
DwoOut->keep();
435+
436+
if (Error Err = Stream->commit())
437+
report_fatal_error(std::move(Err));
435438
}
436439

437440
static void splitCodeGen(const Config &C, TargetMachine *TM,

llvm/lib/Support/Caching.cpp

Lines changed: 29 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
8080
sys::fs::TempFile TempFile;
8181
std::string ModuleName;
8282
unsigned Task;
83+
bool Committed = false;
8384

8485
CacheStream(std::unique_ptr<raw_pwrite_stream> OS, AddBufferFn AddBuffer,
8586
sys::fs::TempFile TempFile, std::string EntryPath,
@@ -88,9 +89,10 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
8889
AddBuffer(std::move(AddBuffer)), TempFile(std::move(TempFile)),
8990
ModuleName(ModuleName), Task(Task) {}
9091

91-
~CacheStream() {
92-
// TODO: Manually commit rather than using non-trivial destructor,
93-
// allowing to replace report_fatal_errors with a return Error.
92+
Error commit() override {
93+
if (Committed)
94+
return Error::success();
95+
Committed = true;
9496

9597
// Make sure the stream is closed before committing it.
9698
OS.reset();
@@ -100,10 +102,12 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
100102
MemoryBuffer::getOpenFile(
101103
sys::fs::convertFDToNativeFile(TempFile.FD), ObjectPathName,
102104
/*FileSize=*/-1, /*RequiresNullTerminator=*/false);
103-
if (!MBOrErr)
104-
report_fatal_error(Twine("Failed to open new cache file ") +
105-
TempFile.TmpName + ": " +
106-
MBOrErr.getError().message() + "\n");
105+
if (!MBOrErr) {
106+
std::error_code EC = MBOrErr.getError();
107+
return createStringError(EC, Twine("Failed to open new cache file ") +
108+
TempFile.TmpName + ": " +
109+
EC.message() + "\n");
110+
}
107111

108112
// On POSIX systems, this will atomically replace the destination if
109113
// it already exists. We try to emulate this on Windows, but this may
@@ -118,7 +122,10 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
118122
E = handleErrors(std::move(E), [&](const ECError &E) -> Error {
119123
std::error_code EC = E.convertToErrorCode();
120124
if (EC != errc::permission_denied)
121-
return errorCodeToError(EC);
125+
return createStringError(
126+
EC, Twine("Failed to rename temporary file ") +
127+
TempFile.TmpName + " to " + ObjectPathName + ": " +
128+
EC.message() + "\n");
122129

123130
auto MBCopy = MemoryBuffer::getMemBufferCopy((*MBOrErr)->getBuffer(),
124131
ObjectPathName);
@@ -131,11 +138,22 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
131138
});
132139

133140
if (E)
134-
report_fatal_error(Twine("Failed to rename temporary file ") +
135-
TempFile.TmpName + " to " + ObjectPathName + ": " +
136-
toString(std::move(E)) + "\n");
141+
return E;
137142

138143
AddBuffer(Task, ModuleName, std::move(*MBOrErr));
144+
return Error::success();
145+
}
146+
147+
~CacheStream() {
148+
// In Debug builds, try to track down places where commit() was not
149+
// called before destruction.
150+
assert(Committed);
151+
// In Release builds, fall back to the previous behaviour of committing
152+
// during destruction and reporting errors with report_fatal_error.
153+
if (Committed)
154+
return;
155+
if (Error Err = commit())
156+
report_fatal_error(Twine(toString(std::move(Err))));
139157
}
140158
};
141159

llvm/tools/gold/gold-plugin.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1103,7 +1103,9 @@ static std::vector<std::pair<SmallString<128>, bool>> runLTO() {
11031103

11041104
auto AddBuffer = [&](size_t Task, const Twine &moduleName,
11051105
std::unique_ptr<MemoryBuffer> MB) {
1106-
*AddStream(Task, moduleName)->OS << MB->getBuffer();
1106+
auto Stream = *AddStream(Task, ModuleName);
1107+
Stream->OS << MB->getBuffer();
1108+
check(Stream->commit(), "Failed to commit cache");
11071109
};
11081110

11091111
FileCache Cache;

llvm/tools/llvm-lto2/llvm-lto2.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -448,7 +448,9 @@ static int run(int argc, char **argv) {
448448

449449
auto AddBuffer = [&](size_t Task, const Twine &ModuleName,
450450
std::unique_ptr<MemoryBuffer> MB) {
451-
*AddStream(Task, ModuleName)->OS << MB->getBuffer();
451+
auto Stream = AddStream(Task, ModuleName);
452+
*Stream->OS << MB->getBuffer();
453+
check(Stream->commit(), "Failed to commit cache");
452454
};
453455

454456
FileCache Cache;

0 commit comments

Comments
 (0)