<div dir="ltr">Hi Jaroslav,<div><br></div><div>Thank you for digging into this. I think it may be possible to not eagerly import a RecordDecl.  Perhaps we could control that from clang::ASTNodeImporter::VisitPointerType and instead of a full import we could just do a minimal import for the PointeeType always. Similarly to references.</div><div>But I am not sure about whether we would know during a subsequent import (where the full type is really needed) that the type had been minimally imported before. This requires some more investigation, I'll come back to you what I find, but it will take some time as this week is quite busy for me.</div><div><br></div><div>Gabor</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Nov 12, 2019 at 11:14 AM Jaroslav Sevcik <<a href="mailto:jarin@google.com">jarin@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br>Hi,<br><br>thanks for the feedback, I did some more investigation and now I understand<br>a bit better where the LayoutRecordType calls are coming from. They are initiated by<br>Clang's code generation for types, which insists on generating full LLVM types<br>for any complete RecordDecl (see<br>    <a href="https://github.com/llvm-mirror/clang/blob/65acf43270ea2894dffa0d0b292b92402f80c8cb/lib/CodeGen/CodeGenTypes.cpp#L752" target="_blank">https://github.com/llvm-mirror/clang/blob/65acf43270ea2894dffa0d0b292b92402f80c8cb/lib/CodeGen/CodeGenTypes.cpp#L752</a>,<br>    the bailout for incomplete record decls is at<br>    <a href="https://github.com/llvm-mirror/clang/blob/master/lib/CodeGen/CodeGenTypes.cpp#L729" target="_blank">https://github.com/llvm-mirror/clang/blob/master/lib/CodeGen/CodeGenTypes.cpp#L729</a>).<br>Ideally, clang would only emit forward declarations for any type that is only<br>used as a pointer, but that is not the case at the moment. It is not quite clear<br>what else we could do on the lldb side to prevent clang from emitting types for all<br>those structs. At the end of this email, I am pasting a stack trace that illustrates <br>the recursive emit for a pointer-chain of structs.<br><br>There is also one additional problem with importing base classes. This one is perhaps<br>more related to AST importer. Again this is best illustrated with an example.<br><br>struct F1 { int f1 = 30; };<br>struct F0 { int f0 = 31; };<br><br>struct B0 {<br>  F0 b0f;<br>};<br><br>struct C0 : B0 {<br>};<br><br>struct B1 { <br>  F1 b1f;<br>  int f(C0* c0); <br>};<br><br>struct C1 : B1 {};<br>struct C2 { int x = 4; C1* c1 = 0; };<br><br>int main() {<br>  C0 c0;<br>  C1 c1;<br>  C2 c2;<br><br>  return 0;  // break here<br>}<br><br>If we evaluate c2.x in lldb after evaluating c1 and c0, the AST importer<br>unnecessarily imports all the classes in this file. The main reason for this is<br>that the import for structs needs to set the DefaultedDestructorIsConstexpr<br>bit, which is computed by looking at destructors of base classes. Looking at<br>the destructors triggers completion and import of the base classes, which in<br>turn might trigger more imports of base classes. In the example above,<br>importing C1 will cause completion and import of B1, which in turn causes<br>import of C0, and recursively completion and import of B0. Below is a<br>stack trace showing import of field F0::f0 from evaluation of c2.x. The<br>stack traces going through the<br>VisitRecordDecl-->ImportDefinition-->setBases path appear to be very hot<br>in the workloads we see (debugging large codebases). Below is a stack<br>trace that illustrates visiting F0::f0 from the evaluation of c2.x.  The<br>question is what is the right place to break this long chain of imports.<br>Ideally, we would not have to import fields of C1 or B1 when evaluating<br>c2.x. Any ideas?</div><div dir="ltr"><br>Stack trace visiting F0::f0 from eval of c2.x, I have tried to highlight<br>what is imported/completed where.<br><br>clang::ASTNodeImporter::VisitFieldDecl                ;; Visit F0::f0<br>clang::declvisitor::Base<...>::Visit<br>clang::ASTImporter::ImportImpl<br>lldb_private::ClangASTImporter::ASTImporterDelegate::ImportImpl<br>clang::ASTImporter::Import<br>lldb_private::ClangASTImporter::CopyDecl<br>lldb_private::ClangASTSource::CopyDecl<br>lldb_private::ClangASTSource::FindExternalLexicalDecls<br>lldb_private::ClangASTSource::ClangASTSourceProxy::FindExternalLexicalDecls<br>clang::ExternalASTSource::FindExternalLexicalDecls<br>clang::DeclContext::LoadLexicalDeclsFromExternalStorage const<br>clang::DeclContext::buildLookup                       ;; lookup destructor of B0<br>clang::DeclContext::lookup const<br>clang::CXXRecordDecl::getDestructor const<br>clang::CXXRecordDecl::hasConstexprDestructor const<br>clang::CXXRecordDecl::addedClassSubobject<br>clang::CXXRecordDecl::addedMember<br>clang::DeclContext::addHiddenDecl<br>clang::DeclContext::addDeclInternal<br>clang::ASTNodeImporter::VisitFieldDecl                ;; visit B0::b0f<br>clang::declvisitor::Base<...>::Visit<br>clang::ASTImporter::ImportImpl<br>lldb_private::ClangASTImporter::ASTImporterDelegate::ImportImpl<br>clang::ASTImporter::Import<br>lldb_private::ClangASTImporter::CopyDecl<br>lldb_private::ClangASTSource::CopyDecl<br>lldb_private::ClangASTSource::FindExternalLexicalDecls<br>lldb_private::ClangASTSource::ClangASTSourceProxy::FindExternalLexicalDecls<br>clang::RecordDecl::LoadFieldsFromExternalStorage const<br>clang::RecordDecl::field_begin const<br>clang::RecordDecl::field_empty const                  ;; fields of B0<br>clang::CXXRecordDecl::setBases                        <br>clang::ASTNodeImporter::ImportDefinition<br>clang::ASTNodeImporter::VisitRecordDecl               ;; visit C0<br>clang::declvisitor::Base<...>::VisitCXXRecordDecl<br>clang::declvisitor::Base<...>::Visit<br>clang::ASTImporter::ImportImpl<br>lldb_private::ClangASTImporter::ASTImporterDelegate::ImportImpl<br>clang::ASTImporter::Import<br>llvm::Expected<clang::RecordDecl*> clang::ASTNodeImporter::import<clang::RecordDecl><br>clang::ASTNodeImporter::VisitRecordType<br>clang::TypeVisitor<clang::ASTNodeImporter, llvm::Expected<clang::QualType> >::Visit<br>clang::ASTImporter::Import<br>llvm::Expected<clang::QualType> clang::ASTNodeImporter::import<clang::QualType><br>clang::ASTNodeImporter::VisitPointerType              ;; visit pointer to C0<br>clang::TypeVisitor<clang::ASTNodeImporter, llvm::Expected<clang::QualType> >::Visit<br>clang::ASTImporter::Import<br>llvm::Expected<clang::QualType> clang::ASTNodeImporter::import<clang::QualType><br>clang::ASTNodeImporter::VisitFunctionProtoType<br>clang::TypeVisitor<clang::ASTNodeImporter, llvm::Expected<clang::QualType> >::Visit<br>clang::ASTImporter::Import<br>llvm::Expected<ang::QualType> clang::ASTNodeImporter::import<clang::QualType><br>llvm::Expected<...> clang::ASTNodeImporter::importSeq<...><br>clang::ASTNodeImporter::VisitFunctionDecl<br>clang::ASTNodeImporter::VisitCXXMethodDecl            ;; visit B1::f<br>clang::declvisitor::Base<...>::Visit<br>clang::ASTImporter::ImportImpl<br>lldb_private::ClangASTImporter::ASTImporterDelegate::ImportImpl<br>clang::ASTImporter::Import<br>lldb_private::ClangASTImporter::CopyDecl<br>lldb_private::ClangASTSource::CopyDecl<br>lldb_private::ClangASTSource::FindExternalLexicalDecls<br>lldb_private::ClangASTSource::ClangASTSourceProxy::FindExternalLexicalDecls<br>clang::ExternalASTSource::FindExternalLexicalDecls<br>clang::DeclContext::LoadLexicalDeclsFromExternalStorage const<br>clang::DeclContext::buildLookup<br>clang::DeclContext::lookup const                      ;; lookup destructor of B1<br>clang::CXXRecordDecl::getDestructor const       <br>clang::CXXRecordDecl::hasConstexprDestructor const<br>clang::CXXRecordDecl::addedClassSubobject<br>clang::CXXRecordDecl::setBases                        ;; B1<br>clang::ASTNodeImporter::ImportDefinition<br>clang::ASTNodeImporter::VisitRecordDecl               ;; visit C1<br>clang::declvisitor::Base<...>::VisitCXXRecordDecl<br>clang::declvisitor::Base<...>::Visit<br>clang::ASTImporter::ImportImpl<br>lldb_private::ClangASTImporter::ASTImporterDelegate::ImportImpl<br>clang::ASTImporter::Import<br>llvm::Expected<clang::RecordDecl*> clang::ASTNodeImporter::import<clang::RecordDecl><br>clang::ASTNodeImporter::VisitRecordType<br>clang::TypeVisitor<clang::ASTNodeImporter, llvm::Expected<clang::QualType> >::Visit<br>clang::ASTImporter::Import<br>llvm::Expected<clang::QualType> clang::ASTNodeImporter::import<clang::QualType><br>clang::ASTNodeImporter::VisitPointerType<br>clang::TypeVisitor<clang::ASTNodeImporter, llvm::Expected<clang::QualType> >::Visit<br>clang::ASTImporter::Import<br>llvm::Expected<clang::QualType> clang::ASTNodeImporter::import<clang::QualType><br>llvm::Expected<...> clang::ASTNodeImporter::importSeq<...><br>llvm::Expected<...> clang::ASTNodeImporter::importSeq<...><br>clang::ASTNodeImporter::VisitFieldDecl                 ;; visit C2::c1<br>clang::declvisitor::Base<...>::Visit<br>clang::ASTImporter::ImportImpl<br>lldb_private::ClangASTImporter::ASTImporterDelegate::ImportImpl<br>clang::ASTImporter::Import<br>llvm::Expected<clang::Decl*> clang::ASTNodeImporter::import<clang::Decl><br>clang::ASTNodeImporter::ImportDeclContext<br>clang::ASTImporter::ImportDefinition<br>lldb_private::ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo<br>lldb_private::ClangASTImporter::CompleteTagDecl<br>lldb_private::ClangASTSource::CompleteType             ;; complete C2<br>lldb_private::ClangExpressionDeclMap::AddOneVariable<br>lldb_private::ClangExpressionDeclMap::FindExternalVisibleDecls<br>lldb_private::ClangExpressionDeclMap::FindExternalVisibleDecls<br>lldb_private::ClangASTSource::FindExternalVisibleDeclsByName<br>lldb_private::ClangASTSource::ClangASTSourceProxy::FindExternalVisibleDeclsByName<br>clang::DeclContext::lookup const<br>LookupDirect<br>CppNamespaceLookup<br>clang::Sema::CppLookupName<br>clang::Sema::LookupName<br>clang::Sema::BuildUsingDeclaration<br>...<br><br><br>Here is a stack trace that illustrates code gen of types for chain of<br>references.  This was obtain by evaluating c2.x after evaluating c1.x and c0.x<br>in the following program.<br><br>struct C0 { int x = 0; };<br>struct C1 { int x = 1; C0* c0 = 0; };<br>struct C2 { int x = 2; C1* c1 = 0; };<br><br>int main() {<br>  C0 c0;<br>  C1 c1;<br>  C2 c2;<br><br>  return 0;  // break here<br>}<br><br><br>...<br>lldb_private::ClangASTSource::layoutRecordType<br>lldb_private::ClangASTSource::ClangASTSourceProxy::layoutRecordType<br>anonymous namespace)::ItaniumRecordLayoutBuilder::InitializeLayout<br>anonymous namespace)::ItaniumRecordLayoutBuilder::Layout<br>lang::ASTContext::getASTRecordLayout<br>anonymous namespace)::CGRecordLowering::CGRecordLowering<br>anonymous namespace)::CGRecordLowering::CGRecordLowering<br>lang::CodeGen::CodeGenTypes::ComputeRecordLayout<br>lang::CodeGen::CodeGenTypes::ConvertRecordDeclType<br>lang::CodeGen::CodeGenTypes::ConvertType          ;; convert C0<br>clang::CodeGen::CodeGenTypes::ConvertTypeForMem<br>clang::CodeGen::CodeGenTypes::ConvertType         ;; convert ptr to C0<br>clang::CodeGen::CodeGenTypes::ConvertTypeForMem<br>(anonymous namespace)::CGRecordLowering::getStorageType<br>(anonymous namespace)::CGRecordLowering::accumulateFields<br>(anonymous namespace)::CGRecordLowering::lower<br>clang::CodeGen::CodeGenTypes::ComputeRecordLayout<br>clang::CodeGen::CodeGenTypes::ConvertRecordDeclType<br>clang::CodeGen::CodeGenTypes::ConvertType         ;; convert C1<br>clang::CodeGen::CodeGenTypes::ConvertTypeForMem<br>clang::CodeGen::CodeGenTypes::ConvertType         ;; convert ptr to C1<br>clang::CodeGen::CodeGenTypes::ConvertTypeForMem<br>clang::CodeGen::CodeGenModule::GetAddrOfGlobalVar<br>...<br><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Nov 8, 2019 at 9:34 PM Jaroslav Sevcik <<a href="mailto:jarin@google.com" target="_blank">jarin@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>Hi Gabor,</div><div><br></div><div>you are right, there is something funny going on the LLDB side. Let me investigate, I will let you know once I know some more.</div><div><br></div><div>Cheers, Jaro</div><div><br></div><div><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Nov 7, 2019 at 6:14 PM Gábor Márton <<a href="mailto:martongabesz@gmail.com" target="_blank">martongabesz@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hi Jaroslav,<div><br></div><div>Thanks for working on this. Still, there are things that are unclear for me.</div><div>I see that `LayoutRecordType` is called superfluously during the second evaluation of `c2.x`, ok.</div><div><br></div><div>But, could you explain why LLDB is calling that multiple times? Maybe it thinks a type is not completed while it is? As far as I know we keep track which RecordDecl needs completeion in LLDB. At least, we do not store that info in clang::ASTImporter. Minimal import gives a CXXRecordDecl whose `DefinitionData` is set, even though the members are not imported the definition data is there! So, there is no way for clang::ASTImporter to know which RecordDecl had been imported in a minimal way.</div><div><br></div><div>I suspect there are multiple invocations of ASTImporter::ImportDefinition with C2, C1, C0 within ClangASTSource (could you please check that?). And `ImportDefinition` will blindly import the full definitions recursively even if the minimal import is set (see ASTNodeImporter::IDK_Everything). The patch would change this behavior which I'd like to avoid if possible. I think first we should discover why there are multiple invocations of ASTImporter::ImportDefinition from within LLDB.</div><div><br></div><div>Gabor</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Nov 6, 2019 at 11:21 PM Raphael “Teemperor” Isemann via lldb-dev <<a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Can you post that patch on Phabricator with an '[ASTImporter]’ as a name prefix? This way the ASTImporter folks will see your patch (The ASTImporter is more of a Clang thing, so they might not read lldb-dev). Also it makes it easier to see/test your patch :)<br>
<br>
(And +Gabor just in case)<br>
<br>
> On Nov 6, 2019, at 10:25 PM, Jaroslav Sevcik via lldb-dev <<a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a>> wrote:<br>
> <br>
> Hello,<br>
> <br>
> I noticed that the AST importer is very eager to import classes/structs that were already completed, even if they are not needed. This is best illustrated with an example.  <br>
> <br>
> struct C0 { int x = 0; };<br>
> struct C1 { int x = 1; C0* c0 = 0; };<br>
> struct C2 { int x = 2; C1* c1 = 0; };<br>
> <br>
> int main() {<br>
>   C0 c0;<br>
>   C1 c1;<br>
>   C2 c2;<br>
> <br>
>   return 0;  // break here<br>
> }<br>
> <br>
> When we evaluate “c2.x” in LLDB, AST importer completes and imports only class C2. This is working as intended. Similarly, evaluating “c1.x” imports just C1 and “c0.x” imports C0. However, if we evaluate “c2.x” after evaluating “c1.x” and “c0.x”, the importer suddenly imports both C1 and C0 (in addition to C2). See a log from a lldb session at the end of this email for illustration.<br>
> <br>
> I believe the culprit here is the following code at the end of the ASTNodeImporter::VisitRecordDecl method:<br>
> <br>
>   if (D->isCompleteDefinition())<br>
>     if (Error Err = ImportDefinition(D, D2, IDK_Default))<br>
>       return std::move(Err);<br>
> <br>
> This will import a definition of class from LLDB if LLDB already happens to have a complete definition from before. For large programs, this can lead to importing very large chunks of ASTs even if they are not needed. I have tried to remove the code above from clang and test performance on several expressions in an Unreal engine sample - preliminary results show this could cut down evaluation time by roughly 50%. <br>
> <br>
> My prototype patch is below; note that couple of lldb tests are failing with a wrong error message, this is a work in progress. What would the experts here think? Is this a plausible direction?<br>
> <br>
> Cheers, Jaro<br>
> <br>
> diff --git a/clang/lib/AST/ASTImporter.cpp b/clang/lib/AST/ASTImporter.cpp<br>
> index 54acca7dc62..ebbce5c66c7 100644<br>
> --- a/clang/lib/AST/ASTImporter.cpp<br>
> +++ b/clang/lib/AST/ASTImporter.cpp<br>
> @@ -2799,7 +2799,7 @@ ExpectedDecl ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {<br>
>    if (D->isAnonymousStructOrUnion())<br>
>      D2->setAnonymousStructOrUnion(true);<br>
> <br>
> -  if (D->isCompleteDefinition())<br>
> +  if (!Importer.isMinimalImport() && D->isCompleteDefinition())<br>
>      if (Error Err = ImportDefinition(D, D2, IDK_Default))<br>
>        return std::move(Err);<br>
> <br>
> @@ -3438,6 +3438,9 @@ ExpectedDecl ASTNodeImporter::VisitFieldDecl(FieldDecl *D) {<br>
>    if (ToInitializer)<br>
>      ToField->setInClassInitializer(ToInitializer);<br>
>    ToField->setImplicit(D->isImplicit());<br>
> +  if (CXXRecordDecl *FieldType = D->getType()->getAsCXXRecordDecl())<br>
> +       if (Error Err = ImportDefinitionIfNeeded(FieldType))<br>
> +         return std::move(Err);<br>
>    LexicalDC->addDeclInternal(ToField);<br>
>    return ToField;<br>
>  }<br>
> @@ -5307,7 +5310,7 @@ ExpectedDecl ASTNodeImporter::VisitClassTemplateSpecializationDecl(<br>
> <br>
>    D2->setTemplateSpecializationKind(D->getTemplateSpecializationKind());<br>
> <br>
> -  if (D->isCompleteDefinition())<br>
> +  if (!Importer.isMinimalImport() && D->isCompleteDefinition())<br>
>      if (Error Err = ImportDefinition(D, D2))<br>
>        return std::move(Err); <br>
> <br>
> <br>
> <br>
> —— lldb session illustrating the unnecessary imports —-<br>
> This shows that evaluation of “c2.x” after evaluation “c1.x” and “c0.x” calls to LayoutRecordType for C2, C1 and C0.<br>
> $ lldb a.out<br>
> (lldb) b h.cc:10<br>
> Breakpoint 1: where = a.out`main + 44 at h.cc:10:3, address = ...<br>
> (lldb) r<br>
> ... Process stopped ...<br>
> (lldb) log enable lldb expr<br>
> (lldb) p c2.x<br>
> ...<br>
> LayoutRecordType[6] ... for (RecordDecl*)0x... [name = 'C2']<br>
> ...<br>
> (lldb) p c1.x<br>
> ...<br>
> LayoutRecordType[7] ... for (RecordDecl*)0x... [name = 'C1']<br>
> ...<br>
> (lldb) p c0.x<br>
> ...<br>
> LayoutRecordType[8] ... for (RecordDecl*)0x... [name = 'C0']<br>
> ...<br>
> (lldb) p c2.x<br>
> ...<br>
> LayoutRecordType[9] ... for (RecordDecl*)0x... [name = 'C2']<br>
> LayoutRecordType[10] ... for (RecordDecl*)0x... [name = 'C1']<br>
> LayoutRecordType[11] ... for (RecordDecl*)0x... [name = 'C0']<br>
> ...<br>
> <br>
> _______________________________________________<br>
> lldb-dev mailing list<br>
> <a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev</a><br>
<br>
_______________________________________________<br>
lldb-dev mailing list<br>
<a href="mailto:lldb-dev@lists.llvm.org" target="_blank">lldb-dev@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev</a></blockquote></div></blockquote></div></div></blockquote></div></div>
</blockquote></div>