VFS: Return new file systems as uniquely owned when possible, almost NFC

[dexonsmith]

Summary

Uniformly return uniquely-owned filesystems from VFS creation APIs. The
one exception is getRealFileSystem, which has a single instance and
needs to be shared.

This is almost NFC, except that it fixes a memory leak in
vfs::collectVFSFromYAML().

Depends on https://reviews.llvm.org/D92888

[JDevlieghere]

LGTM, thanks for cleaning that up!

[dblaikie]

This is almost NFC, except that it fixes a memory leak in vfs::collectVFSFromYAML().

Was this memory leak identified by any of the LLVM/Clang sanitizers, or at least valgrind? (if it was identified by sanitizers I wonder why it persisted so long, given we have sanitizer bots folks like to keep green, etc)

[dexonsmith]

In D92890#2453582, @dblaikie wrote:

This is almost NFC, except that it fixes a memory leak in vfs::collectVFSFromYAML().

Was this memory leak identified by any of the LLVM/Clang sanitizers, or at least valgrind? (if it was identified by sanitizers I wonder why it persisted so long, given we have sanitizer bots folks like to keep green, etc)

Not that I'm aware of, I just noticed it when I was in there. I'm not sure why -fsanitize=address never flagged this; I'd have thought in-tree clang tests using -ivfsoverlay would exercise this.

llvm/include/llvm/Support/VirtualFileSystem.h

public:
   std::error_code setCurrentWorkingDirectory(const Twine &Path) override;
 };
 
 /// Get a globally unique ID for a virtual file or directory.
 llvm::sys::fs::UniqueID getNextVirtualUniqueID();
 
 /// Gets a \p FileSystem for a virtual file system described in YAML
 /// format.
-IntrusiveRefCntPtr<FileSystem>
+std::unique_ptr<FileSystem>
 getVFSFromYAML(std::unique_ptr<llvm::MemoryBuffer> Buffer,
                llvm::SourceMgr::DiagHandlerTy DiagHandler,
                StringRef YAMLFilePath, void *DiagContext = nullptr,
                IntrusiveRefCntPtr<FileSystem> ExternalFS = getRealFileSystem());
 
 struct YAMLVFSEntry {
   template <typename T1, typename T2>
   YAMLVFSEntry(T1 &&VPath, T2 &&RPath, bool IsDirectory = false)
#endif
   ErrorOr<Status> status(const Twine &Path, Entry *E);
 
 public:
   /// Looks up \p Path in \c Roots.
   ErrorOr<Entry *> lookupPath(const Twine &Path) const;
 
   /// Parses \p Buffer, which is expected to be in YAML format and
   /// returns a virtual file system representing its contents.
-  static RedirectingFileSystem *
+  static std::unique_ptr<RedirectingFileSystem>
   create(std::unique_ptr<MemoryBuffer> Buffer,
          SourceMgr::DiagHandlerTy DiagHandler, StringRef YAMLFilePath,
          void *DiagContext, IntrusiveRefCntPtr<FileSystem> ExternalFS);
 
   ErrorOr<Status> status(const Twine &Path) override;
   ErrorOr<std::unique_ptr<File>> openFileForRead(const Twine &Path) override;
 
   std::error_code getRealPath(const Twine &Path,

llvm/lib/Support/VirtualFileSystem.cpp

bool parse(yaml::Node *Root, RedirectingFileSystem *FS) {
     // inside the VFS.
     for (auto &E : RootEntries)
       uniqueOverlayTree(FS, E.get());
 
     return true;
   }
 };
 
-RedirectingFileSystem *
+std::unique_ptr<RedirectingFileSystem>
 RedirectingFileSystem::create(std::unique_ptr<MemoryBuffer> Buffer,
                               SourceMgr::DiagHandlerTy DiagHandler,
                               StringRef YAMLFilePath, void *DiagContext,
                               IntrusiveRefCntPtr<FileSystem> ExternalFS) {
   SourceMgr SM;
   yaml::Stream Stream(Buffer->getMemBufferRef(), SM);
 
   SM.setDiagHandler(DiagHandler, DiagContext);
if (!YAMLFilePath.empty()) {
     assert(!EC && "Overlay dir final path must be absolute");
     (void)EC;
     FS->setExternalContentsPrefixDir(OverlayAbsDir);
   }
 
   if (!P.parse(Root, FS.get()))
     return nullptr;
 
-  return FS.release();
+  return FS;
 }
 
 ErrorOr<RedirectingFileSystem::Entry *>
 RedirectingFileSystem::lookupPath(const Twine &Path_) const {
   SmallString<256> Path;
   Path_.toVector(Path);
 
   // Handle relative paths
if (auto *F =
     return ExternalFS->getRealPath(F->getExternalContentsPath(), Output);
   }
   // Even if there is a directory entry, fall back to ExternalFS if allowed,
   // because directories don't have a single external contents path.
   return shouldUseExternalFS() ? ExternalFS->getRealPath(Path, Output)
                                : llvm::errc::invalid_argument;
 }
 
-IntrusiveRefCntPtr<FileSystem>
+std::unique_ptr<FileSystem>
 vfs::getVFSFromYAML(std::unique_ptr<MemoryBuffer> Buffer,
                     SourceMgr::DiagHandlerTy DiagHandler,
                     StringRef YAMLFilePath, void *DiagContext,
                     IntrusiveRefCntPtr<FileSystem> ExternalFS) {
   return RedirectingFileSystem::create(std::move(Buffer), DiagHandler,
                                        YAMLFilePath, DiagContext,
                                        std::move(ExternalFS));
 }

 }
 
 void vfs::collectVFSFromYAML(std::unique_ptr<MemoryBuffer> Buffer,
                              SourceMgr::DiagHandlerTy DiagHandler,
                              StringRef YAMLFilePath,
                              SmallVectorImpl<YAMLVFSEntry> &CollectedEntries,
                              void *DiagContext,
                              IntrusiveRefCntPtr<FileSystem> ExternalFS) {
-  RedirectingFileSystem *VFS = RedirectingFileSystem::create(
+  std::unique_ptr<RedirectingFileSystem> VFS = RedirectingFileSystem::create(
       std::move(Buffer), DiagHandler, YAMLFilePath, DiagContext,
       std::move(ExternalFS));
   ErrorOr<RedirectingFileSystem::Entry *> RootE = VFS->lookupPath("/");
   if (!RootE)
     return;
   SmallVector<StringRef, 8> Components;
   Components.push_back("/");
   getVFSEntries(*RootE, Components, CollectedEntries);

llvm/unittests/Support/VirtualFileSystemTest.cpp

public:
 
   void SetUp() override { NumDiagnostics = 0; }
 
   static void CountingDiagHandler(const SMDiagnostic &, void *Context) {
     VFSFromYAMLTest *Test = static_cast<VFSFromYAMLTest *>(Context);
     ++Test->NumDiagnostics;
   }
 
-  IntrusiveRefCntPtr<vfs::FileSystem>
+  std::unique_ptr<vfs::FileSystem>
   getFromYAMLRawString(StringRef Content,
                        IntrusiveRefCntPtr<vfs::FileSystem> ExternalFS) {
     std::unique_ptr<MemoryBuffer> Buffer = MemoryBuffer::getMemBuffer(Content);
     return getVFSFromYAML(std::move(Buffer), CountingDiagHandler, "", this,
                           ExternalFS);
   }
 
-  IntrusiveRefCntPtr<vfs::FileSystem> getFromYAMLString(
+  std::unique_ptr<vfs::FileSystem> getFromYAMLString(
       StringRef Content,
       IntrusiveRefCntPtr<vfs::FileSystem> ExternalFS = new DummyFileSystem()) {
     std::string VersionPlusContent("{\n  'version':0,\n");
     VersionPlusContent += Content.slice(Content.find('{') + 1, StringRef::npos);
     return getFromYAMLRawString(VersionPlusContent, ExternalFS);
   }
 
   // This is intended as a "XFAIL" for windows hosts.