[PATCH] D41594: Support `ivfsoverlay` option in Tooling

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

With https://reviews.llvm.org/D41947 in place, do we still need this change? Or 
can it be "abandoned" now?


Repository:
  rC Clang

https://reviews.llvm.org/D41594



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41594: Support `ivfsoverlay` option in Tooling

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: lib/Tooling/Tooling.cpp:287
+  if (Files->getVirtualFileSystem() != VirtualFileSystem) {
+Files = new FileManager(Invocation->getFileSystemOpts(), 
VirtualFileSystem);
+  }

vladimir.plyashkun wrote:
> ilya-biryukov wrote:
> > `Files` is a raw pointer, so we're leaking memory here.
> > 
> Agree.
> I can try to replace type of this field to `llvm::IntrusiveRefCntPtr`  
> instead of raw pointer.
> But if i understand correctly, this class is available during whole 
> execution, so memory will be freed on exit. 
> I saw some other usages, for example, `createFileManager` method in the 
> `CompilerInstance` also just reassign value to raw pointer.
> https://clang.llvm.org/doxygen/classclang_1_1CompilerInstance.html#abeb2bbf46a8de987c227125a84935802
Using `IntrusiveRefCntPtr` locally should do the trick, the 
clients can take ownership if they want and `FileManager` will be properly 
freed if they don't do that.

`CompilerInstance::createFileManager` stores `IntrusiveRefCntPtr` as a field 
before returning a raw pointer, so it seems to properly manage memory there.


Repository:
  rC Clang

https://reviews.llvm.org/D41594



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41594: Support `ivfsoverlay` option in Tooling

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun added inline comments.



Comment at: lib/Tooling/Tooling.cpp:287
+  if (Files->getVirtualFileSystem() != VirtualFileSystem) {
+Files = new FileManager(Invocation->getFileSystemOpts(), 
VirtualFileSystem);
+  }

ilya-biryukov wrote:
> `Files` is a raw pointer, so we're leaking memory here.
> 
Agree.
I can try to replace type of this field to `llvm::IntrusiveRefCntPtr`  
instead of raw pointer.
But if i understand correctly, this class is available during whole execution, 
so memory will be freed on exit. 
I saw some other usages, for example, `createFileManager` method in the 
`CompilerInstance` also just reassign value to raw pointer.
https://clang.llvm.org/doxygen/classclang_1_1CompilerInstance.html#abeb2bbf46a8de987c227125a84935802


Repository:
  rC Clang

https://reviews.llvm.org/D41594



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41594: Support `ivfsoverlay` option in Tooling

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

I'm not sure if it's ok to ignore the shared `FileManager` here.
@klimek, @bkramer, @alexfh, does tooling library rely on having the same 
`FileManager` for all invocations? Is it just a performance optimization or 
there's more to it?




Comment at: lib/Tooling/Tooling.cpp:287
+  if (Files->getVirtualFileSystem() != VirtualFileSystem) {
+Files = new FileManager(Invocation->getFileSystemOpts(), 
VirtualFileSystem);
+  }

`Files` is a raw pointer, so we're leaking memory here.



Repository:
  rC Clang

https://reviews.llvm.org/D41594



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41594: Support `ivfsoverlay` option in Tooling

[Vladimir Plyashkun via Phabricator via cfe-commits]

vladimir.plyashkun created this revision.
vladimir.plyashkun added reviewers: bkramer, alexfh, ilya-biryukov.
vladimir.plyashkun added a project: clang.
Herald added a subscriber: klimek.

Previously, this argument had no effect, since it didn't proceeded.
For more information, check this review: https://reviews.llvm.org/D41535


Repository:
  rC Clang

https://reviews.llvm.org/D41594

Files:
  lib/Frontend/CompilerInvocation.cpp
  lib/Tooling/Tooling.cpp
  unittests/Tooling/ToolingTest.cpp


Index: lib/Tooling/Tooling.cpp
===
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -280,6 +280,12 @@
 Invocation->getPreprocessorOpts().addRemappedFile(It.getKey(),
   Input.release());
   }
+  IntrusiveRefCntPtr VirtualFileSystem =
+  createVFSFromCompilerInvocation(*Invocation, Diagnostics,
+  Files->getVirtualFileSystem());
+  if (Files->getVirtualFileSystem() != VirtualFileSystem) {
+Files = new FileManager(Invocation->getFileSystemOpts(), 
VirtualFileSystem);
+  }
   return runInvocation(BinaryName, Compilation.get(), std::move(Invocation),
std::move(PCHContainerOps));
 }
Index: unittests/Tooling/ToolingTest.cpp
===
--- unittests/Tooling/ToolingTest.cpp
+++ unittests/Tooling/ToolingTest.cpp
@@ -21,6 +21,7 @@
 #include "llvm/Support/Path.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/TargetSelect.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "gtest/gtest.h"
 #include 
 #include 
@@ -171,6 +172,37 @@
   EXPECT_TRUE(Invocation.run());
 }
 
+TEST(ToolInvocation, TestVfsOverlay) {
+  llvm::IntrusiveRefCntPtr OverlayFileSystem(
+  new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
+  llvm::IntrusiveRefCntPtr InMemoryFileSystem(
+  new vfs::InMemoryFileSystem);
+  OverlayFileSystem->pushOverlay(InMemoryFileSystem);
+  llvm::IntrusiveRefCntPtr Files(
+  new FileManager(FileSystemOptions(), OverlayFileSystem));
+  std::vector Args;
+  Args.push_back("tool-executable");
+  Args.push_back("-ivfsoverlay");
+  Args.push_back("overlay.yaml");
+  Args.push_back("-fsyntax-only");
+  Args.push_back("a.cpp");
+  clang::tooling::ToolInvocation Invocation(Args, new SyntaxOnlyAction,
+Files.get());
+  InMemoryFileSystem->addFile(
+  "a.cpp", 0,
+  llvm::MemoryBuffer::getMemBuffer("#include \n"));
+  std::string OverlayContent = llvm::formatv(
+  "{{'version': 0,'roots':[{{'name':'{0}','type':'directory',"
+ "'contents':[{{'name':'a.cpp','type':'file',"
+ "'external-contents':'b.cpp'}]}]}",
+  InMemoryFileSystem.get()->getCurrentWorkingDirectory().get());
+  InMemoryFileSystem->addFile("overlay.yaml", 0,
+  
llvm::MemoryBuffer::getMemBuffer(OverlayContent));
+  InMemoryFileSystem->addFile(
+  "b.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int main() {}"));
+  EXPECT_TRUE(Invocation.run());
+}
+
 TEST(ToolInvocation, TestVirtualModulesCompilation) {
   // FIXME: Currently, this only tests that we don't exit with an error if a
   // mapped module.map is found on the include path. In the future, expand this
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -2988,8 +2988,9 @@
   return IntrusiveRefCntPtr();
 }
 
-IntrusiveRefCntPtr FS = vfs::getVFSFromYAML(
-std::move(Buffer.get()), /*DiagHandler*/ nullptr, File);
+IntrusiveRefCntPtr FS =
+vfs::getVFSFromYAML(std::move(Buffer.get()), /*DiagHandler*/ nullptr,
+File, /*DiagContext*/ nullptr, BaseFS);
 if (!FS.get()) {
   Diags.Report(diag::err_invalid_vfs_overlay) << File;
   return IntrusiveRefCntPtr();


Index: lib/Tooling/Tooling.cpp
===
--- lib/Tooling/Tooling.cpp
+++ lib/Tooling/Tooling.cpp
@@ -280,6 +280,12 @@
 Invocation->getPreprocessorOpts().addRemappedFile(It.getKey(),
   Input.release());
   }
+  IntrusiveRefCntPtr VirtualFileSystem =
+  createVFSFromCompilerInvocation(*Invocation, Diagnostics,
+  Files->getVirtualFileSystem());
+  if (Files->getVirtualFileSystem() != VirtualFileSystem) {
+Files = new FileManager(Invocation->getFileSystemOpts(), VirtualFileSystem);
+  }
   return runInvocation(BinaryName, Compilation.get(), std::move(Invocation),
std::move(PCHContainerOps));
 }
Index: unittests/Tooling/ToolingTest.cpp
===
--- unittests/Tooling/ToolingTest.cpp
+++ unittests/Tooling/ToolingTest.cpp
@@ -21,6 +21,7