[PATCH] D159497: [RFC][clangd] Check if SelectionTree is complete

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv added a comment.

ping? Any thoughts. Thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D159497/new/

https://reviews.llvm.org/D159497

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

[PATCH] D159497: [RFC][clangd] Check if SelectionTree is complete

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv created this revision.
Herald added subscribers: kadircet, arphaman.
Herald added a project: All.
kuganv retitled this revision from "[clangd] Check if SelectionTree is 
complete" to "[RFC][clangd] Check if SelectionTree is complete".
kuganv edited the summary of this revision.
kuganv added reviewers: sammccall, kadircet, nridge, DmitryPolukhin, 
ivanmurashko.
kuganv updated this revision to Diff 556418.
kuganv edited the summary of this revision.
kuganv added a comment.
kuganv edited the summary of this revision.
kuganv published this revision for review.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

Rebased


clangd in the presence of compilation error can navigate to completely 
irrelevant destinations. This is often a problem for headers that cannot be 
compiled standalone. For example, go-to-def often jumps to containing namespace 
because the selection tree just consists of namespace (when  relevant AST nodes 
were missing due to compilation error). This is known issue that is hard to 
solve without using some heuristic.  One such heuristic is to check the nodes 
in the selection tree for the exact line match. In the absence of such match, 
we do not provide navigation. IMO, this is better than jumping to wrong 
position.

TODO:

- This currently does not have test cases but I will add test cases based on 
the feedback for the heuristic.
- Enable only when there is compilation error.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D159497

Files:
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/Selection.h
  clang-tools-extra/clangd/XRefs.cpp


Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -179,6 +179,12 @@
   unsigned Offset = 
AST.getSourceManager().getDecomposedSpellingLoc(Pos).second;
   std::vector> Result;
   auto ResultFromTree = [&](SelectionTree ST) {
+// If the SelectionTree does not have nodes from Pos, we will navigate to
+// wrong locations. In these cases, it is better to return empty results
+// rather than jumping to irrelevant location.
+if (!ST.TochesSourceLoc(Pos, AST.getSourceManager())) {
+  return false;
+}
 if (const SelectionTree::Node *N = ST.commonAncestor()) {
   if (NodeKind)
 *NodeKind = N->ASTNode.getNodeKind();
Index: clang-tools-extra/clangd/Selection.h
===
--- clang-tools-extra/clangd/Selection.h
+++ clang-tools-extra/clangd/Selection.h
@@ -146,6 +146,7 @@
   const Node *commonAncestor() const;
   // The selection node corresponding to TranslationUnitDecl.
   const Node () const { return *Root; }
+  bool TochesSourceLoc(SourceLocation Pos, const SourceManager );
 
 private:
   // Creates a selection tree for the given range in the main file.
Index: clang-tools-extra/clangd/Selection.cpp
===
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -1104,6 +1104,29 @@
   return Ancestor != Root ? Ancestor : nullptr;
 }
 
+// Check if source line at Pos touches any node in SelectionTree. In the
+// presence of compilation error, we will have only root nodes without the
+// nodes that coresponds to tokens from Pos. In these cases, clangd cannot
+// provide meaningful navigation and often jumps to the conataining scope
+// identifier such as napespace.
+bool SelectionTree::TochesSourceLoc(SourceLocation Pos,
+const SourceManager ) {
+  const auto DecomposedPos = SM.getDecomposedSpellingLoc(Pos);
+  int Line = SM.getLineNumber(DecomposedPos.first, DecomposedPos.second);
+  for (const auto  : Nodes) {
+const auto Begin =
+SM.getDecomposedSpellingLoc(getSourceRange(Node.ASTNode).getBegin());
+const auto End =
+SM.getDecomposedSpellingLoc(getSourceRange(Node.ASTNode).getEnd());
+int startLine = SM.getLineNumber(Begin.first, Begin.second);
+int endLine = SM.getLineNumber(End.first, End.second);
+if (startLine == Line || endLine == Line) {
+  return true;
+}
+  }
+  return false;
+}
+
 const DeclContext ::Node::getDeclContext() const {
   for (const Node *CurrentNode = this; CurrentNode != nullptr;
CurrentNode = CurrentNode->Parent) {


Index: clang-tools-extra/clangd/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -179,6 +179,12 @@
   unsigned Offset = AST.getSourceManager().getDecomposedSpellingLoc(Pos).second;
   std::vector> Result;
   auto ResultFromTree = [&](SelectionTree ST) {
+// If the SelectionTree does not have nodes from Pos, we will navigate to
+// wrong locations. In these cases, it is better to return empty results
+  

[PATCH] D148088: [RFC][clangd] Move preamble index out of document open critical path

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv updated this revision to Diff 530328.
kuganv marked 6 inline comments as done.
kuganv added a comment.

Updated based on the review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148088/new/

https://reviews.llvm.org/D148088

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestWorkspace.cpp
  clang/include/clang/Frontend/CompilerInstance.h

Index: clang/include/clang/Frontend/CompilerInstance.h
===
--- clang/include/clang/Frontend/CompilerInstance.h
+++ clang/include/clang/Frontend/CompilerInstance.h
@@ -12,6 +12,7 @@
 #include "clang/AST/ASTConsumer.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetInfo.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
@@ -233,6 +234,8 @@
 return *Invocation;
   }
 
+  std::shared_ptr getInvocationPtr() { return Invocation; }
+
   /// setInvocation - Replace the current invocation.
   void setInvocation(std::shared_ptr Value);
 
@@ -338,6 +341,11 @@
 return *Diagnostics;
   }
 
+  IntrusiveRefCntPtr getDiagnosticsPtr() const {
+assert(Diagnostics && "Compiler instance has no diagnostics!");
+return Diagnostics;
+  }
+
   /// setDiagnostics - Replace the current diagnostics engine.
   void setDiagnostics(DiagnosticsEngine *Value);
 
@@ -373,6 +381,11 @@
 return *Target;
   }
 
+  IntrusiveRefCntPtr getTargetPtr() const {
+assert(Target && "Compiler instance has no target!");
+return Target;
+  }
+
   /// Replace the current Target.
   void setTarget(TargetInfo *Value);
 
@@ -406,6 +419,11 @@
 return *FileMgr;
   }
 
+  IntrusiveRefCntPtr getFileManagerPtr() const {
+assert(FileMgr && "Compiler instance has no file manager!");
+return FileMgr;
+  }
+
   void resetAndLeakFileManager() {
 llvm::BuryPointer(FileMgr.get());
 FileMgr.resetWithoutRelease();
@@ -426,6 +444,11 @@
 return *SourceMgr;
   }
 
+  IntrusiveRefCntPtr getSourceManagerPtr() const {
+assert(SourceMgr && "Compiler instance has no source manager!");
+return SourceMgr;
+  }
+
   void resetAndLeakSourceManager() {
 llvm::BuryPointer(SourceMgr.get());
 SourceMgr.resetWithoutRelease();
@@ -466,6 +489,11 @@
 return *Context;
   }
 
+  IntrusiveRefCntPtr getASTContextPtr() const {
+assert(Context && "Compiler instance has no AST context!");
+return Context;
+  }
+
   void resetAndLeakASTContext() {
 llvm::BuryPointer(Context.get());
 Context.resetWithoutRelease();
Index: clang-tools-extra/clangd/unittests/TestWorkspace.cpp
===
--- clang-tools-extra/clangd/unittests/TestWorkspace.cpp
+++ clang-tools-extra/clangd/unittests/TestWorkspace.cpp
@@ -21,11 +21,14 @@
   continue;
 TU.Code = Input.second.Code;
 TU.Filename = Input.first().str();
-TU.preamble([&](ASTContext , Preprocessor ,
-const CanonicalIncludes ) {
-  Index->updatePreamble(testPath(Input.first()), "null", Ctx, PP,
-CanonIncludes);
-});
+TU.preamble(
+[&](CapturedASTCtx ASTCtx,
+const std::shared_ptr CanonIncludes) {
+  auto  = ASTCtx.getASTContext();
+  auto  = ASTCtx.getPreprocessor();
+  Index->updatePreamble(testPath(Input.first()), "null", Ctx, PP,
+*CanonIncludes);
+});
 ParsedAST MainAST = TU.build();
 Index->updateMain(testPath(Input.first()), MainAST);
   }
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -1129,9 +1129,9 @@
   public:
 BlockPreambleThread(llvm::StringRef BlockVersion, Notification )
 : BlockVersion(BlockVersion), N(N) {}
-void onPreambleAST(PathRef Path, llvm::StringRef Version,
-   const CompilerInvocation &, ASTContext ,
-   Preprocessor &, const CanonicalIncludes &) override {
+void
+onPreambleAST(PathRef Path, llvm::StringRef Version, CapturedASTCtx,
+  const std::shared_ptr) override {
   if (Version == BlockVersion)
 N.wait();
 }
@@ -1208,9 +1208,9 @@
 BlockPreambleThread(Notification , DiagsCB CB)
 

[PATCH] D148088: [RFC][clangd] Move preamble index out of document open critical path

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv added inline comments.



Comment at: clang-tools-extra/clangd/Preamble.cpp:106-113
+CI.setSema(nullptr);
+CI.setASTConsumer(nullptr);
+if (CI.getASTReader()) {
+  CI.getASTReader()->setDeserializationListener(nullptr);
+  /* This just sets consumer to null when DeserializationListener is null 
*/
+  CI.getASTReader()->StartTranslationUnit(nullptr);
 }

kadircet wrote:
> kuganv wrote:
> > kadircet wrote:
> > > kuganv wrote:
> > > > kadircet wrote:
> > > > > why are we triggering destructors for all of these objects eagerly? 
> > > > > if this is deliberate to "fix" some issue, could you mention that in 
> > > > > comments?
> > > > > why are we triggering destructors for all of these objects eagerly? 
> > > > > if this is deliberate to "fix" some issue, could you mention that in 
> > > > > comments?
> > > > 
> > > > Thanks a lot for the review.
> > > > If we don't destruct and set it to null, CapturedASTCtx will also have 
> > > > to keep instances such as ASTConsumer including other related callbacks 
> > > > such PreambleCallbacks. This was making the CapturedASTCtx interface 
> > > > and implementation complex.
> > > > If we don't destruct and set it to null, CapturedASTCtx will also have 
> > > > to keep instances such as ASTConsumer 
> > > 
> > > Sorry if I am being dense but I can't actually see any references to 
> > > those objects in the structs we preseve. AFAICT, Sema, ASTConsumer and 
> > > ASTReader are members of CompilerInstance, which we don't keep around. 
> > > Can you point me towards any references to the rest of the objects you're 
> > > setting to null in case I am missing any?
> > > 
> > > `ASTMutationListener` seems to be the only one that's relevant. It's 
> > > indeed exposed through `ASTContext` and our indexing operations can 
> > > trigger various callbacks to fire. Can you set only that one to nullptr 
> > > and leave a comment explaining the situation?
> > > > If we don't destruct and set it to null, CapturedASTCtx will also have 
> > > > to keep instances such as ASTConsumer 
> > > 
> > > Sorry if I am being dense but I can't actually see any references to 
> > > those objects in the structs we preseve. AFAICT, Sema, ASTConsumer and 
> > > ASTReader are members of CompilerInstance, which we don't keep around. 
> > > Can you point me towards any references to the rest of the objects you're 
> > > setting to null in case I am missing any?
> > > 
> > > `ASTMutationListener` seems to be the only one that's relevant. It's 
> > > indeed exposed through `ASTContext` and our indexing operations can 
> > > trigger various callbacks to fire. Can you set only that one to nullptr 
> > > and leave a comment explaining the situation?
> > 
> > Thanks for the review.  I will revise based on the feedback. 
> > 
> > In buildPreamble, we create CppFilePreambleCallbacks in stack to be used in 
> > PrecompiledPreamble::Build which becomes part of 
> > PrecompilePreambleConsumer. I think this this need to be reset. Probably 
> > this has to be done in a different way. If I dont reset, I am seeing crash 
> > in some cases. See:
> > 
> > ```
> > 85c0c79f in clang::ASTReader::DecodeIdentifierInfo(unsigned int) 
> > /home/kugan/local/llvm-project/clang/lib/Serialization/ASTReader.cpp:8695:32
> > #1 0x558285c75266 in 
> > clang::ASTReader::readIdentifier(clang::serialization::ModuleFile&, 
> > llvm::SmallVector const&, unsigned int&) 
> > /home/kugan/local/llvm-project/clang/include/clang/Seriali
> > zation/ASTReader.h:2126:12
> > #2 0x558285c75266 in clang::ASTRecordReader::readIdentifier() 
> > /home/kugan/local/llvm-project/clang/include/clang/Serialization/ASTRecordReader.h:211:20
> > #3 0x558285c75266 in 
> > clang::serialization::BasicReaderBase::readDeclarationName()
> >  
> > /home/kugan/local/llvm-project/build/tools/clang/include/clang/AST/AbstractBasicReader.inc:780:58
> > #4 0x558285ce10bc in 
> > clang::ASTDeclReader::VisitNamedDecl(clang::NamedDecl*) 
> > /home/kugan/local/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:694:26
> > #5 0x558285ce10bc in 
> > clang::ASTDeclReader::VisitNamespaceDecl(clang::NamespaceDecl*) 
> > /home/kugan/local/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:1785:3
> > #6 0x558285cbc6e8 in clang::ASTDeclReader::Visit(clang::Decl*) 
> > /home/kugan/local/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:543:37
> > #7 0x558285d5a133 in clang::ASTReader::ReadDeclRecord(unsigned int) 
> > /home/kugan/local/llvm-project/clang/lib/Serialization/ASTReaderDecl.cpp:4052:10
> > #8 0x558285c73677 in clang::ASTReader::GetDecl(unsigned int) 
> > /home/kugan/local/llvm-project/clang/lib/Serialization/ASTReader.cpp:7607:5
> > #9 0x558285c73677 in 
> > clang::ASTReader::GetLocalDecl(clang::serialization::ModuleFile&, unsigned 
> > int) 
> > /home/kugan/local/llvm-project/clang/include/clang/Serialization/ASTReader.h:1913:12
> > #10 0x558285bfd741 in 

[PATCH] D148088: [RFC][clangd] Move preamble index out of document open critical path

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv updated this revision to Diff 524836.
kuganv marked an inline comment as done.
kuganv added a comment.

Set just PrecompilePreambleConsumer/PCHGenerator and ASTMutationListener to 
null.
Set the FileManager VFS to consuming FS so that it is thread safe.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148088/new/

https://reviews.llvm.org/D148088

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestWorkspace.cpp
  clang/include/clang/Frontend/CompilerInstance.h

Index: clang/include/clang/Frontend/CompilerInstance.h
===
--- clang/include/clang/Frontend/CompilerInstance.h
+++ clang/include/clang/Frontend/CompilerInstance.h
@@ -12,6 +12,7 @@
 #include "clang/AST/ASTConsumer.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetInfo.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
@@ -233,6 +234,8 @@
 return *Invocation;
   }
 
+  std::shared_ptr getInvocationPtr() { return Invocation; }
+
   /// setInvocation - Replace the current invocation.
   void setInvocation(std::shared_ptr Value);
 
@@ -338,6 +341,11 @@
 return *Diagnostics;
   }
 
+  IntrusiveRefCntPtr getDiagnosticsPtr() const {
+assert(Diagnostics && "Compiler instance has no diagnostics!");
+return Diagnostics;
+  }
+
   /// setDiagnostics - Replace the current diagnostics engine.
   void setDiagnostics(DiagnosticsEngine *Value);
 
@@ -373,6 +381,11 @@
 return *Target;
   }
 
+  IntrusiveRefCntPtr getTargetPtr() const {
+assert(Target && "Compiler instance has no target!");
+return Target;
+  }
+
   /// Replace the current Target.
   void setTarget(TargetInfo *Value);
 
@@ -406,6 +419,11 @@
 return *FileMgr;
   }
 
+  IntrusiveRefCntPtr getFileManagerPtr() const {
+assert(FileMgr && "Compiler instance has no file manager!");
+return FileMgr;
+  }
+
   void resetAndLeakFileManager() {
 llvm::BuryPointer(FileMgr.get());
 FileMgr.resetWithoutRelease();
@@ -426,6 +444,11 @@
 return *SourceMgr;
   }
 
+  IntrusiveRefCntPtr getSourceManagerPtr() const {
+assert(SourceMgr && "Compiler instance has no source manager!");
+return SourceMgr;
+  }
+
   void resetAndLeakSourceManager() {
 llvm::BuryPointer(SourceMgr.get());
 SourceMgr.resetWithoutRelease();
@@ -466,6 +489,11 @@
 return *Context;
   }
 
+  IntrusiveRefCntPtr getASTContextPtr() const {
+assert(Context && "Compiler instance has no AST context!");
+return Context;
+  }
+
   void resetAndLeakASTContext() {
 llvm::BuryPointer(Context.get());
 Context.resetWithoutRelease();
Index: clang-tools-extra/clangd/unittests/TestWorkspace.cpp
===
--- clang-tools-extra/clangd/unittests/TestWorkspace.cpp
+++ clang-tools-extra/clangd/unittests/TestWorkspace.cpp
@@ -21,11 +21,14 @@
   continue;
 TU.Code = Input.second.Code;
 TU.Filename = Input.first().str();
-TU.preamble([&](ASTContext , Preprocessor ,
-const CanonicalIncludes ) {
-  Index->updatePreamble(testPath(Input.first()), "null", Ctx, PP,
-CanonIncludes);
-});
+TU.preamble(
+[&](CapturedASTCtx ASTCtx,
+const std::shared_ptr CanonIncludes) {
+  auto  = ASTCtx.getASTContext();
+  auto  = ASTCtx.getPreprocessor();
+  Index->updatePreamble(testPath(Input.first()), "null", Ctx, PP,
+*CanonIncludes);
+});
 ParsedAST MainAST = TU.build();
 Index->updateMain(testPath(Input.first()), MainAST);
   }
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -1129,9 +1129,9 @@
   public:
 BlockPreambleThread(llvm::StringRef BlockVersion, Notification )
 : BlockVersion(BlockVersion), N(N) {}
-void onPreambleAST(PathRef Path, llvm::StringRef Version,
-   const CompilerInvocation &, ASTContext ,
-   Preprocessor &, const CanonicalIncludes &) override {
+void
+onPreambleAST(PathRef Path, llvm::StringRef Version, CapturedASTCtx,
+  const std::shared_ptr) override {
   if (Version == BlockVersion)
 N.wait();
 }
@@ -1208,9 

[PATCH] D148088: [RFC][clangd] Move preamble index out of document open critical path

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv updated this revision to Diff 523784.
kuganv added a comment.

Revised Except for:

1. Explicitly destructing in AfterExecute
2. setStatCache to take refence

Removing Both of which is causing crash.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148088/new/

https://reviews.llvm.org/D148088

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestWorkspace.cpp
  clang/include/clang/Frontend/CompilerInstance.h

Index: clang/include/clang/Frontend/CompilerInstance.h
===
--- clang/include/clang/Frontend/CompilerInstance.h
+++ clang/include/clang/Frontend/CompilerInstance.h
@@ -12,6 +12,7 @@
 #include "clang/AST/ASTConsumer.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetInfo.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
@@ -233,6 +234,8 @@
 return *Invocation;
   }
 
+  std::shared_ptr getInvocationPtr() { return Invocation; }
+
   /// setInvocation - Replace the current invocation.
   void setInvocation(std::shared_ptr Value);
 
@@ -338,6 +341,11 @@
 return *Diagnostics;
   }
 
+  IntrusiveRefCntPtr getDiagnosticsPtr() const {
+assert(Diagnostics && "Compiler instance has no diagnostics!");
+return Diagnostics;
+  }
+
   /// setDiagnostics - Replace the current diagnostics engine.
   void setDiagnostics(DiagnosticsEngine *Value);
 
@@ -373,6 +381,11 @@
 return *Target;
   }
 
+  IntrusiveRefCntPtr getTargetPtr() const {
+assert(Target && "Compiler instance has no target!");
+return Target;
+  }
+
   /// Replace the current Target.
   void setTarget(TargetInfo *Value);
 
@@ -406,6 +419,11 @@
 return *FileMgr;
   }
 
+  IntrusiveRefCntPtr getFileManagerPtr() const {
+assert(FileMgr && "Compiler instance has no file manager!");
+return FileMgr;
+  }
+
   void resetAndLeakFileManager() {
 llvm::BuryPointer(FileMgr.get());
 FileMgr.resetWithoutRelease();
@@ -426,6 +444,11 @@
 return *SourceMgr;
   }
 
+  IntrusiveRefCntPtr getSourceManagerPtr() const {
+assert(SourceMgr && "Compiler instance has no source manager!");
+return SourceMgr;
+  }
+
   void resetAndLeakSourceManager() {
 llvm::BuryPointer(SourceMgr.get());
 SourceMgr.resetWithoutRelease();
@@ -466,6 +489,11 @@
 return *Context;
   }
 
+  IntrusiveRefCntPtr getASTContextPtr() const {
+assert(Context && "Compiler instance has no AST context!");
+return Context;
+  }
+
   void resetAndLeakASTContext() {
 llvm::BuryPointer(Context.get());
 Context.resetWithoutRelease();
Index: clang-tools-extra/clangd/unittests/TestWorkspace.cpp
===
--- clang-tools-extra/clangd/unittests/TestWorkspace.cpp
+++ clang-tools-extra/clangd/unittests/TestWorkspace.cpp
@@ -21,11 +21,14 @@
   continue;
 TU.Code = Input.second.Code;
 TU.Filename = Input.first().str();
-TU.preamble([&](ASTContext , Preprocessor ,
-const CanonicalIncludes ) {
-  Index->updatePreamble(testPath(Input.first()), "null", Ctx, PP,
-CanonIncludes);
-});
+TU.preamble(
+[&](CapturedASTCtx ASTCtx,
+const std::shared_ptr CanonIncludes) {
+  auto  = ASTCtx.getASTContext();
+  auto  = ASTCtx.getPreprocessor();
+  Index->updatePreamble(testPath(Input.first()), "null", Ctx, PP,
+*CanonIncludes);
+});
 ParsedAST MainAST = TU.build();
 Index->updateMain(testPath(Input.first()), MainAST);
   }
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -1129,9 +1129,9 @@
   public:
 BlockPreambleThread(llvm::StringRef BlockVersion, Notification )
 : BlockVersion(BlockVersion), N(N) {}
-void onPreambleAST(PathRef Path, llvm::StringRef Version,
-   const CompilerInvocation &, ASTContext ,
-   Preprocessor &, const CanonicalIncludes &) override {
+void
+onPreambleAST(PathRef Path, llvm::StringRef Version, CapturedASTCtx,
+  const std::shared_ptr) override {
   if (Version == BlockVersion)
 N.wait();
 }
@@ -1208,9 +1208,9 @@
 BlockPreambleThread(Notification , 

[PATCH] D148088: [RFC][clangd] Move preamble index out of document open critical path

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv marked 2 inline comments as done.
kuganv added inline comments.



Comment at: clang-tools-extra/clangd/Preamble.cpp:698-700
+  // While extending the life of FileMgr and VFS, StatCache should also be
+  // extended.
+  Ctx.setStatCache(Result->StatCache);

kadircet wrote:
> we use a "producingfs" when building the preamble, hence there shouldn't be 
> any references to statcache inside the FM stored in the compiler instance 
> here. have you noticed something to the contrary ?
> we use a "producingfs" when building the preamble, hence there shouldn't be 
> any references to statcache inside the FM stored in the compiler instance 
> here. have you noticed something to the contrary ?


In PrecompiledPreamble::Build, we use StatCacheFS for FileMgr creation. 
Shouldn’t we keep this Alive? Without this, I am seeing:

```
==1509807==ERROR: AddressSanitizer: heap-use-after-free on address 
0x606000464c00 at pc 0x55f558fd7de2 bp 0x7f18a73ff010 sp 0x7f18a73ff008
READ of size 8 at 0x606000464c00 thread T48 (NodeServer.cpp1)
V[23:14:41.667] Ignored diagnostic. 
/data/users/kugan/fbsource/fbcode/synapse_dev/prod/NodeServer.cpp:2:2:Mandatory 
header  not found in standard library!
I[23:14:41.669] Indexed c++20 standard library (incomplete due to errors): 0 
symbols, 0 filtered
#0 0x55f558fd7de1 in std::__cxx11::basic_string, std::allocator>::_M_data() const 
/opt/rh/gcc-toolset-12/root/usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/bits/basic_string.h:234:28
#1 0x55f558fd7de1 in std::__cxx11::basic_string, std::allocator>::data() const 
/opt/rh/gcc-toolset-12/root/usr/lib/gcc/x86_64-redhat-linux/12/../../../../include/c++/12/bits/basic_string.h:2567:16
#2 0x55f558fd7de1 in 
llvm::StringRef::StringRef(std::__cxx11::basic_string, std::allocator> const&) 
/home/kugan/local/llvm-project/llvm/include/llvm/ADT/StringRef.h:101:18
#3 0x55f558fd7de1 in 
clang::clangd::PreambleFileStatusCache::update(llvm::vfs::FileSystem const&, 
llvm::vfs::Status) 
/home/kugan/local/llvm-project/clang-tools-extra/clangd/FS.cpp:33:20
#4 0x55f558fd925f in 
clang::clangd::PreambleFileStatusCache::getProducingFS(llvm::IntrusiveRefCntPtr)::CollectFS::status(llvm::Twine
 const&) /home/kugan/local/llvm-project/clang-tools-extra/clangd/FS.cpp:82:19
#5 0x55f55917ddee in clang::clangd::(anonymous 
namespace)::TimerFS::status(llvm::Twine const&) 
/home/kugan/local/llvm-project/clang-tools-extra/clangd/Preamble.cpp:492:30
#6 0x55f55670dcff in clang::FileSystemStatCache::get(llvm::StringRef, 
llvm::vfs::Status&, bool, std::unique_ptr>*, clang::FileSystemStatCache*, 
llvm::vfs::FileSystem&) 
/home/kugan/local/llvm-project/clang/lib/Basic/FileSystemStatCache.cpp:47:55
#7 0x55f556701ab4 in clang::FileManager::getStatValue(llvm::StringRef, 
llvm::vfs::Status&, bool, std::unique_ptr>*) 
/home/kugan/local/llvm-project/clang/lib/Basic/FileManager.cpp:590:12
#8 0x55f556700eed in clang::FileManager::getDirectoryRef(llvm::StringRef, 
bool) /home/kugan/local/llvm-project/clang/lib/Basic/FileManager.cpp:161:20
#9 0x55f556701d3e in clang::FileManager::getDirectory(llvm::StringRef, 
bool) /home/kugan/local/llvm-project/clang/lib/Basic/FileManager.cpp:191:17
#10 0x55f55929a800 in 
clang::clangd::getCanonicalPath[abi:cxx11](clang::FileEntryRef, 
clang::SourceManager const&) 
/home/kugan/local/llvm-project/clang-tools-extra/clangd/SourceCode.cpp:540:45
#11 0x55f559627681 in 
clang::clangd::SymbolCollector::HeaderFileURICache::toURI[abi:cxx11](clang::FileEntryRef)
 
/home/kugan/local/llvm-project/clang-tools-extra/clangd/index/SymbolCollector.cpp:211:24
#12 0x55f55961926e in 
clang::clangd::SymbolCollector::getTokenLocation(clang::SourceLocation) 
/home/kugan/local/llvm-project/clang-tools-extra/clangd/index/SymbolCollector.cpp:425:36
#13 0x55f5596233a9 in 
clang::clangd::SymbolCollector::handleMacroOccurrence(clang::IdentifierInfo 
const*, clang::MacroInfo const*, unsigned int, clang::SourceLocation) 
/home/kugan/local/llvm-project/clang-tools-extra/clangd/index/SymbolCollector.cpp:753:22
#14 0x55f55c1c7e3d in indexPreprocessorMacro(clang::IdentifierInfo const*, 
clang::MacroInfo const*, clang::MacroDirective::Kind, clang::SourceLocation, 
clang::index::IndexDataConsumer&) 
/home/kugan/local/llvm-project/clang/lib/Index/IndexingAction.cpp:229:16
#15 0x55f55c1c7e3d in indexPreprocessorMacros(clang::Preprocessor&, 
clang::index::IndexDataConsumer&) 
/home/kugan/local/llvm-project/clang/lib/Index/IndexingAction.cpp:236:7
#16 0x55f55c1c8290 in clang::index::indexTopLevelDecls(clang::ASTContext&, 
clang::Preprocessor&, llvm::ArrayRef, 
clang::index::IndexDataConsumer&, clang::index::IndexingOptions) 
/home/kugan/local/llvm-project/clang/lib/Index/IndexingAction.cpp:283:5
#17 0x55f55959d350 in clang::clangd::(anonymous 
namespace)::indexSymbols(clang::ASTContext&, clang::Preprocessor&, 
llvm::ArrayRef, clang::clangd::MainFileMacros const*, 

[PATCH] D148088: [RFC][clangd] Move preamble index out of document open critical path

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv updated this revision to Diff 522523.
kuganv added a comment.

Fixed missing merge that caused build error.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148088/new/

https://reviews.llvm.org/D148088

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestWorkspace.cpp
  clang/include/clang/Frontend/CompilerInstance.h

Index: clang/include/clang/Frontend/CompilerInstance.h
===
--- clang/include/clang/Frontend/CompilerInstance.h
+++ clang/include/clang/Frontend/CompilerInstance.h
@@ -12,6 +12,7 @@
 #include "clang/AST/ASTConsumer.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetInfo.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
@@ -233,6 +234,8 @@
 return *Invocation;
   }
 
+  std::shared_ptr getInvocationPtr() { return Invocation; }
+
   /// setInvocation - Replace the current invocation.
   void setInvocation(std::shared_ptr Value);
 
@@ -338,6 +341,11 @@
 return *Diagnostics;
   }
 
+  IntrusiveRefCntPtr getDiagnosticsPtr() const {
+assert(Diagnostics && "Compiler instance has no diagnostics!");
+return Diagnostics;
+  }
+
   /// setDiagnostics - Replace the current diagnostics engine.
   void setDiagnostics(DiagnosticsEngine *Value);
 
@@ -373,6 +381,11 @@
 return *Target;
   }
 
+  IntrusiveRefCntPtr getTargetPtr() const {
+assert(Target && "Compiler instance has no target!");
+return Target;
+  }
+
   /// Replace the current Target.
   void setTarget(TargetInfo *Value);
 
@@ -406,6 +419,11 @@
 return *FileMgr;
   }
 
+  IntrusiveRefCntPtr getFileManagerPtr() const {
+assert(FileMgr && "Compiler instance has no file manager!");
+return FileMgr;
+  }
+
   void resetAndLeakFileManager() {
 llvm::BuryPointer(FileMgr.get());
 FileMgr.resetWithoutRelease();
@@ -426,6 +444,11 @@
 return *SourceMgr;
   }
 
+  IntrusiveRefCntPtr getSourceManagerPtr() const {
+assert(SourceMgr && "Compiler instance has no source manager!");
+return SourceMgr;
+  }
+
   void resetAndLeakSourceManager() {
 llvm::BuryPointer(SourceMgr.get());
 SourceMgr.resetWithoutRelease();
@@ -466,6 +489,11 @@
 return *Context;
   }
 
+  IntrusiveRefCntPtr getASTContextPtr() const {
+assert(Context && "Compiler instance has no AST context!");
+return Context;
+  }
+
   void resetAndLeakASTContext() {
 llvm::BuryPointer(Context.get());
 Context.resetWithoutRelease();
Index: clang-tools-extra/clangd/unittests/TestWorkspace.cpp
===
--- clang-tools-extra/clangd/unittests/TestWorkspace.cpp
+++ clang-tools-extra/clangd/unittests/TestWorkspace.cpp
@@ -21,10 +21,12 @@
   continue;
 TU.Code = Input.second.Code;
 TU.Filename = Input.first().str();
-TU.preamble([&](ASTContext , Preprocessor ,
-const CanonicalIncludes ) {
+TU.preamble([&](CapturedASTCtx ASTCtx,
+const std::shared_ptr CanonIncludes) {
+  auto  = ASTCtx.getASTContext();
+  auto  = ASTCtx.getPreprocessor();
   Index->updatePreamble(testPath(Input.first()), "null", Ctx, PP,
-CanonIncludes);
+*CanonIncludes);
 });
 ParsedAST MainAST = TU.build();
 Index->updateMain(testPath(Input.first()), MainAST);
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -1129,9 +1129,8 @@
   public:
 BlockPreambleThread(llvm::StringRef BlockVersion, Notification )
 : BlockVersion(BlockVersion), N(N) {}
-void onPreambleAST(PathRef Path, llvm::StringRef Version,
-   const CompilerInvocation &, ASTContext ,
-   Preprocessor &, const CanonicalIncludes &) override {
+void onPreambleAST(PathRef Path, llvm::StringRef Version, CapturedASTCtx,
+   const std::shared_ptr) override {
   if (Version == BlockVersion)
 N.wait();
 }
@@ -1208,9 +1207,8 @@
 BlockPreambleThread(Notification , DiagsCB CB)
 : UnblockPreamble(UnblockPreamble), CB(std::move(CB)) {}
 
-void onPreambleAST(PathRef Path, llvm::StringRef Version,
-   

[PATCH] D148088: [RFC][clangd] Move preamble index out of document open critical path

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv added inline comments.



Comment at: clang-tools-extra/clangd/Preamble.cpp:694
 Result->MainIsIncludeGuarded = CapturedInfo.isMainFileIncludeGuarded();
-return Result;
+CapturedCtx.emplace(CapturedInfo.takeLife());
+return std::make_pair(Result, CapturedCtx);

kadircet wrote:
> kuganv wrote:
> > kadircet wrote:
> > > what about just keeping the callback (with a different signature) and 
> > > calling it here? e.g.:
> > > ```
> > > PreambleCallback(CapturedInfo.takeLife());
> > > ```
> > > 
> > > that way we don't need to change the return type and also control if we 
> > > received a valid object on every call site (since callback will only be 
> > > invoked on success)
> > > what about just keeping the callback (with a different signature) and 
> > > calling it here? e.g.:
> > > ```
> > > PreambleCallback(CapturedInfo.takeLife());
> > > ```
> > > 
> > > that way we don't need to change the return type and also control if we 
> > > received a valid object on every call site (since callback will only be 
> > > invoked on success)
> > 
> > Apologies for the misunderstanding.  Just to be clear, you prefer indexing 
> > in UpdateIndexCallbacks using the thread Tasks that also indexes in 
> > indexStdlib? In this implementation, I am calling the index action in 
> > preamble thread. I will revise it.
> > Apologies for the misunderstanding. Just to be clear, you prefer indexing 
> > in UpdateIndexCallbacks using the thread Tasks that also indexes in 
> > indexStdlib? In this implementation, I am calling the index action in 
> > preamble thread. I will revise it.
> 
> Yes, i guess that's the reason why I was confused while looking at the code. 
> Sorry if I give the impression that suggests doing the indexing on 
> `PreambleThread`, but I think both in my initial comment:
> 
> >> As for AsyncTaskRunner to use, since this indexing task only depends on 
> >> the file-index, which is owned by ClangdServer, I don't think there's any 
> >> need to introduce a new taskrunner into TUScheduler and block its 
> >> destruction. We can just re-use the existing TaskRunner inside 
> >> parsingcallbacks, in which we run stdlib indexing tasks.
> 
> and in the follow up;
> 
> >> I think we can just change the signature for PreambleParsedCallback to 
> >> pass along refcounted objects. forgot to mention in the first comment, but 
> >> we should also change the CanonicalIncludes to be a shared_ptr so that it 
> >> can outlive the PreambleData. We should invoke the callback inside 
> >> buildPreamble after a successful build. Afterwards we should also change 
> >> the signature for onPreambleAST to take AST, PP and CanonicalIncludes as 
> >> ref-counted objects again and PreambleThread::build should just forward 
> >> objects received from PreambleParsedCallback. Afterwards inside the 
> >> UpdateIndexCallbacks::onPreambleAST we can just invoke indexing on Tasks 
> >> if it's present or synchronously in the absence of it.
> 
> I was pointing towards running this inside the `Tasks` in 
> `UpdateIndexCallbacks`.
> 
> ---
> 
> There's definitely some upsides to running that indexing on the preamble 
> thread as well (which is what we do today) but I think the extra sequencing 
> requirements (make sure to first notify the ASTPeer and then issue preamble 
> callbacks) we put into TUScheduler (which is already quite complex) is 
> probably not worth it.
> > Apologies for the misunderstanding. Just to be clear, you prefer indexing 
> > in UpdateIndexCallbacks using the thread Tasks that also indexes in 
> > indexStdlib? In this implementation, I am calling the index action in 
> > preamble thread. I will revise it.
> 
> Yes, i guess that's the reason why I was confused while looking at the code. 
> Sorry if I give the impression that suggests doing the indexing on 
> `PreambleThread`, but I think both in my initial comment:
> 
> >> As for AsyncTaskRunner to use, since this indexing task only depends on 
> >> the file-index, which is owned by ClangdServer, I don't think there's any 
> >> need to introduce a new taskrunner into TUScheduler and block its 
> >> destruction. We can just re-use the existing TaskRunner inside 
> >> parsingcallbacks, in which we run stdlib indexing tasks.
> 
> and in the follow up;
> 
> >> I think we can just change the signature for PreambleParsedCallback to 
> >> pass along refcounted objects. forgot to mention in the first comment, but 
> >> we should also change the CanonicalIncludes to be a shared_ptr so that it 
> >> can outlive the PreambleData. We should invoke the callback inside 
> >> buildPreamble after a successful build. Afterwards we should also change 
> >> the signature for onPreambleAST to take AST, PP and CanonicalIncludes as 
> >> ref-counted objects again and PreambleThread::build should just forward 
> >> objects received from PreambleParsedCallback. Afterwards inside the 
> >> UpdateIndexCallbacks::onPreambleAST we can just invoke 

[PATCH] D148088: [RFC][clangd] Move preamble index out of document open critical path

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv updated this revision to Diff 522471.
kuganv added a comment.

As per review, moved Preamble indexing into ClangdServer's IndexTasks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148088/new/

https://reviews.llvm.org/D148088

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestWorkspace.cpp
  clang/include/clang/Frontend/CompilerInstance.h

Index: clang/include/clang/Frontend/CompilerInstance.h
===
--- clang/include/clang/Frontend/CompilerInstance.h
+++ clang/include/clang/Frontend/CompilerInstance.h
@@ -12,6 +12,7 @@
 #include "clang/AST/ASTConsumer.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetInfo.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
@@ -233,6 +234,8 @@
 return *Invocation;
   }
 
+  std::shared_ptr getInvocationPtr() { return Invocation; }
+
   /// setInvocation - Replace the current invocation.
   void setInvocation(std::shared_ptr Value);
 
@@ -338,6 +341,11 @@
 return *Diagnostics;
   }
 
+  IntrusiveRefCntPtr getDiagnosticsPtr() const {
+assert(Diagnostics && "Compiler instance has no diagnostics!");
+return Diagnostics;
+  }
+
   /// setDiagnostics - Replace the current diagnostics engine.
   void setDiagnostics(DiagnosticsEngine *Value);
 
@@ -373,6 +381,11 @@
 return *Target;
   }
 
+  IntrusiveRefCntPtr getTargetPtr() const {
+assert(Target && "Compiler instance has no target!");
+return Target;
+  }
+
   /// Replace the current Target.
   void setTarget(TargetInfo *Value);
 
@@ -406,6 +419,11 @@
 return *FileMgr;
   }
 
+  IntrusiveRefCntPtr getFileManagerPtr() const {
+assert(FileMgr && "Compiler instance has no file manager!");
+return FileMgr;
+  }
+
   void resetAndLeakFileManager() {
 llvm::BuryPointer(FileMgr.get());
 FileMgr.resetWithoutRelease();
@@ -426,6 +444,11 @@
 return *SourceMgr;
   }
 
+  IntrusiveRefCntPtr getSourceManagerPtr() const {
+assert(SourceMgr && "Compiler instance has no source manager!");
+return SourceMgr;
+  }
+
   void resetAndLeakSourceManager() {
 llvm::BuryPointer(SourceMgr.get());
 SourceMgr.resetWithoutRelease();
@@ -466,6 +489,11 @@
 return *Context;
   }
 
+  IntrusiveRefCntPtr getASTContextPtr() const {
+assert(Context && "Compiler instance has no AST context!");
+return Context;
+  }
+
   void resetAndLeakASTContext() {
 llvm::BuryPointer(Context.get());
 Context.resetWithoutRelease();
Index: clang-tools-extra/clangd/unittests/TestWorkspace.cpp
===
--- clang-tools-extra/clangd/unittests/TestWorkspace.cpp
+++ clang-tools-extra/clangd/unittests/TestWorkspace.cpp
@@ -21,10 +21,12 @@
   continue;
 TU.Code = Input.second.Code;
 TU.Filename = Input.first().str();
-TU.preamble([&](ASTContext , Preprocessor ,
-const CanonicalIncludes ) {
+TU.preamble([&](CapturedASTCtx ASTCtx,
+const std::shared_ptr CanonIncludes) {
+  auto  = ASTCtx.getASTContext();
+  auto  = ASTCtx.getPreprocessor();
   Index->updatePreamble(testPath(Input.first()), "null", Ctx, PP,
-CanonIncludes);
+*CanonIncludes);
 });
 ParsedAST MainAST = TU.build();
 Index->updateMain(testPath(Input.first()), MainAST);
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -1129,9 +1129,8 @@
   public:
 BlockPreambleThread(llvm::StringRef BlockVersion, Notification )
 : BlockVersion(BlockVersion), N(N) {}
-void onPreambleAST(PathRef Path, llvm::StringRef Version,
-   const CompilerInvocation &, ASTContext ,
-   Preprocessor &, const CanonicalIncludes &) override {
+void onPreambleAST(PathRef Path, llvm::StringRef Version, CapturedASTCtx,
+   const std::shared_ptr) override {
   if (Version == BlockVersion)
 N.wait();
 }
@@ -1208,9 +1207,8 @@
 BlockPreambleThread(Notification , DiagsCB CB)
 : UnblockPreamble(UnblockPreamble), CB(std::move(CB)) {}
 
-void onPreambleAST(PathRef Path, llvm::StringRef 

[PATCH] D148088: [RFC][clangd] Move preamble index out of document open critical path

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv added inline comments.



Comment at: clang-tools-extra/clangd/Preamble.cpp:106-113
+CI.setSema(nullptr);
+CI.setASTConsumer(nullptr);
+if (CI.getASTReader()) {
+  CI.getASTReader()->setDeserializationListener(nullptr);
+  /* This just sets consumer to null when DeserializationListener is null 
*/
+  CI.getASTReader()->StartTranslationUnit(nullptr);
 }

kadircet wrote:
> why are we triggering destructors for all of these objects eagerly? if this 
> is deliberate to "fix" some issue, could you mention that in comments?
> why are we triggering destructors for all of these objects eagerly? if this 
> is deliberate to "fix" some issue, could you mention that in comments?

Thanks a lot for the review.
If we don't destruct and set it to null, CapturedASTCtx will also have to keep 
instances such as ASTConsumer including other related callbacks such 
PreambleCallbacks. This was making the CapturedASTCtx interface and 
implementation complex.



Comment at: clang-tools-extra/clangd/Preamble.cpp:694
 Result->MainIsIncludeGuarded = CapturedInfo.isMainFileIncludeGuarded();
-return Result;
+CapturedCtx.emplace(CapturedInfo.takeLife());
+return std::make_pair(Result, CapturedCtx);

kadircet wrote:
> what about just keeping the callback (with a different signature) and calling 
> it here? e.g.:
> ```
> PreambleCallback(CapturedInfo.takeLife());
> ```
> 
> that way we don't need to change the return type and also control if we 
> received a valid object on every call site (since callback will only be 
> invoked on success)
> what about just keeping the callback (with a different signature) and calling 
> it here? e.g.:
> ```
> PreambleCallback(CapturedInfo.takeLife());
> ```
> 
> that way we don't need to change the return type and also control if we 
> received a valid object on every call site (since callback will only be 
> invoked on success)

Apologies for the misunderstanding.  Just to be clear, you prefer indexing in 
UpdateIndexCallbacks using the thread Tasks that also indexes in indexStdlib? 
In this implementation, I am calling the index action in preamble thread. I 
will revise it.



Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1063-1071
+  if (LatestBuild) {
+assert(CapturedCtx);
+CanonicalIncludes CanonIncludes = LatestBuild->CanonIncludes;
+CompilerInvocation  = CapturedCtx->getCompilerInvocation();
+ASTContext  = CapturedCtx->getASTContext();
+Preprocessor  = CapturedCtx->getPreprocessor();
+Callbacks.onPreambleAST(FileName, Inputs.Version, CI, Ctx, PP,

kadircet wrote:
> you can just execute this block around the call to `reportPreambleBuild` 
> right? is there any particular reason to make this part of scope cleanup ? 
> especially call it after the call to `updatePreamble` ?
> you can just execute this block around the call to `reportPreambleBuild` 
> right? is there any particular reason to make this part of scope cleanup ? 
> especially call it after the call to `updatePreamble` ?

Yes, we can do it around reportPreambleBuild. However, based on your previous 
comment, I got the impression that you prefer it be a callback to buildPreamble 
which indexes in the tasks. We could do it either way. 





Comment at: clang-tools-extra/clangd/TUScheduler.cpp:1066-1068
+CompilerInvocation  = CapturedCtx->getCompilerInvocation();
+ASTContext  = CapturedCtx->getASTContext();
+Preprocessor  = CapturedCtx->getPreprocessor();

kadircet wrote:
> these are all references to `CapturedCtx` which will be destroyed after the 
> call. i guess we should be passing shared_ptrs/IntrusiveRefCntPtrs instead, 
> right?
> 
> also I don't think it's enough to just pass these 3. we need to make sure 
> rest of the fields in CapturedCtx is also kept alive (e.g. Preprocessor has 
> raw pointers to Target/AuxTarget, which would be dangling after CapturedCtx 
> goes out of scope).
> these are all references to `CapturedCtx` which will be destroyed after the 
> call. i guess we should be passing shared_ptrs/IntrusiveRefCntPtrs instead, 
> right?
> 
> also I don't think it's enough to just pass these 3. we need to make sure 
> rest of the fields in CapturedCtx is also kept alive (e.g. Preprocessor has 
> raw pointers to Target/AuxTarget, which would be dangling after CapturedCtx 
> goes out of scope).

In this implementation, I am using these in the same scope. But I will revise 
it as per previous comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148088/new/

https://reviews.llvm.org/D148088

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

[PATCH] D148088: [RFC][clangd] Move preamble index out of document open critical path

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv updated this revision to Diff 522083.
kuganv added a comment.

Fixed tests
clangd/unittests/TUSchedulerTests.cpp require re-warite due to delayed 
onPreambleAST callback.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148088/new/

https://reviews.llvm.org/D148088

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h
  clang/include/clang/Frontend/CompilerInstance.h

Index: clang/include/clang/Frontend/CompilerInstance.h
===
--- clang/include/clang/Frontend/CompilerInstance.h
+++ clang/include/clang/Frontend/CompilerInstance.h
@@ -12,6 +12,7 @@
 #include "clang/AST/ASTConsumer.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetInfo.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
@@ -233,6 +234,8 @@
 return *Invocation;
   }
 
+  std::shared_ptr getInvocationPtr() { return Invocation; }
+
   /// setInvocation - Replace the current invocation.
   void setInvocation(std::shared_ptr Value);
 
@@ -338,6 +341,11 @@
 return *Diagnostics;
   }
 
+  IntrusiveRefCntPtr getDiagnosticsPtr() const {
+assert(Diagnostics && "Compiler instance has no diagnostics!");
+return Diagnostics;
+  }
+
   /// setDiagnostics - Replace the current diagnostics engine.
   void setDiagnostics(DiagnosticsEngine *Value);
 
@@ -373,6 +381,11 @@
 return *Target;
   }
 
+  IntrusiveRefCntPtr getTargetPtr() const {
+assert(Target && "Compiler instance has no target!");
+return Target;
+  }
+
   /// Replace the current Target.
   void setTarget(TargetInfo *Value);
 
@@ -406,6 +419,11 @@
 return *FileMgr;
   }
 
+  IntrusiveRefCntPtr getFileManagerPtr() const {
+assert(FileMgr && "Compiler instance has no file manager!");
+return FileMgr;
+  }
+
   void resetAndLeakFileManager() {
 llvm::BuryPointer(FileMgr.get());
 FileMgr.resetWithoutRelease();
@@ -426,6 +444,11 @@
 return *SourceMgr;
   }
 
+  IntrusiveRefCntPtr getSourceManagerPtr() const {
+assert(SourceMgr && "Compiler instance has no source manager!");
+return SourceMgr;
+  }
+
   void resetAndLeakSourceManager() {
 llvm::BuryPointer(SourceMgr.get());
 SourceMgr.resetWithoutRelease();
@@ -466,6 +489,11 @@
 return *Context;
   }
 
+  IntrusiveRefCntPtr getASTContextPtr() const {
+assert(Context && "Compiler instance has no AST context!");
+return Context;
+  }
+
   void resetAndLeakASTContext() {
 llvm::BuryPointer(Context.get());
 Context.resetWithoutRelease();
Index: clang-tools-extra/clangd/unittests/TestTU.h
===
--- clang-tools-extra/clangd/unittests/TestTU.h
+++ clang-tools-extra/clangd/unittests/TestTU.h
@@ -32,6 +32,8 @@
 namespace clang {
 namespace clangd {
 
+using PreambleParsedCallback = std::function;
 struct TestTU {
   static TestTU withCode(llvm::StringRef Code) {
 TestTU TU;
@@ -89,7 +91,7 @@
   // The result will always have getDiagnostics() populated.
   ParsedAST build() const;
   std::shared_ptr
-  preamble(PreambleParsedCallback PreambleCallback = nullptr) const;
+  preamble(PreambleParsedCallback = nullptr) const;
   ParseInputs inputs(MockFS ) const;
   SymbolSlab headerSymbols() const;
   RefSlab headerRefs() const;
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -107,8 +107,16 @@
 initializeModuleCache(*CI);
   auto ModuleCacheDeleter = llvm::make_scope_exit(
   std::bind(deleteModuleCache, CI->getHeaderSearchOpts().ModuleCachePath));
-  return clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs,
-  /*StoreInMemory=*/true, PreambleCallback);
+  auto res = clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs,
+  /*StoreInMemory=*/true);
+  if (PreambleCallback) {
+auto  = *res.second;
+ASTContext  = ASTCtx.getASTContext();
+Preprocessor  = ASTCtx.getPreprocessor();
+auto  = res.first->CanonIncludes;
+PreambleCallback(Ctx, PP, CanonIncludes);
+  }
+  return res.first;
 }
 
 ParsedAST TestTU::build() const {
@@ -124,8 +132,8 @@
   std::bind(deleteModuleCache, 

[PATCH] D148088: [RFC][clangd] Move preamble index task to a seperate task

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv updated this revision to Diff 521936.
kuganv edited the summary of this revision.
kuganv added a comment.
Herald added a project: clang.

Re-implemented based on review


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148088/new/

https://reviews.llvm.org/D148088

Files:
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
  clang-tools-extra/clangd/unittests/FileIndexTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h
  clang/include/clang/Frontend/CompilerInstance.h

Index: clang/include/clang/Frontend/CompilerInstance.h
===
--- clang/include/clang/Frontend/CompilerInstance.h
+++ clang/include/clang/Frontend/CompilerInstance.h
@@ -12,6 +12,7 @@
 #include "clang/AST/ASTConsumer.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TargetInfo.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PCHContainerOperations.h"
 #include "clang/Frontend/Utils.h"
@@ -233,6 +234,8 @@
 return *Invocation;
   }
 
+  std::shared_ptr getInvocationPtr() { return Invocation; }
+
   /// setInvocation - Replace the current invocation.
   void setInvocation(std::shared_ptr Value);
 
@@ -338,6 +341,11 @@
 return *Diagnostics;
   }
 
+  IntrusiveRefCntPtr getDiagnosticsPtr() const {
+assert(Diagnostics && "Compiler instance has no diagnostics!");
+return Diagnostics;
+  }
+
   /// setDiagnostics - Replace the current diagnostics engine.
   void setDiagnostics(DiagnosticsEngine *Value);
 
@@ -373,6 +381,11 @@
 return *Target;
   }
 
+  IntrusiveRefCntPtr getTargetPtr() const {
+assert(Target && "Compiler instance has no target!");
+return Target;
+  }
+
   /// Replace the current Target.
   void setTarget(TargetInfo *Value);
 
@@ -406,6 +419,11 @@
 return *FileMgr;
   }
 
+  IntrusiveRefCntPtr getFileManagerPtr() const {
+assert(FileMgr && "Compiler instance has no file manager!");
+return FileMgr;
+  }
+
   void resetAndLeakFileManager() {
 llvm::BuryPointer(FileMgr.get());
 FileMgr.resetWithoutRelease();
@@ -426,6 +444,11 @@
 return *SourceMgr;
   }
 
+  IntrusiveRefCntPtr getSourceManagerPtr() const {
+assert(SourceMgr && "Compiler instance has no source manager!");
+return SourceMgr;
+  }
+
   void resetAndLeakSourceManager() {
 llvm::BuryPointer(SourceMgr.get());
 SourceMgr.resetWithoutRelease();
@@ -466,6 +489,11 @@
 return *Context;
   }
 
+  IntrusiveRefCntPtr getASTContextPtr() const {
+assert(Context && "Compiler instance has no AST context!");
+return Context;
+  }
+
   void resetAndLeakASTContext() {
 llvm::BuryPointer(Context.get());
 Context.resetWithoutRelease();
Index: clang-tools-extra/clangd/unittests/TestTU.h
===
--- clang-tools-extra/clangd/unittests/TestTU.h
+++ clang-tools-extra/clangd/unittests/TestTU.h
@@ -32,6 +32,8 @@
 namespace clang {
 namespace clangd {
 
+using PreambleParsedCallback = std::function;
 struct TestTU {
   static TestTU withCode(llvm::StringRef Code) {
 TestTU TU;
@@ -89,7 +91,7 @@
   // The result will always have getDiagnostics() populated.
   ParsedAST build() const;
   std::shared_ptr
-  preamble(PreambleParsedCallback PreambleCallback = nullptr) const;
+  preamble(PreambleParsedCallback = nullptr) const;
   ParseInputs inputs(MockFS ) const;
   SymbolSlab headerSymbols() const;
   RefSlab headerRefs() const;
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -107,8 +107,16 @@
 initializeModuleCache(*CI);
   auto ModuleCacheDeleter = llvm::make_scope_exit(
   std::bind(deleteModuleCache, CI->getHeaderSearchOpts().ModuleCachePath));
-  return clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs,
-  /*StoreInMemory=*/true, PreambleCallback);
+  auto res = clang::clangd::buildPreamble(testPath(Filename), *CI, Inputs,
+  /*StoreInMemory=*/true);
+  if (PreambleCallback) {
+auto  = *res.second;
+ASTContext  = ASTCtx.getASTContext();
+Preprocessor  = ASTCtx.getPreprocessor();
+auto  = res.first->CanonIncludes;
+PreambleCallback(Ctx, PP, CanonIncludes);
+  }
+  return res.first;
 }
 
 ParsedAST TestTU::build() const {
@@ -124,8 +132,8 @@
   std::bind(deleteModuleCache, 

[PATCH] D148088: [RFC][clangd] Move preamble index task to a seperate task

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv added a comment.

In D148088#4316352 , @kadircet wrote:

> Sorry I was trying to give some brief idea about what it might look like in 
> `Implementation Concerns` section above, but let me give some more details;
>
> I think we can just change the signature for PreambleParsedCallback 
> 
>  to pass along refcounted objects. forgot to mention in the first comment, 
> but we should also change the CanonicalIncludes to be a shared_ptr so that it 
> can outlive the PreambleData. We should invoke the callback inside 
> buildPreamble 
> 
>  after a successful build. Afterwards we should also change the signature for 
> onPreambleAST 
> 
>  to take AST, PP and CanonicalIncludes as ref-counted objects again and 
> `PreambleThread::build` should just forward objects received from 
> `PreambleParsedCallback`. Afterwards inside the 
> UpdateIndexCallbacks::onPreambleAST 
> 
>  we can just invoke indexing on `Tasks` if it's present or synchronously in 
> the absence of it.

Thanks  @kadircet for the details.

> Does that make sense?

It does make sense. Let me work on this and get back to you.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148088/new/

https://reviews.llvm.org/D148088

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

[PATCH] D148088: [RFC][clangd] Move preamble index task to a seperate task

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv added a comment.

Thanks @kadircet , @sammccall and @ilya-biryukov for the super detailed 
explanation. As mentioned, we will test clangd with an ASAN build to test.

I also would like to get your feedback on the implementation.  After we claim 
the AST as per @sammccall's patch into LiveAST, we seem to have at least two 
options for delayed indexing.

Option 1:

- Change onPreambleAST to get LiveAST to pass it to PreambleThread
- In PreambleThread::build, as part of the scope exit

  if (!ReusedPreamble)
 // Index Preamble

Option 2:

- Change onPreambleAST to get LiveAST as value
- Start an Indexing thread
- TUScheduler can manage another thread for indexing similar to how it does 
preamble and AST thread.

Have you had any thoughts about this. Thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148088/new/

https://reviews.llvm.org/D148088

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

[PATCH] D148088: [RFC][clangd] Move preamble index task to a seperate task

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv marked an inline comment as done.
kuganv added inline comments.



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:88
+if (PreambleIndexTask)
+  PreambleIndexTask->runAsync("task:" + Path + Version,
+  std::move(Task));

ilya-biryukov wrote:
> This definitely does not work. After `onPreambleAST` returns, the AST will be 
> destroyed and async tasks may access it afterwards.
> 
Thanks for the review and suggestion. Let me rework the prototype to address 
this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148088/new/

https://reviews.llvm.org/D148088

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

[PATCH] D148088: [RFC][clangd] Move preamble index task to a seperate task

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv added a comment.

In D148088#4302092 , @ilya-biryukov 
wrote:

>> We would like to move the preamble index out of the critical path
>
> Could you provide motivation why you need to do it? What is the underlying 
> problem that this change is trying to solve?
> We rely on preamble being indexed at that particular time for correct results 
> in future steps, it's definitely not a no-op change even if the threading 
> issues are resolved (see the other comment).

Thanks for your comment. Motivation for this is:

1. We see preamble indexing taking as much as 18% of the time for some files.  
Moving  preamble indexing out of the critical path helps there.
2. We are also experimenting with preamble caching with clang modules. Early 
results from this also shows that preamble indexing outside  the critical path 
improves performance.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148088/new/

https://reviews.llvm.org/D148088

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

[PATCH] D148088: [RFC][clangd] Move preamble index task to a seperate task

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv created this revision.
Herald added subscribers: kadircet, arphaman, javed.absar.
Herald added a project: All.
kuganv updated this revision to Diff 517147.
kuganv added a comment.
kuganv edited the summary of this revision.
kuganv added reviewers: kadircet, sam, ilya-biryukov, nridge.
kuganv added subscribers: DmitryPolukhin, ivanmurashko.
kuganv published this revision for review.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang-tools-extra.

Removed wrong assert


We would like to move the preamble index out of the critical path. 
This patch is an RFC to get feedback on the correct implementation and 
potential pitfalls to keep into consideration.

I am not entirely sure if the lazy AST initialisation would create using 
Preamble AST in parallel. I tried with tsan enabled clangd but it seems to work 
OK (at least for the cases I tried)


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148088

Files:
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/TUScheduler.h

Index: clang-tools-extra/clangd/TUScheduler.h
===
--- clang-tools-extra/clangd/TUScheduler.h
+++ clang-tools-extra/clangd/TUScheduler.h
@@ -335,6 +335,10 @@
   /// Mostly useful for synchronizing tests.
   bool blockUntilIdle(Deadline D) const;
 
+  AsyncTaskRunner* getPreambleIndexTasks() {
+return PreambleIndexTasks ? () : nullptr;
+  }
+
 private:
   /// This class stores per-file data in the Files map.
   struct FileData;
@@ -371,6 +375,7 @@
   // None when running tasks synchronously and non-None when running tasks
   // asynchronously.
   std::optional PreambleTasks;
+  std::optional PreambleIndexTasks;
   std::optional WorkerThreads;
   // Used to create contexts for operations that are not bound to a particular
   // file (e.g. index queries).
Index: clang-tools-extra/clangd/TUScheduler.cpp
===
--- clang-tools-extra/clangd/TUScheduler.cpp
+++ clang-tools-extra/clangd/TUScheduler.cpp
@@ -1646,6 +1646,7 @@
   }
   if (0 < Opts.AsyncThreadsCount) {
 PreambleTasks.emplace();
+PreambleIndexTasks.emplace();
 WorkerThreads.emplace();
   }
 }
@@ -1657,6 +1658,8 @@
   // Wait for all in-flight tasks to finish.
   if (PreambleTasks)
 PreambleTasks->wait();
+  if (PreambleIndexTasks)
+PreambleIndexTasks->wait();
   if (WorkerThreads)
 WorkerThreads->wait();
 }
@@ -1668,6 +1671,9 @@
   if (PreambleTasks)
 if (!PreambleTasks->wait(D))
   return false;
+  if (PreambleIndexTasks)
+if (!PreambleIndexTasks->wait(D))
+  return false;
   return true;
 }
 
Index: clang-tools-extra/clangd/ClangdServer.cpp
===
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -61,21 +61,34 @@
 struct UpdateIndexCallbacks : public ParsingCallbacks {
   UpdateIndexCallbacks(FileIndex *FIndex,
ClangdServer::Callbacks *ServerCallbacks,
-   const ThreadsafeFS , AsyncTaskRunner *Tasks)
+   const ThreadsafeFS , AsyncTaskRunner *Tasks,
+   AsyncTaskRunner* PreambleIndexTask)
   : FIndex(FIndex), ServerCallbacks(ServerCallbacks), TFS(TFS),
-Stdlib{std::make_shared()}, Tasks(Tasks) {}
+Stdlib{std::make_shared()}, Tasks(Tasks),
+PreambleIndexTask(PreambleIndexTask) {}
 
   void onPreambleAST(PathRef Path, llvm::StringRef Version,
  const CompilerInvocation , ASTContext ,
  Preprocessor ,
  const CanonicalIncludes ) override {
+if (!FIndex)
+  return;
+
 // If this preamble uses a standard library we haven't seen yet, index it.
-if (FIndex)
-  if (auto Loc = Stdlib->add(*CI.getLangOpts(), PP.getHeaderSearchInfo()))
-indexStdlib(CI, std::move(*Loc));
+if (auto Loc = Stdlib->add(*CI.getLangOpts(), PP.getHeaderSearchInfo()))
+  indexStdlib(CI, std::move(*Loc));
 
-if (FIndex)
-  FIndex->updatePreamble(Path, Version, Ctx, PP, CanonIncludes);
+auto Task = [FIndex(FIndex), Path(Path),
+Version(Version), Ctx(), PP(),
+CanonIncludes(CanonIncludes)] {
+  FIndex->updatePreamble(Path, Version, *Ctx, *PP, CanonIncludes);
+};
+
+if (PreambleIndexTask)
+  PreambleIndexTask->runAsync("task:" + Path + Version,
+  std::move(Task));
+else
+  Task();
   }
 
   void indexStdlib(const CompilerInvocation , StdLibLocation Loc) {
@@ -139,6 +152,7 @@
   const ThreadsafeFS 
   std::shared_ptr Stdlib;
   AsyncTaskRunner *Tasks;
+  AsyncTaskRunner *PreambleIndexTask;
 };
 
 class DraftStoreFS : public ThreadsafeFS {
@@ -201,7 +215,8 @@
   WorkScheduler.emplace(CDB, TUScheduler::Options(Opts),
 

[PATCH] D144599: [clangd/index/remote]NFC: Adapt code to newer grpc/protobuf versions

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv added a comment.



> if you don't mind me asking, are you deliberately building remote-index 
> components? i.e. do you have an internal remote-index server/deployment ? 
> Asking as it'd be great to know that we've adoption here, outside of 
> ourselves.
>
> OTOH, if you're not actually using it, this shouldn't be built unless 
> `-DCLANGD_ENABLE_REMOTE=1 ` is part of the build config, hence we might have 
> a bug in the build configuration and I'd like to make sure it's fixed.

Thanks for looking into it. Yes, we use -DCLANGD_ENABLE_REMOTE=On and provide 
grpc path with -DGRPC_INSTALL_PATH.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144599/new/

https://reviews.llvm.org/D144599

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

[PATCH] D141950: [NFC] Use find_last_of when seraching for code in getRawCommentForDeclNoCacheImpl

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv added a comment.

In D141950#4141455 , @aaron.ballman 
wrote:

> LGTM! You should probably put "NFC" into the patch title when landing the 
> changes though, so it's more clear as to why there's no test coverage.
>
> Do you think this warrants adding a release note to let users know about the 
> performance improvement?

Thanks for the review. I have updated the title with NFC. This may not be 
significant enough to require update on release notes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141950/new/

https://reviews.llvm.org/D141950

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

[PATCH] D141950: Use find_last_of when seraching for code in getRawCommentForDeclNoCacheImpl

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv updated this revision to Diff 496936.
kuganv added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141950/new/

https://reviews.llvm.org/D141950

Files:
  clang/lib/AST/ASTContext.cpp


Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -349,7 +349,7 @@
 
   // There should be no other declarations or preprocessor directives between
   // comment and declaration.
-  if (Text.find_first_of(";{}#@") != StringRef::npos)
+  if (Text.find_last_of(";{}#@") != StringRef::npos)
 return nullptr;
 
   return CommentBeforeDecl;


Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -349,7 +349,7 @@
 
   // There should be no other declarations or preprocessor directives between
   // comment and declaration.
-  if (Text.find_first_of(";{}#@") != StringRef::npos)
+  if (Text.find_last_of(";{}#@") != StringRef::npos)
 return nullptr;
 
   return CommentBeforeDecl;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141950: Use find_last_of when seraching for code in getRawCommentForDeclNoCacheImpl

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv updated this revision to Diff 494870.
kuganv added a comment.

Rebasing branch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141950/new/

https://reviews.llvm.org/D141950

Files:
  clang/lib/AST/ASTContext.cpp


Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -348,7 +348,7 @@
 
   // There should be no other declarations or preprocessor directives between
   // comment and declaration.
-  if (Text.find_first_of(";{}#@") != StringRef::npos)
+  if (Text.find_last_of(";{}#@") != StringRef::npos)
 return nullptr;
 
   return CommentBeforeDecl;


Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -348,7 +348,7 @@
 
   // There should be no other declarations or preprocessor directives between
   // comment and declaration.
-  if (Text.find_first_of(";{}#@") != StringRef::npos)
+  if (Text.find_last_of(";{}#@") != StringRef::npos)
 return nullptr;
 
   return CommentBeforeDecl;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141950: Use find_last_of when seraching for code in getRawCommentForDeclNoCacheImpl

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv created this revision.
Herald added a subscriber: kadircet.
Herald added a project: All.
kuganv updated this revision to Diff 489882.
kuganv edited the summary of this revision.
kuganv added a comment.
kuganv added subscribers: DmitryPolukhin, 0x1eaf, ivanmurashko.
kuganv updated this revision to Diff 494242.
kuganv added reviewers: craig.topper, jkorous, rnk, gribozavr.
kuganv published this revision for review.
Herald added subscribers: cfe-commits, ilya-biryukov.
Herald added a project: clang.

Updating the summary.


kuganv added a comment.

Rebasing the diff to latest main


Especially in generated code with sparse comments, it takes longer
to bailout when there is code in-between. Searching from last
(with find_last_of) bails out faster.

This shows up in perf profiles with clangd in some auto-generated code.
ASTContext::getRawCommentForDeclNoCacheImpl showing as much as 18.2% in this
case. With find_last_of, this drops to 2.8%.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141950

Files:
  clang/lib/AST/ASTContext.cpp


Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -348,7 +348,7 @@
 
   // There should be no other declarations or preprocessor directives between
   // comment and declaration.
-  if (Text.find_first_of(";{}#@") != StringRef::npos)
+  if (Text.find_last_of(";{}#@") != StringRef::npos)
 return nullptr;
 
   return CommentBeforeDecl;


Index: clang/lib/AST/ASTContext.cpp
===
--- clang/lib/AST/ASTContext.cpp
+++ clang/lib/AST/ASTContext.cpp
@@ -348,7 +348,7 @@
 
   // There should be no other declarations or preprocessor directives between
   // comment and declaration.
-  if (Text.find_first_of(";{}#@") != StringRef::npos)
+  if (Text.find_last_of(";{}#@") != StringRef::npos)
 return nullptr;
 
   return CommentBeforeDecl;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124909: Fix issues with using clangd with C++ modules

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv added a comment.

@sammccall, What are the first steps that is needed for supporting modules with 
clangd. From what I see, options related to handling comments  that are 
specifically added to the compile options by clangd needs handling. This  RFC 
diff is on that note.

AFIK, since, clangd relies on clang proper for compilation action, I thought 
the module semantics would work as in normal clang compilation. I could be 
completely wrong here and  missing some details.

I am happy to look into any other requirements and iterate the diff to get it 
to where we want it to be acceptable by the community.  Any feedback welcome.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124909/new/

https://reviews.llvm.org/D124909

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

[PATCH] D124909: Fix issues with using clangd with C++ modules

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv added a comment.

Thanks a lot for the detailed. comment. Yes, you are right in that we are using 
build system (buck) to build the modules and provide as part of the CDB. This 
is fairly cheap when we hit the buck cache that is also shared by others.

You are right in that the PCM format is not stable and clangd can only consume 
modules if the build system uses the exact same version of the clang. We do 
have checks in ASTReader that validates this. I don’t see anyway we can relax 
this unless we formalise AST and guarantee full compatibility. For our use, we 
expect to use the exact same version of clang as clangd for building the PCM.

As for different modes, unless I am missing something, we should only care 
about modes that we explicitly set in clangd. We should expect the modules in 
CDB should match the options provided by CDB. We could  relax some these if 
they difference does not matter.

My main goat of this patch is to enable middles so that we can test is more 
widely.  I understand that we would find more issues that may need fixing,


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124909/new/

https://reviews.llvm.org/D124909

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

[PATCH] D124909: Fixes following incompatibility issues with c++ modules

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv created this revision.
Herald added subscribers: usaxena95, kadircet, arphaman.
Herald added a project: All.
kuganv requested review of this revision.
Herald added subscribers: cfe-commits, ilya-biryukov.
Herald added projects: clang, clang-tools-extra.

1.

Preamble generated by clangd is generated with WriteCommentListToPCH = false;
However, if we are using precompiled modules that have comments in the
serialised AST, following sanity check in the clangd fails.

// Sanity check that the comment does not come from the PCH. We choose to not
// write them into PCH, because they are racy and slow to load.
assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getBeginLoc()));

In this patch when we are generating Preamble with
WriteCommentListToPCH, we do not load the comments from AST.

1.

If we have modules that are built with RetainCommentsFromSystemHeaders
difference, that wouldn prevent modules from getting loaded. Since this
difference is not critical that should be stopping modules from being loaded,
this patch changes it to be a COMPATIBLE_LANGOPT.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124909

Files:
  clang-tools-extra/clangd/test/modules-options-compatablity-test/A.h
  clang-tools-extra/clangd/test/modules-options-compatablity-test/B.h
  
clang-tools-extra/clangd/test/modules-options-compatablity-test/compile_commands.json
  
clang-tools-extra/clangd/test/modules-options-compatablity-test/definition.jsonrpc
  
clang-tools-extra/clangd/test/modules-options-compatablity-test/module.modulemap
  clang-tools-extra/clangd/test/modules-options-compatablity-test/use.c
  clang-tools-extra/clangd/test/modules-options-compatablity.test
  clang/include/clang/Basic/LangOptions.def
  clang/lib/Serialization/ASTReader.cpp

Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -3079,6 +3079,10 @@
   return Err;
 if (llvm::Error Err = ReadBlockAbbrevs(C, COMMENTS_BLOCK_ID))
   return Err;
+if (PP.getPreprocessorOpts().GeneratePreamble &&
+!PP.getPreprocessorOpts().WriteCommentListToPCH) {
+  break;
+}
 CommentsCursors.push_back(std::make_pair(C, ));
 break;
   }
Index: clang/include/clang/Basic/LangOptions.def
===
--- clang/include/clang/Basic/LangOptions.def
+++ clang/include/clang/Basic/LangOptions.def
@@ -387,7 +387,7 @@
 
 LANGOPT(XLPragmaPack, 1, 0, "IBM XL #pragma pack handling")
 
-LANGOPT(RetainCommentsFromSystemHeaders, 1, 0, "retain documentation comments from system headers in the AST")
+COMPATIBLE_LANGOPT(RetainCommentsFromSystemHeaders, 1, 0, "retain documentation comments from system headers in the AST")
 
 LANGOPT(SanitizeAddressFieldPadding, 2, 0, "controls how aggressive is ASan "
"field padding (0: none, 1:least "
Index: clang-tools-extra/clangd/test/modules-options-compatablity.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/modules-options-compatablity.test
@@ -0,0 +1,10 @@
+
+# RUN: rm -rf %t && mkdir -p %t
+# RUN: cp -r %S/modules-options-compatablity-test/* %t
+# RUN: mkdir -p %t/prebuilt
+# RUN: sed -i -e "s|DIRECTORY|%t|g" %t/definition.jsonrpc
+# RUN: sed -i -e "s|DIRECTORY|%t|g" %t/compile_commands.json
+# RUN: %clang -cc1 -emit-module -o %t/prebuilt/A.pcm -fmodules %t/module.modulemap -fmodule-name=A
+# RUN: %clang -cc1 -fretain-comments-from-system-headers -emit-module -o %t/prebuilt/B.pcm -fmodules %t/module.modulemap -fmodule-name=B -fprebuilt-module-path=%t/prebuilt
+# RUN: rm %t/*.h
+# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc
Index: clang-tools-extra/clangd/test/modules-options-compatablity-test/use.c
===
--- /dev/null
+++ clang-tools-extra/clangd/test/modules-options-compatablity-test/use.c
@@ -0,0 +1,5 @@
+/* use */
+#include 
+#include 
+
+void use() { a(); }
Index: clang-tools-extra/clangd/test/modules-options-compatablity-test/module.modulemap
===
--- /dev/null
+++ clang-tools-extra/clangd/test/modules-options-compatablity-test/module.modulemap
@@ -0,0 +1,8 @@
+/* module.modulemap */
+module A {
+  header "A.h"
+}
+module B {
+  header "B.h"
+  export *
+}
Index: clang-tools-extra/clangd/test/modules-options-compatablity-test/definition.jsonrpc
===
--- /dev/null
+++ clang-tools-extra/clangd/test/modules-options-compatablity-test/definition.jsonrpc
@@ -0,0 +1,28 @@
+{
+  "jsonrpc": "2.0",
+"id": 0,
+"method": "initialize",
+"params": {
+  "processId": 123,
+  "rootPath": "clangd",
+  "capabilities": { 

[PATCH] D124432: Fix issues with using clangd with C++ modules

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv updated this revision to Diff 426724.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124432/new/

https://reviews.llvm.org/D124432

Files:
  clang-tools-extra/clangd/test/modules-options-compatablity-test/A.h
  clang-tools-extra/clangd/test/modules-options-compatablity-test/B.h
  
clang-tools-extra/clangd/test/modules-options-compatablity-test/compile_commands.json
  
clang-tools-extra/clangd/test/modules-options-compatablity-test/definition.jsonrpc
  
clang-tools-extra/clangd/test/modules-options-compatablity-test/module.modulemap
  clang-tools-extra/clangd/test/modules-options-compatablity-test/use.c
  clang-tools-extra/clangd/test/modules-options-compatablity.test
  clang/include/clang/Basic/LangOptions.def
  clang/lib/Serialization/ASTReader.cpp

Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -3077,6 +3077,10 @@
   return Err;
 if (llvm::Error Err = ReadBlockAbbrevs(C, COMMENTS_BLOCK_ID))
   return Err;
+if (PP.getPreprocessorOpts().GeneratePreamble &&
+!PP.getPreprocessorOpts().WriteCommentListToPCH) {
+  break;
+}
 CommentsCursors.push_back(std::make_pair(C, ));
 break;
   }
Index: clang/include/clang/Basic/LangOptions.def
===
--- clang/include/clang/Basic/LangOptions.def
+++ clang/include/clang/Basic/LangOptions.def
@@ -387,7 +387,7 @@
 
 LANGOPT(XLPragmaPack, 1, 0, "IBM XL #pragma pack handling")
 
-LANGOPT(RetainCommentsFromSystemHeaders, 1, 0, "retain documentation comments from system headers in the AST")
+COMPATIBLE_LANGOPT(RetainCommentsFromSystemHeaders, 1, 0, "retain documentation comments from system headers in the AST")
 
 LANGOPT(SanitizeAddressFieldPadding, 2, 0, "controls how aggressive is ASan "
"field padding (0: none, 1:least "
Index: clang-tools-extra/clangd/test/modules-options-compatablity.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/modules-options-compatablity.test
@@ -0,0 +1,10 @@
+
+# RUN: rm -rf %t && mkdir -p %t
+# RUN: cp -r %S/modules-options-compatablity-test/* %t
+# RUN: mkdir -p %t/prebuilt
+# RUN: sed -i -e "s|DIRECTORY|%t|g" %t/definition.jsonrpc
+# RUN: sed -i -e "s|DIRECTORY|%t|g" %t/compile_commands.json
+# RUN: %clang -cc1 -emit-module -o %t/prebuilt/A.pcm -fmodules %t/module.modulemap -fmodule-name=A
+# RUN: %clang -cc1 -fretain-comments-from-system-headers -emit-module -o %t/prebuilt/B.pcm -fmodules %t/module.modulemap -fmodule-name=B -fprebuilt-module-path=%t/prebuilt
+# RUN: rm %t/*.h
+# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc
Index: clang-tools-extra/clangd/test/modules-options-compatablity-test/use.c
===
--- /dev/null
+++ clang-tools-extra/clangd/test/modules-options-compatablity-test/use.c
@@ -0,0 +1,5 @@
+/* use */
+#include 
+#include 
+
+void use() { a(); }
Index: clang-tools-extra/clangd/test/modules-options-compatablity-test/module.modulemap
===
--- /dev/null
+++ clang-tools-extra/clangd/test/modules-options-compatablity-test/module.modulemap
@@ -0,0 +1,8 @@
+/* module.modulemap */
+module A {
+  header "A.h"
+}
+module B {
+  header "B.h"
+  export *
+}
Index: clang-tools-extra/clangd/test/modules-options-compatablity-test/definition.jsonrpc
===
--- /dev/null
+++ clang-tools-extra/clangd/test/modules-options-compatablity-test/definition.jsonrpc
@@ -0,0 +1,28 @@
+{
+  "jsonrpc": "2.0",
+"id": 0,
+"method": "initialize",
+"params": {
+  "processId": 123,
+  "rootPath": "clangd",
+  "capabilities": { "window": { "workDoneProgress": true, "implicitWorkDoneProgressCreate": true} },
+  "trace": "off"
+}
+}
+---
+{
+  "jsonrpc": "2.0",
+"method": "textDocument/didOpen",
+"params": {
+  "textDocument": {
+"uri": "file://DIRECTORY/use.c",
+"languageId": "cpp",
+"version": 1,
+"text": "#include \"B.h\"\nvoid use(){\na();\n}"
+  }
+}
+}
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/clangd/test/modules-options-compatablity-test/compile_commands.json
===
--- /dev/null
+++ clang-tools-extra/clangd/test/modules-options-compatablity-test/compile_commands.json
@@ -0,0 +1,8 @@
+[{
+  "directory" : ".",
+  "command" : "clang -emit-obj DIRECTORY/use.c -fno-modules-global -fmodules "
+  "-fmodules-cache-path=/DOES/NOT/EXIST  -fno-builtin-module-map "
+  "-fno-implicit-modules  

[PATCH] D124432: Fix issues with using clangd with C++ modules

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv updated this revision to Diff 426690.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124432/new/

https://reviews.llvm.org/D124432

Files:
  clang-tools-extra/clangd/test/modules-options-compatablity-test/A.h
  clang-tools-extra/clangd/test/modules-options-compatablity-test/B.h
  
clang-tools-extra/clangd/test/modules-options-compatablity-test/compile_commands.json
  
clang-tools-extra/clangd/test/modules-options-compatablity-test/definition.jsonrpc
  
clang-tools-extra/clangd/test/modules-options-compatablity-test/module.modulemap
  clang-tools-extra/clangd/test/modules-options-compatablity-test/use.c
  clang-tools-extra/clangd/test/modules-options-compatablity.test
  clang/include/clang/Basic/LangOptions.def
  clang/lib/Serialization/ASTReader.cpp

Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -3077,6 +3077,10 @@
   return Err;
 if (llvm::Error Err = ReadBlockAbbrevs(C, COMMENTS_BLOCK_ID))
   return Err;
+if (PP.getPreprocessorOpts().GeneratePreamble &&
+!PP.getPreprocessorOpts().WriteCommentListToPCH) {
+  break;
+}
 CommentsCursors.push_back(std::make_pair(C, ));
 break;
   }
Index: clang/include/clang/Basic/LangOptions.def
===
--- clang/include/clang/Basic/LangOptions.def
+++ clang/include/clang/Basic/LangOptions.def
@@ -387,7 +387,7 @@
 
 LANGOPT(XLPragmaPack, 1, 0, "IBM XL #pragma pack handling")
 
-LANGOPT(RetainCommentsFromSystemHeaders, 1, 0, "retain documentation comments from system headers in the AST")
+COMPATIBLE_LANGOPT(RetainCommentsFromSystemHeaders, 1, 0, "retain documentation comments from system headers in the AST")
 
 LANGOPT(SanitizeAddressFieldPadding, 2, 0, "controls how aggressive is ASan "
"field padding (0: none, 1:least "
Index: clang-tools-extra/clangd/test/modules-options-compatablity.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/modules-options-compatablity.test
@@ -0,0 +1,10 @@
+
+# RUN: rm -rf %t && mkdir -p %t
+# RUN: cp -r %S/modules-options-compatablity-test/* %t
+# RUN: mkdir -p %t/prebuilt
+# RUN: sed -i -e "s|DIRECTORY|%t|g" %t/definition.jsonrpc
+# RUN: sed -i -e "s|DIRECTORY|%t|g" %t/compile_commands.json
+# RUN: %clang -cc1 -emit-module -o %t/prebuilt/A.pcm -fmodules %t/module.modulemap -fmodule-name=A
+# RUN: %clang -cc1 -fretain-comments-from-system-headers -emit-module -o %t/prebuilt/B.pcm -fmodules %t/module.modulemap -fmodule-name=B -fprebuilt-module-path=%t/prebuilt
+# RUN: rm %t/*.h
+# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc
Index: clang-tools-extra/clangd/test/modules-options-compatablity-test/use.c
===
--- /dev/null
+++ clang-tools-extra/clangd/test/modules-options-compatablity-test/use.c
@@ -0,0 +1,5 @@
+/* use */
+#include 
+#include 
+
+void use() { a(); }
Index: clang-tools-extra/clangd/test/modules-options-compatablity-test/module.modulemap
===
--- /dev/null
+++ clang-tools-extra/clangd/test/modules-options-compatablity-test/module.modulemap
@@ -0,0 +1,8 @@
+/* module.modulemap */
+module A {
+  header "A.h"
+}
+module B {
+  header "B.h"
+  export *
+}
Index: clang-tools-extra/clangd/test/modules-options-compatablity-test/definition.jsonrpc
===
--- /dev/null
+++ clang-tools-extra/clangd/test/modules-options-compatablity-test/definition.jsonrpc
@@ -0,0 +1,28 @@
+{
+  "jsonrpc": "2.0",
+"id": 0,
+"method": "initialize",
+"params": {
+  "processId": 123,
+  "rootPath": "clangd",
+  "capabilities": { "window": { "workDoneProgress": true, "implicitWorkDoneProgressCreate": true} },
+  "trace": "off"
+}
+}
+---
+{
+  "jsonrpc": "2.0",
+"method": "textDocument/didOpen",
+"params": {
+  "textDocument": {
+"uri": "file://DIRECTORY/use.c",
+"languageId": "cpp",
+"version": 1,
+"text": "#include \"B.h\"\nvoid use(){\na();\n}"
+  }
+}
+}
+---
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
+---
+{"jsonrpc":"2.0","method":"exit"}
Index: clang-tools-extra/clangd/test/modules-options-compatablity-test/compile_commands.json
===
--- /dev/null
+++ clang-tools-extra/clangd/test/modules-options-compatablity-test/compile_commands.json
@@ -0,0 +1,5 @@
+[{
+  "directory": ".",
+  "command": "clang -emit-obj DIRECTORY/use.c -fno-modules-global -fmodules -fmodules-cache-path=/DOES/NOT/EXIST  -fno-builtin-module-map -fno-implicit-modules  -fimplicit-module-maps 

[PATCH] D124432: Fix issues with using clangd with C++ modules

[Kugan Vivekanandarajah via Phabricator via cfe-commits]

kuganv created this revision.
Herald added subscribers: dexonsmith, usaxena95, kadircet, arphaman.
Herald added a project: All.
kuganv requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added projects: clang, clang-tools-extra.

Fixes following incompatibility issues with c++ modules
(clangd with c++ modules results in assertion failure and pch_langopt_mismatch 
#1105)

1. Preamble generated by clangd is generated with WriteCommentListToPCH = false;

However, if we are using precompiled modules that have comments in the
serialised AST, following sanity check in the clangd fails.

// Sanity check that the comment does not come from the PCH. We choose to not
// write them into PCH, because they are racy and slow to load.
assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC→getBeginLoc()));

In this patch when we are generating Preamble with
WriteCommentListToPCH, we do not load the comments from AST.

2. If we have modules that are built with RetainCommentsFromSystemHeaders

difference, that would prevent modules from getting loaded. Since this
difference is not critical that should be stopping modules from being loaded,
this patch changes it to be a COMPATIBLE_LANGOPT.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124432

Files:
  clang-tools-extra/clangd/test/modules-options-compatablity-test/A.h
  clang-tools-extra/clangd/test/modules-options-compatablity-test/B.h
  
clang-tools-extra/clangd/test/modules-options-compatablity-test/compile_commands.json
  
clang-tools-extra/clangd/test/modules-options-compatablity-test/definition.jsonrpc
  
clang-tools-extra/clangd/test/modules-options-compatablity-test/module.modulemap
  clang-tools-extra/clangd/test/modules-options-compatablity-test/use.c
  clang-tools-extra/clangd/test/modules-options-compatablity.test
  clang/include/clang/Basic/LangOptions.def
  clang/lib/Serialization/ASTReader.cpp

Index: clang/lib/Serialization/ASTReader.cpp
===
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -3077,6 +3077,10 @@
   return Err;
 if (llvm::Error Err = ReadBlockAbbrevs(C, COMMENTS_BLOCK_ID))
   return Err;
+if (PP.getPreprocessorOpts().GeneratePreamble &&
+!PP.getPreprocessorOpts().WriteCommentListToPCH) {
+  break;
+}
 CommentsCursors.push_back(std::make_pair(C, ));
 break;
   }
Index: clang/include/clang/Basic/LangOptions.def
===
--- clang/include/clang/Basic/LangOptions.def
+++ clang/include/clang/Basic/LangOptions.def
@@ -387,7 +387,7 @@
 
 LANGOPT(XLPragmaPack, 1, 0, "IBM XL #pragma pack handling")
 
-LANGOPT(RetainCommentsFromSystemHeaders, 1, 0, "retain documentation comments from system headers in the AST")
+COMPATIBLE_LANGOPT(RetainCommentsFromSystemHeaders, 1, 0, "retain documentation comments from system headers in the AST")
 
 LANGOPT(SanitizeAddressFieldPadding, 2, 0, "controls how aggressive is ASan "
"field padding (0: none, 1:least "
Index: clang-tools-extra/clangd/test/modules-options-compatablity.test
===
--- /dev/null
+++ clang-tools-extra/clangd/test/modules-options-compatablity.test
@@ -0,0 +1,10 @@
+
+# RUN: rm -rf %t && mkdir -p %t
+# RUN: cp -r %S/modules-options-compatablity-test/* %t
+# RUN: mkdir -p %t/prebuilt
+# RUN: sed -i -e "s|DIRECTORY|%t|g" %t/definition.jsonrpc
+# RUN: sed -i -e "s|DIRECTORY|%t|g" %t/compile_commands.json
+# RUN: %clang -cc1 -emit-module -o %t/prebuilt/A.pcm -fmodules %t/module.modulemap -fmodule-name=A
+# RUN: %clang -cc1 -fretain-comments-from-system-headers -emit-module -o %t/prebuilt/B.pcm -fmodules %t/module.modulemap -fmodule-name=B -fprebuilt-module-path=%t/prebuilt
+# RUN: rm %t/*.h
+# RUN: clangd -background-index -lit-test < %t/definition.jsonrpc
Index: clang-tools-extra/clangd/test/modules-options-compatablity-test/use.c
===
--- /dev/null
+++ clang-tools-extra/clangd/test/modules-options-compatablity-test/use.c
@@ -0,0 +1,5 @@
+/* use */
+#include 
+#include 
+
+void use() { a(); }
Index: clang-tools-extra/clangd/test/modules-options-compatablity-test/module.modulemap
===
--- /dev/null
+++ clang-tools-extra/clangd/test/modules-options-compatablity-test/module.modulemap
@@ -0,0 +1,8 @@
+/* module.modulemap */
+module A {
+  header "A.h"
+}
+module B {
+  header "B.h"
+  export *
+}
Index: clang-tools-extra/clangd/test/modules-options-compatablity-test/definition.jsonrpc
===
--- /dev/null
+++ clang-tools-extra/clangd/test/modules-options-compatablity-test/definition.jsonrpc
@@ -0,0 +1,28 @@
+{