[WIP][FileCollector] move Root creation

[jkorous]

Summary

I might be missing something be since we aren't checking the return value of create_directories in constructor I'd say we can't really rely on that as part of the state.
Maybe we can just create it later and actually propagate the error? Or maybe just remove it completely as it'd get created in copyFiles when we iterate VFSWriter.getMappings()?

[JDevlieghere]

This seems reasonable. Another alternative might be taking the error_code as an output argument if we want to be eager in creating the root directory. However, I think this is a better solution.

I probably added it to the ctor thinking that you'd rather fail early rather than when something goes wrong. For lldb for example it would be disappointing to reproduce a bug with reproducer capture enabled, only to find out at the last minute that we couldn't capture the files because we couldn't create the root. However, I don't think that's something we should optimize for. The reproducers will already create the reproducer root directory, and it seems unlikely that this would succeed, but then creating a subdirectory (the FileCollector root) would fail.

LGTM.

[jkorous]

In D78959#2006231, @JDevlieghere wrote:

For lldb for example it would be disappointing to reproduce a bug with reproducer capture enabled, only to find out at the last minute that we couldn't capture the files because we couldn't create the root.

Yes, this was the only reason I thought we might want to create it eagerly. Maybe we should then make existence of the dir (and maybe write access) an explicit requirement in doc comment and push this responsibility to client code? WDYT?

[JDevlieghere]

In D78959#2006318, @jkorous wrote:

In D78959#2006231, @JDevlieghere wrote:

For lldb for example it would be disappointing to reproduce a bug with reproducer capture enabled, only to find out at the last minute that we couldn't capture the files because we couldn't create the root.

Yes, this was the only reason I thought we might want to create it eagerly. Maybe we should then make existence of the dir (and maybe write access) an explicit requirement in doc comment and push this responsibility to client code? WDYT?

I would keep the approach in the patch. If the directory already exists then create_directories is a no-op, so if the client wants to deal with the error eagerly they can, otherwise they can rely on copyFiles doing the right thing. It would be good to include that in the docs.

llvm/include/llvm/Support/FileCollector.h

 #include <mutex>
 
 namespace llvm {
 class FileCollectorFileSystem;
 /// Collects files into a directory and generates a mapping that can be used by
 /// the VFS.
 class FileCollector {
 public:
+  /// \p Root directory gets created in copyFiles unless it already exists.
   FileCollector(std::string Root, std::string OverlayRoot);
 
   void addFile(const Twine &file);
   void addDirectory(const Twine &Dir);
 
   /// Write the yaml mapping (for the VFS) to the given file.
   std::error_code writeMapping(StringRef MappingFile);
 

llvm/lib/Support/FileCollector.cpp

static bool isCaseSensitivePath(StringRef Path) {
   UpperDest = Path.upper();
   if (sys::fs::real_path(UpperDest, RealDest) && Path.equals(RealDest))
     return false;
   return true;
 }
 
 FileCollector::FileCollector(std::string Root, std::string OverlayRoot)
     : Root(std::move(Root)), OverlayRoot(std::move(OverlayRoot)) {
-  sys::fs::create_directories(this->Root, true);
 }
 
 bool FileCollector::getRealPath(StringRef SrcPath,
                                 SmallVectorImpl<char> &Result) {
   SmallString<256> RealPath;
   StringRef FileName = sys::path::filename(SrcPath);
   std::string Directory = sys::path::parent_path(SrcPath).str();
   auto DirWithSymlink = SymlinkMap.find(Directory);
copyAccessAndModificationTime(StringRef Filename,
 
   if (auto EC = sys::Process::SafelyCloseFileDescriptor(FD))
     return EC;
 
   return {};
 }
 
 std::error_code FileCollector::copyFiles(bool StopOnError) {
+  auto Err = sys::fs::create_directories(Root, /*IgnoreExisting=*/true);
+  if (Err) {
+    return Err;
+  }
+
   std::lock_guard<std::mutex> lock(Mutex);
 
   for (auto &entry : VFSWriter.getMappings()) {
     // Create directory tree.
     if (std::error_code EC =
             sys::fs::create_directories(sys::path::parent_path(entry.RPath),
                                         /*IgnoreExisting=*/true)) {
       if (StopOnError)