[ThinLTO] Replace ErrorOr with Expected

[jansvoboda11]

The ModuleCacheEntry APIs return ErrorOr types. The implementations of these APIs call out to functions that return Expected, which cannot be always converted into ErrorOr. This patch propagates the Expected types into the ModuleCacheEntry APIs, resulting in fewer potential crashes (stemming from errorToErrorCode()) and better error-messages.

Moreover, cache misses are now represented by Expected<std::unique_ptr<MemoryBuffer>>(nullptr) instead of ErrorOr<std::unique_ptr<MemoryBuffer>>(std::error_code()) (error with non-error code?).

[llvmbot]

@llvm/pr-subscribers-lto

Author: Jan Svoboda (jansvoboda11)

Changes

The ModuleCacheEntry APIs return ErrorOr types. The implementations of these APIs call out to functions that return Expected, which cannot be always converted into ErrorOr. This patch propagates the Expected types into the ModuleCacheEntry APIs, resulting in fewer potential crashes (stemming from errorToErrorCode()) and better error-messages.

Moreover, cache misses are now represented by Expected<std::unique_ptr<MemoryBuffer>>(nullptr) instead of ErrorOr<std::unique_ptr<MemoryBuffer>>(std::error_code()) (error with non-error code?).

---

Full diff: https://github.com/llvm/llvm-project/pull/80088.diff

1 Files Affected:

• (modified) llvm/lib/LTO/ThinLTOCodeGenerator.cpp (+25-18)

diff --git a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
index 443439b71e756..ec37d756095d0 100644
--- a/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
+++ b/llvm/lib/LTO/ThinLTOCodeGenerator.cpp
@@ -397,19 +397,22 @@ class ModuleCacheEntry {
   // Access the path to this entry in the cache.
   StringRef getEntryPath() { return EntryPath; }
 
-  // Try loading the buffer for this cache entry.
-  ErrorOr<std::unique_ptr<MemoryBuffer>> tryLoadingBuffer() {
-    if (EntryPath.empty())
-      return std::error_code();
+  /// Try loading the buffer for this cache entry.
+  ///
+  /// \returns The buffer on cache hit, null on cache miss, or an error when
+  /// unable to load the cache contents.
+  Expected<std::unique_ptr<MemoryBuffer>> tryLoadingBuffer() {
+    if (EntryPath.empty() || !sys::fs::exists(EntryPath))
+      return nullptr;
     SmallString<64> ResultPath;
     Expected<sys::fs::file_t> FDOrErr = sys::fs::openNativeFileForRead(
-        Twine(EntryPath), sys::fs::OF_UpdateAtime, &ResultPath);
+        EntryPath, sys::fs::OF_UpdateAtime, &ResultPath);
     if (!FDOrErr)
-      return errorToErrorCode(FDOrErr.takeError());
+      return FDOrErr.takeError();
     ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr = MemoryBuffer::getOpenFile(
         *FDOrErr, EntryPath, /*FileSize=*/-1, /*RequiresNullTerminator=*/false);
     sys::fs::closeFile(*FDOrErr);
-    return MBOrErr;
+    return errorOrToExpected(std::move(MBOrErr));
   }
 
   // Cache the Produced object file
@@ -1153,18 +1156,22 @@ void ThinLTOCodeGenerator::run() {
         auto CacheEntryPath = CacheEntry.getEntryPath();
 
         {
-          auto ErrOrBuffer = CacheEntry.tryLoadingBuffer();
-          LLVM_DEBUG(dbgs() << "Cache " << (ErrOrBuffer ? "hit" : "miss")
+          std::unique_ptr<MemoryBuffer> MaybeBuffer;
+          if (auto E = CacheEntry.tryLoadingBuffer().moveInto(MaybeBuffer)) {
+            errs() << "remark: can't read cached file '" << CacheEntryPath
+                   << "': " << toString(std::move(E)) << "\n";
+          }
+          LLVM_DEBUG(dbgs() << "Cache " << (MaybeBuffer ? "hit" : "miss")
                             << " '" << CacheEntryPath << "' for buffer "
                             << count << " " << ModuleIdentifier << "\n");
 
-          if (ErrOrBuffer) {
+          if (MaybeBuffer) {
             // Cache Hit!
             if (SavedObjectsDirectoryPath.empty())
-              ProducedBinaries[count] = std::move(ErrOrBuffer.get());
+              ProducedBinaries[count] = std::move(MaybeBuffer);
             else
-              ProducedBinaryFiles[count] = writeGeneratedObject(
-                  count, CacheEntryPath, *ErrOrBuffer.get());
+              ProducedBinaryFiles[count] =
+                  writeGeneratedObject(count, CacheEntryPath, *MaybeBuffer);
             return;
           }
         }
@@ -1201,7 +1208,7 @@ void ThinLTOCodeGenerator::run() {
         CacheEntry.write(*OutputBuffer);
 
         if (SavedObjectsDirectoryPath.empty()) {
-          // We need to generated a memory buffer for the linker.
+          // We need to generate a memory buffer for the linker.
           if (!CacheEntryPath.empty()) {
             // When cache is enabled, reload from the cache if possible.
             // Releasing the buffer from the heap and reloading it from the
@@ -1209,13 +1216,13 @@ void ThinLTOCodeGenerator::run() {
             // The freed memory can be used for the next input file.
             // The final binary link will read from the VFS cache (hopefully!)
             // or from disk (if the memory pressure was too high).
-            auto ReloadedBufferOrErr = CacheEntry.tryLoadingBuffer();
-            if (auto EC = ReloadedBufferOrErr.getError()) {
+            std::unique_ptr<MemoryBuffer> MaybeBuffer;
+            if (auto E = CacheEntry.tryLoadingBuffer().moveInto(MaybeBuffer)) {
               // On error, keep the preexisting buffer and print a diagnostic.
               errs() << "remark: can't reload cached file '" << CacheEntryPath
-                     << "': " << EC.message() << "\n";
+                     << "': " << toString(std::move(E)) << "\n";
             } else {
-              OutputBuffer = std::move(*ReloadedBufferOrErr);
+              OutputBuffer = std::move(MaybeBuffer);
             }
           }
           ProducedBinaries[count] = std::move(OutputBuffer);

[MaskRay]

Does the extra !sys::fs::exists(EntryPath) cause any behavior change worth testing?

[jansvoboda11]

Yes, that's one aspect of this PR I'm not happy about. I think it's a time-of-check time-of-use bug, but something like this is necessary to be able to distinguish between cache miss and an actual error. Guarding this with a file lock might fix it, but I'd be interested in other ideas.

[cachemeifyoucan]

This is good in general as the reported error doesn't cause any fatal error, just some logs. But on the other hand, I can see problem here as the file can be deleted between exist check and read file. The file (the cache entry) deletion can be normal from a parallel link command that finishes and decide to prune the cache.

Maybe a general error from openNativeForRead should just return nullptr here, but some legit errors/misuses will not be diagnosed here.