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<unsigned long, 64u> 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<clang::ASTRecordReader>::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
> > clang::ASTReader::FindExternalLexicalDecls(clang::DeclContext const*,
> > llvm::function_ref<bool (clang::Decl::Kind)>,
> > llvm::SmallVectorImpl<clang::Decl*>&)::$_9::operator()(clang::serializ
> > ation::ModuleFile*,
> > llvm::ArrayRef<llvm::support::detail::packed_endian_specific_integral<unsigned
> > int, (llvm::support::endianness)2, 1ul, 1ul>>) const
> > /home/kugan/local/llvm-project/clang/lib/Serialization/ASTRe
> > ader.cpp:7687:21
> > #11 0x558285bfd741 in
> > clang::ASTReader::FindExternalLexicalDecls(clang::DeclContext const*,
> > llvm::function_ref<bool (clang::Decl::Kind)>,
> > llvm::SmallVectorImpl<clang::Decl*>&) /home/kugan/local/llvm-project/c
> > lang/lib/Serialization/ASTReader.cpp:7697:7
> > #12 0x55827e942de7 in
> > clang::ExternalASTSource::FindExternalLexicalDecls(clang::DeclContext
> > const*, llvm::SmallVectorImpl<clang::Decl*>&)
> > /home/kugan/local/llvm-project/clang/include/clang/AST/ExternalASTSour
> > ce.h:185:5
> > #13 0x55827e942de7 in
> > clang::DeclContext::LoadLexicalDeclsFromExternalStorage() const
> > /home/kugan/local/llvm-project/clang/lib/AST/DeclBase.cpp:1421:11
> > #14 0x55827e94436b in clang::DeclContext::decls_begin() const
> > /home/kugan/local/llvm-project/clang/lib/AST/DeclBase.cpp:1477:5
> > #15 0x5582822787ab in clang::DeclContext::decls() const
> > /home/kugan/local/llvm-project/clang/include/clang/AST/DeclBase.h:2188:48
> > #16 0x5582822787ab in
> > clang::clangd::indexHeaderSymbols(llvm::StringRef, clang::ASTContext&,
> > clang::Preprocessor&, clang::clangd::CanonicalIncludes const&)
> > /home/kugan/local/llvm-project/clang-tools-extra/cla
> > ngd/index/FileIndex.cpp:233:37
> > #17 0x558282281be9 in
> > clang::clangd::FileIndex::updatePreamble(llvm::StringRef, llvm::StringRef,
> > clang::ASTContext&, clang::Preprocessor&, clang::clangd::CanonicalIncludes
> > const&) /home/kugan/local/llvm-proje
> > ct/clang-tools-extra/clangd/index/FileIndex.cpp:464:7
> > ```
> >
> >
> >
> ah, this one is unfortunately a little bit more involved, and triggers only
> in the cases where we have modules enabled (which is incidentally working in
> clangd today, but effect is wider as -std=c++20 implicitly turns on
> -fmodules).
>
> tl;dr; astcontext is keeping references to pchgenerator in module-enabled
> builds.
>
> TBH the safest option here seems like clearing external source in astcontext
> completely. As we can't really risk callbacks to PCHGenerator, even if we
> fixed all the life-time concerns (ast consumer keeps references to
> frontendaction alive, so both the callbacks we pass here and the action
> itself needs to outlive the consumer).
>
> But that's likely going to break preamble-indexing completely for codebases
> that use modules (we'll likely only index top-level decls from the current
> modules, but I am not sure). The alternative suggested here strikes some
> middle ground (there's still going to be behavior change, previously all the
> modules accessed during indexing would effect the final serialized preamble,
> they won't anymore) but I am afraid it's too fragile. The dependency between
> these clang-internals were so subtle that it took ~hours for me to analyze,
> despite me specifically looking for them, hence I don't think anyone that's
> unaware can preserve these invariants (i.e. make sure we don't have
> references to astconsumer in other places.).
>
> I am not sure how these work for you today, but can you check if just setting
> external source on the astcontext to null is good enough?
Thanks for checking. Unfortunately, just setting external source on the
astcontext to null is not working. Setting
PrecompilePreambleConsumer/PCHGenerator and ASTMutationListener to null seem
to be sufficient.
I checked the Preamble index size with and without the patch for some files and
they are the same. Updating to the revised patch.
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
