[clangd] [Modules] Use ASTReader directly in IsModuleFileUpToDate #113879

ChuanqiXu9 · 2024-10-28T08:03:10Z

@kadircet mentioned in 448d8fa#diff-fb3ba8a781117ff04736f951a274812cb7ad1678f9d71d4d91870b711ab45da0L285 that:

this is definitely a functional change, clangd is used in environments that solely relies on VFS, and doesn't depend on ASTUnit at all.

right now this is both introducing a dependency on ASTUnit, and making all the logical IO physical instead. can you instead use the regular compiler utilities in clangd, and get the astreader from CompilerInstance directly, which is VFS-aware, and doesn't depend on ASTUnit ?

This tries to resolve the problem by creating ASTReader directly and use VFS to create the FileManager.

// Original comments

But I like ASTUnit since I feel it is a better abstract here. For compiler instance, we need to provide the compiler invocation. But for the purpose here. we're just trying to open the BMI file to verify if the source files are out of date or not. We don't need compiler invocation here completely. So I prefer ASTUnit here especially clangd already depends on clangFrontend already so I feel like I didn't introduce a new dependency.

Then I added the VFS argument to ASTUnit to address the Virtual/Physical FS problem.

llvmbot · 2024-10-28T08:03:46Z

@llvm/pr-subscribers-clangd

@llvm/pr-subscribers-clang-tools-extra

Author: Chuanqi Xu (ChuanqiXu9)

Changes

@kadircet mentioned in 448d8fa#diff-fb3ba8a781117ff04736f951a274812cb7ad1678f9d71d4d91870b711ab45da0L285 that:

> this is definitely a functional change, clangd is used in environments that solely relies on VFS, and doesn't depend on ASTUnit at all.

> right now this is both introducing a dependency on ASTUnit, and making all the logical IO physical instead. can you instead use the regular compiler utilities in clangd, and get the astreader from CompilerInstance directly, which is VFS-aware, and doesn't depend on ASTUnit ?

But I like ASTUnit since I feel it is a better abstract here. For compiler instance, we need to provide the compiler invocation. But for the purpose here. we're just trying to open the BMI file to verify if the source files are out of date or not. We don't need compiler invocation here completely. So I prefer ASTUnit here especially clangd already depends on clangFrontend already so I feel like I didn't introduce a new dependency.

Then I added the VFS argument to ASTUnit to address the Virtual/Physical FS problem.

Full diff: https://github.com/llvm/llvm-project/pull/113879.diff

1 Files Affected:

(modified) clang-tools-extra/clangd/ModulesBuilder.cpp (+16-10)

diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp
index 1eeff468ef1236..25f9ab6631fca7 100644
--- a/clang-tools-extra/clangd/ModulesBuilder.cpp
+++ b/clang-tools-extra/clangd/ModulesBuilder.cpp
@@ -127,10 +127,10 @@ struct ModuleFile {
   std::string ModuleFilePath;
 };
 
-bool IsModuleFileUpToDate(
-    PathRef ModuleFilePath,
-    const PrerequisiteModules &RequisiteModules) {
-IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
+bool IsModuleFileUpToDate(PathRef ModuleFilePath,
+                          const PrerequisiteModules &RequisiteModules,
+                          llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS) {
+  IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
       CompilerInstance::createDiagnostics(new DiagnosticOptions());
 
   auto HSOpts = std::make_shared<HeaderSearchOptions>();
@@ -141,7 +141,11 @@ IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
   PCHContainerOperations PCHOperations;
   std::unique_ptr<ASTUnit> Unit = ASTUnit::LoadFromASTFile(
       ModuleFilePath.str(), PCHOperations.getRawReader(), ASTUnit::LoadASTOnly,
-      Diags, FileSystemOptions(), std::move(HSOpts));
+      Diags, FileSystemOptions(), std::move(HSOpts),
+      /*LangOpts=*/nullptr, /*OnlyLocalDecls=*/false,
+      /*CaptureDiagnostics=*/CaptureDiagsKind::None,
+      /*AllowASTWithCompilerErrors=*/false, /*UserFilesAreVolatile=*/false,
+      VFS);
 
   if (!Unit)
     return false;
@@ -167,10 +171,12 @@ IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
 
 bool IsModuleFilesUpToDate(
     llvm::SmallVector<PathRef> ModuleFilePaths,
-    const PrerequisiteModules &RequisiteModules) {
-  return llvm::all_of(ModuleFilePaths, [&RequisiteModules](auto ModuleFilePath) {
-    return IsModuleFileUpToDate(ModuleFilePath, RequisiteModules);
-  });
+    const PrerequisiteModules &RequisiteModules,
+    llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS) {
+  return llvm::all_of(
+      ModuleFilePaths, [&RequisiteModules, VFS](auto ModuleFilePath) {
+        return IsModuleFileUpToDate(ModuleFilePath, RequisiteModules, VFS);
+      });
 }
 
 // StandalonePrerequisiteModules - stands for PrerequisiteModules for which all
@@ -347,7 +353,7 @@ bool StandalonePrerequisiteModules::canReuse(
   SmallVector<StringRef> BMIPaths;
   for (auto &MF : RequiredModules)
     BMIPaths.push_back(MF.ModuleFilePath);
-  return IsModuleFilesUpToDate(BMIPaths, *this);
+  return IsModuleFilesUpToDate(BMIPaths, *this, VFS);
 }
 
 } // namespace clangd

kadircet · 2024-10-28T08:29:00Z

So I prefer ASTUnit here especially clangd already depends on clangFrontend already so I feel like I didn't introduce a new dependency.

It isn't at all about having this dependency in terms of "build graph", but rather about mental complexity and maintenance burden of that.

Clangd already has its own primitives for dealing with preambles, keeping ASTcontexts alive and even handling modules (and soon also module caches!). ASTUnit has its own way of doing all of these. So if we introduced any semantic dependency here, everytime someone is chaging those core primitives in clangd will need to think about how all of these going to interact with each other.

Moreover, if this was sent with a review, I would've surely objected to that.

For compiler instance, we need to provide the compiler invocation

We already have the compiler invocation available. I am not sure about the implications of loading a PCM with a "vanilla" compiler instance (that's what ASTUnit does underneath, creates a preprocessor, astcontext, diagsengine, ... with default options), I'd be much more comfortable if we created that ASTReader and loaded the AST with some relevant compilerinvocation that describes user's intent.

But you definitely know much more about modules, if you think that's fine, i think we can always create that astreader directly here, without a compilerinstance as well.

kadircet

(need to write something here apparently)

ChuanqiXu9 · 2024-10-28T08:56:14Z

(Update too quickly)

github-actions · 2024-10-28T08:58:36Z

✅ With the latest revision this PR passed the C/C++ code formatter.

clang-tools-extra/clangd/ModulesBuilder.cpp

kadircet · 2024-10-28T09:21:32Z

clang-tools-extra/clangd/ModulesBuilder.cpp

    return false;

-  auto Reader = Unit->getASTReader();
+  auto Reader = Clang.getASTReader();


i think we should call Clang.createASTReader() first. I don't see any of the previous call creating that.

ChuanqiXu9 · 2024-10-28T11:03:07Z

if you think that's fine, i think we can always create that astreader directly here, without a compilerinstance as well.

I like the idea more. (so I typed 'updated too quickly') The most important reason I did the previous change is, I think the compiler invocation and compiler instance are not needed to check if source files recorded in BMI are out dated or not. They are fake parameters here. I feel they are redundant and want to get rid of that. And now you pointed out that, actually ASTUnit may be redundant too and what we need actually is the ASTReader only. I like the idea. (Although it introduces the codes a little bit longer)

kadircet

thanks!

clang-tools-extra/clangd/ModulesBuilder.cpp

kadircet · 2024-10-29T09:57:37Z

clang-tools-extra/clangd/ModulesBuilder.cpp


-  if (!Unit)
-    return false;
+  Reader.setListener(std::make_unique<PPIntializer>(PP));


do we really need a Listener? and where's PPIntializer defined?

My bad. It was not updated. I guess I had a typo when verify it by typing check-clangd into check-clang : (

kadircet

thanks, lgtm!

ChuanqiXu9 · 2024-10-31T01:31:54Z

Thanks : )

@kadircet

…vm#113879) @kadircet mentioned in llvm@448d8fa#diff-fb3ba8a781117ff04736f951a274812cb7ad1678f9d71d4d91870b711ab45da0L285 that: > this is definitely a functional change, clangd is used in environments that solely relies on VFS, and doesn't depend on ASTUnit at all. > right now this is both introducing a dependency on ASTUnit, and making all the logical IO physical instead. can you instead use the regular compiler utilities in clangd, and get the astreader from CompilerInstance directly, which is VFS-aware, and doesn't depend on ASTUnit ? This tries to resolve the problem by creating ASTReader directly and use VFS to create the FileManager.

@kadircet

…vm#113879) @kadircet mentioned in llvm@448d8fa#diff-fb3ba8a781117ff04736f951a274812cb7ad1678f9d71d4d91870b711ab45da0L285 that: > this is definitely a functional change, clangd is used in environments that solely relies on VFS, and doesn't depend on ASTUnit at all. > right now this is both introducing a dependency on ASTUnit, and making all the logical IO physical instead. can you instead use the regular compiler utilities in clangd, and get the astreader from CompilerInstance directly, which is VFS-aware, and doesn't depend on ASTUnit ? This tries to resolve the problem by creating ASTReader directly and use VFS to create the FileManager.

ChuanqiXu9 requested a review from kadircet October 28, 2024 08:03

llvmbot added clang-tools-extra clangd labels Oct 28, 2024

kadircet requested changes Oct 28, 2024

View reviewed changes

[clangd] [Modules] Use ASTReader directly in IsModuleFileUpToDate

99e1989

ChuanqiXu9 force-pushed the ClangdModulesVFS branch from b6abb73 to 99e1989 Compare October 28, 2024 10:58

ChuanqiXu9 changed the title ~~[clangd] [Modules] Add VFS to ASTUnit::LoadFromASTFile~~ [clangd] [Modules] Use ASTReader directly in IsModuleFileUpToDate Oct 28, 2024

kadircet requested changes Oct 28, 2024

View reviewed changes

clang-tools-extra/clangd/ModulesBuilder.cpp Show resolved Hide resolved

clang-tools-extra/clangd/ModulesBuilder.cpp Show resolved Hide resolved

clang-tools-extra/clangd/ModulesBuilder.cpp Outdated Show resolved Hide resolved

Update

61a2c9a

kadircet requested changes Oct 29, 2024

View reviewed changes

ChuanqiXu9 added 2 commits October 30, 2024 10:34

update

0ffb403

fmt

2261028

kadircet approved these changes Oct 30, 2024

View reviewed changes

ChuanqiXu9 merged commit e9b7fe8 into llvm:main Oct 31, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clangd] [Modules] Use ASTReader directly in IsModuleFileUpToDate #113879

[clangd] [Modules] Use ASTReader directly in IsModuleFileUpToDate #113879

Uh oh!

ChuanqiXu9 commented Oct 28, 2024 •

edited

Loading

Uh oh!

llvmbot commented Oct 28, 2024 •

edited

Loading

Uh oh!

kadircet commented Oct 28, 2024

Uh oh!

kadircet left a comment

Uh oh!

ChuanqiXu9 commented Oct 28, 2024

Uh oh!

github-actions bot commented Oct 28, 2024 •

edited

Loading

Uh oh!

Uh oh!

kadircet Oct 28, 2024

Uh oh!

ChuanqiXu9 commented Oct 28, 2024 •

edited

Loading

Uh oh!

kadircet left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kadircet Oct 29, 2024

Uh oh!

ChuanqiXu9 Oct 30, 2024

Uh oh!

kadircet left a comment

Uh oh!

ChuanqiXu9 commented Oct 31, 2024

Uh oh!

Uh oh!

Uh oh!

[clangd] [Modules] Use ASTReader directly in IsModuleFileUpToDate #113879

[clangd] [Modules] Use ASTReader directly in IsModuleFileUpToDate #113879

Uh oh!

Conversation

ChuanqiXu9 commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kadircet commented Oct 28, 2024

Uh oh!

kadircet left a comment

Choose a reason for hiding this comment

Uh oh!

ChuanqiXu9 commented Oct 28, 2024

Uh oh!

github-actions bot commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

kadircet Oct 28, 2024

Choose a reason for hiding this comment

Uh oh!

ChuanqiXu9 commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kadircet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kadircet Oct 29, 2024

Choose a reason for hiding this comment

Uh oh!

ChuanqiXu9 Oct 30, 2024

Choose a reason for hiding this comment

Uh oh!

kadircet left a comment

Choose a reason for hiding this comment

Uh oh!

ChuanqiXu9 commented Oct 31, 2024

Uh oh!

Uh oh!

Uh oh!

ChuanqiXu9 commented Oct 28, 2024 •

edited

Loading

llvmbot commented Oct 28, 2024 •

edited

Loading

github-actions bot commented Oct 28, 2024 •

edited

Loading

ChuanqiXu9 commented Oct 28, 2024 •

edited

Loading