[clang] Don't silently inherit the VFS from FileManager
#164323
+21
−11
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jansvoboda11
requested review from
JDevlieghere and
cyndyishida
as code owners
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-modules @llvm/pr-subscribers-clang-static-analyzer-1 Author: Jan Svoboda (jansvoboda11) ChangesSince #158381 the Full diff: https://github.com/llvm/llvm-project/pull/164323.diff 14 Files Affected:
diff --git a/clang-tools-extra/clang-include-fixer/IncludeFixer.cpp b/clang-tools-extra/clang-include-fixer/IncludeFixer.cpp
index e825547ba0134..799e02ffaf3af 100644
--- a/clang-tools-extra/clang-include-fixer/IncludeFixer.cpp
+++ b/clang-tools-extra/clang-include-fixer/IncludeFixer.cpp
@@ -90,6 +90,7 @@ bool IncludeFixerActionFactory::runInvocation(
// Set up Clang.
CompilerInstance Compiler(std::move(Invocation), std::move(PCHContainerOps));
+ Compiler.setVirtualFileSystem(Files->getVirtualFileSystemPtr());
Compiler.setFileManager(Files);
// Create the compiler's actual diagnostics engine. We want to drop all
diff --git a/clang/include/clang/Frontend/ASTUnit.h b/clang/include/clang/Frontend/ASTUnit.h
index f66df89aad904..3cea159afa33c 100644
--- a/clang/include/clang/Frontend/ASTUnit.h
+++ b/clang/include/clang/Frontend/ASTUnit.h
@@ -499,6 +499,11 @@ class ASTUnit {
return *PPOpts;
}
+ IntrusiveRefCntPtr<llvm::vfs::FileSystem> getVirtualFileSystemPtr() {
+ // FIXME: Don't defer VFS ownership to the FileManager.
+ return FileMgr->getVirtualFileSystemPtr();
+ }
+
const FileManager &getFileManager() const { return *FileMgr; }
FileManager &getFileManager() { return *FileMgr; }
IntrusiveRefCntPtr<FileManager> getFileManagerPtr() { return FileMgr; }
diff --git a/clang/include/clang/Frontend/CompilerInstance.h b/clang/include/clang/Frontend/CompilerInstance.h
index 44fff69c217c5..2403cbbb652dd 100644
--- a/clang/include/clang/Frontend/CompilerInstance.h
+++ b/clang/include/clang/Frontend/CompilerInstance.h
@@ -460,7 +460,7 @@ class CompilerInstance : public ModuleLoader {
FileMgr.resetWithoutRelease();
}
- /// Replace the current file manager and virtual file system.
+ /// Replace the current file manager.
void setFileManager(IntrusiveRefCntPtr<FileManager> Value);
/// @}
diff --git a/clang/lib/Frontend/ASTUnit.cpp b/clang/lib/Frontend/ASTUnit.cpp
index cb445682ac48b..d53b64a414ddc 100644
--- a/clang/lib/Frontend/ASTUnit.cpp
+++ b/clang/lib/Frontend/ASTUnit.cpp
@@ -1651,6 +1651,7 @@ ASTUnit *ASTUnit::LoadFromCompilerInvocationAction(
AST->Reader = nullptr;
// Create a file manager object to provide access to and cache the filesystem.
+ Clang->setVirtualFileSystem(AST->getVirtualFileSystemPtr());
Clang->setFileManager(AST->getFileManagerPtr());
// Create the source manager.
@@ -2290,6 +2291,7 @@ void ASTUnit::CodeComplete(
"IR inputs not support here!");
// Use the source and file managers that we were given.
+ Clang->setVirtualFileSystem(FileMgr->getVirtualFileSystemPtr());
Clang->setFileManager(FileMgr);
Clang->setSourceManager(SourceMgr);
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index 584436665622d..374138fe4cf8f 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -160,8 +160,6 @@ bool CompilerInstance::createTarget() {
}
void CompilerInstance::setFileManager(IntrusiveRefCntPtr<FileManager> Value) {
- if (!hasVirtualFileSystem())
- setVirtualFileSystem(Value->getVirtualFileSystemPtr());
assert(Value == nullptr ||
getVirtualFileSystemPtr() == Value->getVirtualFileSystemPtr());
FileMgr = std::move(Value);
diff --git a/clang/lib/Frontend/FrontendAction.cpp b/clang/lib/Frontend/FrontendAction.cpp
index 1b63c40a6efd7..b61bcf0a62b8f 100644
--- a/clang/lib/Frontend/FrontendAction.cpp
+++ b/clang/lib/Frontend/FrontendAction.cpp
@@ -945,6 +945,7 @@ bool FrontendAction::BeginSourceFile(CompilerInstance &CI,
// Set the shared objects, these are reset when we finish processing the
// file, otherwise the CompilerInstance will happily destroy them.
+ CI.setVirtualFileSystem(AST->getVirtualFileSystemPtr());
CI.setFileManager(AST->getFileManagerPtr());
CI.setSourceManager(AST->getSourceManagerPtr());
CI.setPreprocessor(AST->getPreprocessorPtr());
diff --git a/clang/lib/Frontend/PrecompiledPreamble.cpp b/clang/lib/Frontend/PrecompiledPreamble.cpp
index 03f70b74dfb42..9bf18b4d897ce 100644
--- a/clang/lib/Frontend/PrecompiledPreamble.cpp
+++ b/clang/lib/Frontend/PrecompiledPreamble.cpp
@@ -479,16 +479,12 @@ llvm::ErrorOr<PrecompiledPreamble> PrecompiledPreamble::Build(
Diagnostics->Reset();
ProcessWarningOptions(*Diagnostics, Clang->getDiagnosticOpts(), *VFS);
- VFS = createVFSFromCompilerInvocation(Clang->getInvocation(), *Diagnostics,
- VFS);
-
// Create a file manager object to provide access to and cache the filesystem.
- Clang->setFileManager(
- llvm::makeIntrusiveRefCnt<FileManager>(Clang->getFileSystemOpts(), VFS));
+ Clang->createVirtualFileSystem(VFS);
+ Clang->createFileManager();
// Create the source manager.
- Clang->setSourceManager(llvm::makeIntrusiveRefCnt<SourceManager>(
- *Diagnostics, Clang->getFileManager()));
+ Clang->createSourceManager();
auto PreambleDepCollector = std::make_shared<PreambleDependencyCollector>();
Clang->addDependencyCollector(PreambleDepCollector);
diff --git a/clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp b/clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
index 5301f88057203..531c642bf4f31 100644
--- a/clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
+++ b/clang/lib/StaticAnalyzer/Frontend/ModelInjector.cpp
@@ -93,6 +93,7 @@ void ModelInjector::onBodySynthesis(const NamedDecl *D) {
// The instance wants to take ownership, however DisableFree frontend option
// is set to true to avoid double free issues
+ Instance.setVirtualFileSystem(CI.getVirtualFileSystemPtr());
Instance.setFileManager(CI.getFileManagerPtr());
Instance.setSourceManager(SM);
Instance.setPreprocessor(CI.getPreprocessorPtr());
diff --git a/clang/lib/Tooling/Tooling.cpp b/clang/lib/Tooling/Tooling.cpp
index ea5a37216e959..e8eef5ed9c9fa 100644
--- a/clang/lib/Tooling/Tooling.cpp
+++ b/clang/lib/Tooling/Tooling.cpp
@@ -446,6 +446,7 @@ bool FrontendActionFactory::runInvocation(
DiagnosticConsumer *DiagConsumer) {
// Create a compiler instance to handle the actual work.
CompilerInstance Compiler(std::move(Invocation), std::move(PCHContainerOps));
+ Compiler.setVirtualFileSystem(Files->getVirtualFileSystemPtr());
Compiler.setFileManager(Files);
// The FrontendAction can have lifetime requirements for Compiler or its
diff --git a/clang/tools/clang-installapi/ClangInstallAPI.cpp b/clang/tools/clang-installapi/ClangInstallAPI.cpp
index 16abeb10284c0..4e66485343b89 100644
--- a/clang/tools/clang-installapi/ClangInstallAPI.cpp
+++ b/clang/tools/clang-installapi/ClangInstallAPI.cpp
@@ -114,6 +114,7 @@ static bool run(ArrayRef<const char *> Args, const char *ProgName) {
// Set up compilation.
std::unique_ptr<CompilerInstance> CI(new CompilerInstance());
+ CI->setVirtualFileSystem(FM->getVirtualFileSystemPtr());
CI->setFileManager(FM);
CI->createDiagnostics();
if (!CI->hasDiagnostics())
diff --git a/clang/unittests/Tooling/DependencyScanning/DependencyScannerTest.cpp b/clang/unittests/Tooling/DependencyScanning/DependencyScannerTest.cpp
index aa32bb3d39f6d..4523af33e3c28 100644
--- a/clang/unittests/Tooling/DependencyScanning/DependencyScannerTest.cpp
+++ b/clang/unittests/Tooling/DependencyScanning/DependencyScannerTest.cpp
@@ -49,6 +49,8 @@ class TestFileCollector : public DependencyFileGenerator {
std::vector<std::string> &Deps;
};
+// FIXME: Use the regular Service/Worker/Collector APIs instead of
+// reimplementing the action.
class TestDependencyScanningAction : public tooling::ToolAction {
public:
TestDependencyScanningAction(std::vector<std::string> &Deps) : Deps(Deps) {}
@@ -59,6 +61,7 @@ class TestDependencyScanningAction : public tooling::ToolAction {
DiagnosticConsumer *DiagConsumer) override {
CompilerInstance Compiler(std::move(Invocation),
std::move(PCHContainerOps));
+ Compiler.setVirtualFileSystem(FileMgr->getVirtualFileSystemPtr());
Compiler.setFileManager(FileMgr);
Compiler.createDiagnostics(DiagConsumer, /*ShouldOwnClient=*/false);
diff --git a/clang/unittests/Tooling/Syntax/TokensTest.cpp b/clang/unittests/Tooling/Syntax/TokensTest.cpp
index 6094177e4817b..47184cbf5d768 100644
--- a/clang/unittests/Tooling/Syntax/TokensTest.cpp
+++ b/clang/unittests/Tooling/Syntax/TokensTest.cpp
@@ -134,6 +134,7 @@ class TokenCollectorTest : public ::testing::Test {
FileName, llvm::MemoryBuffer::getMemBufferCopy(Code).release());
CompilerInstance Compiler(std::move(CI));
Compiler.setDiagnostics(Diags);
+ Compiler.setVirtualFileSystem(FS);
Compiler.setFileManager(FileMgr);
Compiler.setSourceManager(SourceMgr);
diff --git a/clang/unittests/Tooling/Syntax/TreeTestBase.cpp b/clang/unittests/Tooling/Syntax/TreeTestBase.cpp
index 400a0d5a1801b..b2be64fc08f3d 100644
--- a/clang/unittests/Tooling/Syntax/TreeTestBase.cpp
+++ b/clang/unittests/Tooling/Syntax/TreeTestBase.cpp
@@ -153,6 +153,7 @@ SyntaxTreeTest::buildTree(StringRef Code, const TestClangConfig &ClangConfig) {
FileName, llvm::MemoryBuffer::getMemBufferCopy(Code).release());
CompilerInstance Compiler(Invocation);
Compiler.setDiagnostics(Diags);
+ Compiler.setVirtualFileSystem(FS);
Compiler.setFileManager(FileMgr);
Compiler.setSourceManager(SourceMgr);
diff --git a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
index 6b121c934dfbb..990074566be7e 100644
--- a/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ b/lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -754,7 +754,6 @@ ClangExpressionParser::ClangExpressionParser(
// Make sure clang uses the same VFS as LLDB.
m_compiler->setVirtualFileSystem(
FileSystem::Instance().GetVirtualFileSystem());
- m_compiler->createFileManager();
// 2. Configure the compiler with a set of default options that are
// appropriate for most situations.
|
jansvoboda11
commented
Oct 20, 2025
| @@ -160,8 +160,6 @@ bool CompilerInstance::createTarget() { | |||
| } | |||
|
|
|||
| void CompilerInstance::setFileManager(IntrusiveRefCntPtr<FileManager> Value) { | |||
| if (!hasVirtualFileSystem()) | |||
| setVirtualFileSystem(Value->getVirtualFileSystemPtr()); | |||
There was a problem hiding this comment.
This is a possible null dereference.
jansvoboda11
commented
Oct 20, 2025
| @@ -945,6 +945,7 @@ bool FrontendAction::BeginSourceFile(CompilerInstance &CI, | |||
|
|
|||
| // Set the shared objects, these are reset when we finish processing the | |||
| // file, otherwise the CompilerInstance will happily destroy them. | |||
| CI.setVirtualFileSystem(AST->getVirtualFileSystemPtr()); | |||
There was a problem hiding this comment.
Although CI.getVirtualFileSystemPtr() is being passed into ASTUnit::LoadFromASTFile(), the AST may end up being configured with a different VFS compared to CI which trips the assertion in CompilerInstance::setFileManager(). This is a good opportunity to fix that.
jansvoboda11
commented
Oct 20, 2025
| @@ -754,7 +754,6 @@ ClangExpressionParser::ClangExpressionParser( | |||
| // Make sure clang uses the same VFS as LLDB. | |||
| m_compiler->setVirtualFileSystem( | |||
| FileSystem::Instance().GetVirtualFileSystem()); | |||
| m_compiler->createFileManager(); | |||
There was a problem hiding this comment.
This is redundant with line 825.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Since #158381 the
CompilerInstanceis aware of the VFS and co-owns it. To reduce scope of that PR, the VFS was being inherited from theFileManagerduringsetFileManager()if it wasn't configured before. However, the implementation of that setter was buggy. This PR fixes the bug, and moves us closer to the long-term goal ofCompilerInstancerequiring the VFS to be configured explicitly and owned by the instance.