[Support] [DebugInfo] Lazily create cache dir.

[mysterymath]

Summary

This change defers creating Support/Caching.cpp's cache directory until
it actually writes to the cache.

This allows using Caching library in a read-only fashion. If read-only,
the cache is guaranteed not to write to disk. This keeps tools using
DebugInfod (currently llvm-symbolizer) hermetic when not configured to
perform remote lookups.

[phosek]

llvm/lib/Support/Caching.cpp

153

Do you think we could avoid static and call sys::fs::create_directories(Path, /*IgnoreExisting=*/true) unconditionally?

[mysterymath]

Removed magic static wrapper; premature optimization.

llvm/lib/Support/Caching.cpp

153

Yeah, looking at this with fresh eyes, the attempt to avoid a syscall here is a premature optimization. Removed.

[phosek]

LGTM

llvm/lib/Support/Caching.cpp

20

Is this needed?

[mysterymath]

Remove unnecessary include.

llvm/lib/Support/Caching.cpp

20

Nope, removed.

[zhuhan0]

This is causing a failure in LLVM :: tools/gold/X86/cache.ll on our side. Do you see the same failure?

...
ls: cannot access '/home/zhuhan/server-llvm-build/auto-rebase/test/tools/gold/X86/Output/cache.ll.tmp.cache': No such file or directory
Expected 1 lines, got 0.

[mysterymath]

In D117589#3260421, @zhuhan0 wrote:

This is causing a failure in LLVM :: tools/gold/X86/cache.ll on our side. Do you see the same failure?

...
ls: cannot access '/home/zhuhan/server-llvm-build/auto-rebase/test/tools/gold/X86/Output/cache.ll.tmp.cache': No such file or directory
Expected 1 lines, got 0.

I've verified the issue, and I've got a quick patch to the test case.
I'm going to go through all of the other uses of localCache and see if any of their tests similarly need updating; I didn't notice that my local build didn't include these.
Shouldn't take too long; I'll add you to the patch once it's ready.

[zhuhan0]

Sounds good. Thank you.

llvm/include/llvm/Support/Caching.h

 
 /// This type defines the callback to add a pre-existing file (e.g. in a cache).
 ///
 /// Buffer callbacks must be thread safe.
 using AddBufferFn =
     std::function<void(unsigned Task, std::unique_ptr<MemoryBuffer> MB)>;
 
 /// Create a local file system cache which uses the given cache name, temporary
-/// file prefix, cache directory and file callback. This function also creates
-/// the cache directory if it does not already exist. The cache name appears in
-/// error messages for errors during caching. The temporary file prefix is used
-/// in the temporary file naming scheme used when writing files atomically.
+/// file prefix, cache directory and file callback.  This function does not
+/// immediately create the cache directory if it does not yet exist; this is
+/// done lazily the first time a file is added.  The cache name appears in error
+/// messages for errors during caching. The temporary file prefix is used in the
+/// temporary file naming scheme used when writing files atomically.
 Expected<FileCache> localCache(
     Twine CacheNameRef, Twine TempFilePrefixRef, Twine CacheDirectoryPathRef,
     AddBufferFn AddBuffer = [](size_t Task, std::unique_ptr<MemoryBuffer> MB) {
     });
 } // namespace llvm
 
 #endif

llvm/lib/Support/Caching.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/Caching.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 
 #if !defined(_MSC_VER) && !defined(__MINGW32__)
 #include <unistd.h>
 #else
 #include <io.h>
 #endif
 
 using namespace llvm;
 
 Expected<FileCache> llvm::localCache(Twine CacheNameRef,
                                      Twine TempFilePrefixRef,
                                      Twine CacheDirectoryPathRef,
                                      AddBufferFn AddBuffer) {
-  if (std::error_code EC = sys::fs::create_directories(CacheDirectoryPathRef))
-    return errorCodeToError(EC);
 
   // Create local copies which are safely captured-by-copy in lambdas
   SmallString<64> CacheName, TempFilePrefix, CacheDirectoryPath;
   CacheNameRef.toVector(CacheName);
   TempFilePrefixRef.toVector(TempFilePrefix);
   CacheDirectoryPathRef.toVector(CacheDirectoryPath);
 
   return [=](unsigned Task, StringRef Key) -> Expected<AddStreamFn> {
struct CacheStream : CachedFileStream {
                              TempFile.TmpName + " to " + ObjectPathName + ": " +
                              toString(std::move(E)) + "\n");
 
         AddBuffer(Task, std::move(*MBOrErr));
       }
     };
 
     return [=](size_t Task) -> Expected<std::unique_ptr<CachedFileStream>> {
+      // Create the cache directory if not already done. Doing this lazily
+      // ensures the filesystem isn't mutated until the cache is.
+      if (std::error_code EC = sys::fs::create_directories(
+              CacheDirectoryPath, /*IgnoreExisting=*/true))
+        return errorCodeToError(EC);
+
       // Write to a temporary to avoid race condition
       SmallString<64> TempFilenameModel;
       sys::path::append(TempFilenameModel, CacheDirectoryPath,
                         TempFilePrefix + "-%%%%%%.tmp.o");
       Expected<sys::fs::TempFile> Temp = sys::fs::TempFile::create(
           TempFilenameModel, sys::fs::owner_read | sys::fs::owner_write);
       if (!Temp)
         return createStringError(errc::io_error,
                                  toString(Temp.takeError()) + ": " + CacheName +
                                      ": Can't get a temporary file");
 
       // This CacheStream will move the temporary file into the cache when done.
       return std::make_unique<CacheStream>(
           std::make_unique<raw_fd_ostream>(Temp->FD, /* ShouldClose */ false),
           AddBuffer, std::move(*Temp), std::string(EntryPath.str()), Task);
     };
   };
 }

llvm/test/ThinLTO/X86/cache.ll

 ; RUN: ls %t.cache.noindex | count 1
 
 ; Verify that enabling caching is ignoring module without hash with llvm-lto2
 ; RUN: rm -Rf %t.cache.noindex
 ; RUN: llvm-lto2 run -o %t.o %t2.bc  %t.bc -cache-dir %t.cache.noindex \
 ; RUN:  -r=%t2.bc,_main,plx \
 ; RUN:  -r=%t2.bc,_globalfunc,lx \
 ; RUN:  -r=%t.bc,_globalfunc,plx
-; RUN: ls %t.cache.noindex | count 0
+; RUN: not ls %t.cache.noindex
 
 
 ; Repeat again, *with* hash this time.
 
 ; RUN: opt -module-hash -module-summary %s -o %t.bc
 ; RUN: opt -module-hash -module-summary %p/Inputs/cache.ll -o %t2.bc
 
 ; Verify that enabling caching is working, and that the pruner only removes

llvm/test/ThinLTO/X86/empty_module_with_cache.ll

 ; RUN: llvm-lto -thinlto-action=run %t2.bc  %t.bc -exported-symbol=main -thinlto-cache-dir %t.cache
 ; RUN: ls %t.cache/llvmcache.timestamp
 ; RUN: ls %t.cache | count 1
 
 ; Verify that caching is disabled for module without hash, with llvm-lto2
 ; RUN: rm -Rf %t.cache
 ; RUN: llvm-lto2 run -o %t.o %t2.bc  %t.bc -cache-dir %t.cache \
 ; RUN:  -r=%t2.bc,_main,plx
-; RUN: ls %t.cache | count 0
+; RUN: not ls %t.cache
 
 
 target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.11.0"

llvm/unittests/Debuginfod/DebuginfodTests.cpp

 #include "llvm/Support/Path.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gtest/gtest.h"
 
 #ifdef _WIN32
 #define setenv(name, var, ignore) _putenv_s(name, var)
 #endif
 
+#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 {                                                                     \
+  }
+
 using namespace llvm;
 
 // Check that the Debuginfod client can find locally cached artifacts.
 TEST(DebuginfodClient, CacheHit) {
   int FD;
   SmallString<64> CachedFilePath;
   sys::fs::createTemporaryFile("llvmcache-key", "temp", FD, CachedFilePath);
   StringRef CacheDir = sys::path::parent_path(CachedFilePath);
   StringRef UniqueKey = sys::path::filename(CachedFilePath);
   EXPECT_TRUE(UniqueKey.consume_front("llvmcache-"));
   raw_fd_ostream OF(FD, true, /*unbuffered=*/true);
   OF << "contents\n";
   OF << CacheDir << "\n";
   OF.close();
   Expected<std::string> PathOrErr = getCachedOrDownloadArtifact(
       UniqueKey, /*UrlPath=*/"/null", CacheDir,
       /*DebuginfodUrls=*/{}, /*Timeout=*/std::chrono::milliseconds(1));
   EXPECT_THAT_EXPECTED(PathOrErr, HasValue(CachedFilePath));
 }
 
 // Check that the Debuginfod client returns an Error when it fails to find an
 // artifact.
 TEST(DebuginfodClient, CacheMiss) {
-  // Set the cache path to a temp directory to avoid permissions issues if $HOME
-  // is not writable.
-  SmallString<32> TempDir;
-  sys::path::system_temp_directory(true, TempDir);
-  setenv("DEBUGINFOD_CACHE_PATH", TempDir.c_str(),
+  SmallString<32> CacheDir;
+  ASSERT_NO_ERROR(
+      sys::fs::createUniqueDirectory("debuginfod-unittest", CacheDir));
+  sys::path::append(CacheDir, "cachedir");
+  ASSERT_FALSE(sys::fs::exists(CacheDir));
+  setenv("DEBUGINFOD_CACHE_PATH", CacheDir.c_str(),
          /*replace=*/1);
   // Ensure there are no urls to guarantee a cache miss.
   setenv("DEBUGINFOD_URLS", "", /*replace=*/1);
   HTTPClient::initialize();
   Expected<std::string> PathOrErr = getCachedOrDownloadArtifact(
       /*UniqueKey=*/"nonexistent-key", /*UrlPath=*/"/null");
   EXPECT_THAT_EXPECTED(PathOrErr, Failed<StringError>());
+  // A cache miss with no possible URLs should not create the cache directory.
+  EXPECT_FALSE(sys::fs::exists(CacheDir));
 }