[LibTooling] Make interface of VFS injection into ClangTool more user-friendly

[whisperity]

Summary

This patch extends upon D41947 because the interface that was landed from that patch isn't much user-friendly.

Injecting a custom virtual file system into the tool is a dangerous operation. If the real file system (where the installed Clang's headers reside) are not mirrored, it only turns out at ClangTool::run() that something was not mounted properly. Originally, the execution of a ClangTool always used the real filesystem.

Starting with D41947, the client code setting the tool up must manually create the OverlayFS (which is, in turn, added as an OverlayFS into the tool' inner OverlayFS) containing the real system and the user's intention. I believe this logic should not be duplicated in client code with calling pushOverlay and initialisation of FileSystem objects all the time.

[ilya-biryukov]

Thanks, this patch raises a very good point. Having a VFS that is not overlayed over RealFS is almost always the wrong thing to do.
On the other hand, I think it's useful to have the client code mention that it overlays over real filesystem, rather than relying on magic inside ClangTool.

I suggest we add a comment hinting that vfs you pass should be overlayed over the real filesystem, unless the client code knows what it's doing.
And provide an easy-to-use helper function to do that.

/// Creates overlayed file system with RealFS at the lowest priority and \p FS after that.
IntrusiveRefCntPtr<vfs::FileSystem> overlayRealFs(IntrusiveRefCntPtr<vfs::FileSystem> FS);

// .....

/// \param FileSystem ..... 
///     Don't pass a vfs that is not overlayed over the RealFS unless you know what you're doing. 
///     Use overlayRealFS helper to easily add the RealFS overlay.

include/clang/Tooling/Tooling.h

299

The convention in this file seems to be not to capitalize the terms, unless they are class names.
E.g. "ClangTool" is capitalized, but "clang tool" is not.
Maybe leave the comment as is?

[whisperity]

• Use an even more explicit way with the documentation requiring that the file system should be an overlay.
• Add a method to easily overlay a FileSystem above the real one.

[whisperity]

Simplify the patch.

[ilya-biryukov]

include/clang/Basic/VirtualFileSystem.h

313

I suggest leaving out the quotes around 'real'

lib/Basic/VirtualFileSystem.cpp

328

I generally agree that it might be useful, but given that we can't use dynamic_cast in LLVM code addressing this FIXME is probably not worth the effort.

And this patch is probably not the right place to add this comment, since it doesn't change OverlayFileSystem in any way.

unittests/Tooling/ToolingTest.cpp

411

Using '/' in paths would probably break on Windows.
Why do we need to add it in the first place?

435

It's not clear what this tests expects when looking at the code
A comment would be helpful.

Also consider matching on an actual error diagnostic (i.e. "<cstdio> not found", right?)

[ilya-biryukov]

include/clang/Basic/VirtualFileSystem.h

315

NIT: I'm not an expert in English, but shouldn't it be createOverlayOverReal.....
Also maybe shorten the suffix: createOverlayOverRealFS?

lib/Basic/VirtualFileSystem.cpp

372

Maybe add an assert the parameter is non-null?

[whisperity]

include/clang/Basic/VirtualFileSystem.h

315

According to this dictionary overlay can mean "to lay on " something. Although above could also work to eliminate saying "over" repeatedly.

unittests/Tooling/ToolingTest.cpp

411

Agreed, this will be removed. It was added because overlaying the real-FS changes the working directory in the memory filesystem too, which in the previous patch (where the overlay was done by the ClangTool constructor) broke where the file resides. It's not applicable anymore.

Although it's strange that saying make check-clang-tooling did NOT execute these tests and said everything passed, I had to run the build ToolingTest binary manually!

435

How can I match on the error message?

[whisperity]

Pinging this as the talk has stalled.

[klimek]

lib/Basic/VirtualFileSystem.cpp

328

We can dyn_cast in LLVM code. It incurs some cost on the class hierarchy, but it's possible.

[whisperity]

Softly pinging this. Perhaps we could discuss this and get it in before Clang7 rides off into the sunset?

include/clang/Basic/VirtualFileSystem.h

public:
   /// \brief Get an iterator pointing to the most recently added file system.
   iterator overlays_begin() { return FSList.rbegin(); }
 
   /// \brief Get an iterator pointing one-past the least recently added file
   /// system.
   iterator overlays_end() { return FSList.rend(); }
 };
 
+/// \brief Creates a \p vfs::OverlayFileSystem which overlays the given file
+/// system above the 'real' file system, as seen by the operating system.
+IntrusiveRefCntPtr<OverlayFileSystem>
+createOverlayOnRealFilesystem(IntrusiveRefCntPtr<FileSystem> TopFS);
+
 namespace detail {
 
 class InMemoryDirectory;
 
 } // namespace detail
 
 /// An in-memory file system.
 class InMemoryFileSystem : public FileSystem {

include/clang/Tooling/Tooling.h

 ///
 /// This class is written to be usable for command line utilities.
 /// By default the class uses ClangSyntaxOnlyAdjuster to modify
 /// command line arguments before the arguments are used to run
 /// a frontend action. One could install an additional command line
 /// arguments adjuster by calling the appendArgumentsAdjuster() method.
 class ClangTool {
 public:
   /// \brief Constructs a clang tool to run over a list of files.
   ///
   /// \param Compilations The CompilationDatabase which contains the compile
   ///        command lines for the given source paths.
   /// \param SourcePaths The source files to run over. If a source files is
   ///        not found in Compilations, it is skipped.
   /// \param PCHContainerOps The PCHContainerOperations for loading and creating
-  /// clang modules.
-  /// \param BaseFS VFS used for all underlying file accesses when running the
-  /// tool.
+  ///        clang modules.
+  /// \param FileSystem The Virtual File System that is used to retrieve file
+  ///        contents by the tool. In most cases, this should be an overlay upon
+  ///        the real file system. (See \p vfs::createOverlayOnRealFilesystem
+  ///        for easily creating an overlay.)
   ClangTool(const CompilationDatabase &Compilations,
             ArrayRef<std::string> SourcePaths,
             std::shared_ptr<PCHContainerOperations> PCHContainerOps =
                 std::make_shared<PCHContainerOperations>(),
-            IntrusiveRefCntPtr<vfs::FileSystem> BaseFS =
+            IntrusiveRefCntPtr<vfs::FileSystem> FileSystem =
                 vfs::getRealFileSystem());
 
   ~ClangTool();
 
   /// \brief Set a \c DiagnosticConsumer to use during parsing.
   void setDiagnosticConsumer(DiagnosticConsumer *DiagConsumer) {
     this->DiagConsumer = DiagConsumer;
   }

lib/Basic/VirtualFileSystem.cpp

 // OverlayFileSystem implementation
 //===-----------------------------------------------------------------------===/
 
 OverlayFileSystem::OverlayFileSystem(IntrusiveRefCntPtr<FileSystem> BaseFS) {
   FSList.push_back(std::move(BaseFS));
 }
 
 void OverlayFileSystem::pushOverlay(IntrusiveRefCntPtr<FileSystem> FS) {
+  // FIXME: OverlayFS containing another one in its stack could be flattened.
   FSList.push_back(FS);
   // Synchronize added file systems by duplicating the working directory from
   // the first one in the list.
   FS->setCurrentWorkingDirectory(getCurrentWorkingDirectory().get());
 }
 
 ErrorOr<Status> OverlayFileSystem::status(const Twine &Path) {
   // FIXME: handle symlinks that cross file systems

 std::error_code
 OverlayFileSystem::setCurrentWorkingDirectory(const Twine &Path) {
   for (auto &FS : FSList)
     if (std::error_code EC = FS->setCurrentWorkingDirectory(Path))
       return EC;
   return {};
 }
 
+IntrusiveRefCntPtr<OverlayFileSystem>
+vfs::createOverlayOnRealFilesystem(IntrusiveRefCntPtr<FileSystem> TopFS) {
+  IntrusiveRefCntPtr<OverlayFileSystem> OverlayFS =
+    new OverlayFileSystem(getRealFileSystem());
+  OverlayFS->pushOverlay(TopFS);
+  return OverlayFS;
+}
+
 clang::vfs::detail::DirIterImpl::~DirIterImpl() = default;
 
 namespace {
 
 class OverlayFSDirIterImpl : public clang::vfs::detail::DirIterImpl {
   OverlayFileSystem &Overlays;
   std::string Path;
   OverlayFileSystem::iterator CurrentFS;

lib/Tooling/Tooling.cpp

bool FrontendActionFactory::runInvocation(
 
   Files->clearStatCaches();
   return Success;
 }
 
 ClangTool::ClangTool(const CompilationDatabase &Compilations,
                      ArrayRef<std::string> SourcePaths,
                      std::shared_ptr<PCHContainerOperations> PCHContainerOps,
-                     IntrusiveRefCntPtr<vfs::FileSystem> BaseFS)
+                     IntrusiveRefCntPtr<vfs::FileSystem> FileSystem)
     : Compilations(Compilations), SourcePaths(SourcePaths),
       PCHContainerOps(std::move(PCHContainerOps)),
-      OverlayFileSystem(new vfs::OverlayFileSystem(std::move(BaseFS))),
+      OverlayFileSystem(new vfs::OverlayFileSystem(std::move(FileSystem))),
       InMemoryFileSystem(new vfs::InMemoryFileSystem),
       Files(new FileManager(FileSystemOptions(), OverlayFileSystem)) {
   OverlayFileSystem->pushOverlay(InMemoryFileSystem);
   appendArgumentsAdjuster(getClangStripOutputAdjuster());
   appendArgumentsAdjuster(getClangSyntaxOnlyAdjuster());
   appendArgumentsAdjuster(getClangStripDependencyFileAdjuster());
 }
 

unittests/Tooling/ToolingTest.cpp

TEST(ClangToolTest, ArgumentAdjusters) {
   Tool.clearArgumentsAdjusters();
   Tool.appendArgumentsAdjuster(CheckSyntaxOnlyAdjuster);
   Tool.appendArgumentsAdjuster(getClangSyntaxOnlyAdjuster());
   Tool.run(Action.get());
   EXPECT_TRUE(Ran);
   EXPECT_FALSE(Found);
 }
 
-TEST(ClangToolTest, BaseVirtualFileSystemUsage) {
+TEST(ClangToolVFSTest, VirtualFileSystemUsage) {
   FixedCompilationDatabase Compilations("/", std::vector<std::string>());
-  llvm::IntrusiveRefCntPtr<vfs::OverlayFileSystem> OverlayFileSystem(
-      new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
   llvm::IntrusiveRefCntPtr<vfs::InMemoryFileSystem> InMemoryFileSystem(
       new vfs::InMemoryFileSystem);
-  OverlayFileSystem->pushOverlay(InMemoryFileSystem);
 
   InMemoryFileSystem->addFile(
-      "a.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int main() {}"));
+      "/a.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int main() {}"));
 
-  ClangTool Tool(Compilations, std::vector<std::string>(1, "a.cpp"),
-                 std::make_shared<PCHContainerOperations>(), OverlayFileSystem);
+  ClangTool Tool(Compilations, std::vector<std::string>(1, "/a.cpp"),
+                 std::make_shared<PCHContainerOperations>(),
+                 InMemoryFileSystem);
   std::unique_ptr<FrontendActionFactory> Action(
       newFrontendActionFactory<SyntaxOnlyAction>());
   EXPECT_EQ(0, Tool.run(Action.get()));
 }
 
+TEST(ClangToolVFSTest, VFSDoesntContainEveryFile) {
+  FixedCompilationDatabase Compilations("/", std::vector<std::string>());
+  llvm::IntrusiveRefCntPtr<vfs::InMemoryFileSystem> InMemoryFileSystem(
+      new vfs::InMemoryFileSystem);
+
+  InMemoryFileSystem->addFile(
+      "/a.cpp", 0, llvm::MemoryBuffer::getMemBuffer("#include <cstdio>\n"
+                                                    "int main() {}"));
+
+  ClangTool Tool(Compilations, std::vector<std::string>(1, "/a.cpp"),
+                 std::make_shared<PCHContainerOperations>(),
+                 InMemoryFileSystem);
+  std::unique_ptr<FrontendActionFactory> Action(
+      newFrontendActionFactory<SyntaxOnlyAction>());
+  EXPECT_NE(0, Tool.run(Action.get()));
+}
+
 // Check getClangStripDependencyFileAdjuster doesn't strip args after -MD/-MMD.
 TEST(ClangToolTest, StripDependencyFileAdjuster) {
   FixedCompilationDatabase Compilations("/", {"-MD", "-c", "-MMD", "-w"});
 
   ClangTool Tool(Compilations, std::vector<std::string>(1, "/a.cc"));
   Tool.mapVirtualFile("/a.cc", "void a() {}");
 
   std::unique_ptr<FrontendActionFactory> Action(