Hi Jaroslav,
Thanks for working on this. Still, there are things that are unclear for me.
I see that `LayoutRecordType` is called superfluously during the second
evaluation of `c2.x`, ok.
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.
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.
Gabor
On Wed, Nov 6, 2019 at 11:21 PM Raphael “Teemperor” Isemann via lldb-dev <
lldb-dev@lists.llvm.org> wrote:
> 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 :)
>
> (And +Gabor just in case)
>
> > On Nov 6, 2019, at 10:25 PM, Jaroslav Sevcik via lldb-dev <
> lldb-dev@lists.llvm.org> wrote:
> >
> > Hello,
> >
> > 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.
> >
> > struct C0 { int x = 0; };
> > struct C1 { int x = 1; C0* c0 = 0; };
> > struct C2 { int x = 2; C1* c1 = 0; };
> >
> > int main() {
> > C0 c0;
> > C1 c1;
> > C2 c2;
> >
> > return 0; // break here
> > }
> >
> > 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.
> >
> > I believe the culprit here is the following code at the end of the
> ASTNodeImporter::VisitRecordDecl method:
> >
> > if (D->isCompleteDefinition())
> > if (Error Err = ImportDefinition(D, D2, IDK_Default))
> > return std::move(Err);
> >
> > 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%.
> >
> > 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?
> >
> > Cheers, Jaro
> >
> > diff --git a/clang/lib/AST/ASTImporter.cpp
> b/clang/lib/AST/ASTImporter.cpp
> > index 54acca7dc62..ebbce5c66c7 100644
> > --- a/clang/lib/AST/ASTImporter.cpp
> > +++ b/clang/lib/AST/ASTImporter.cpp
> > @@ -2799,7 +2799,7 @@ ExpectedDecl
> ASTNodeImporter::VisitRecordDecl(RecordDecl *D) {
> > if (D->isAnonymousStructOrUnion())
> > D2->setAnonymousStructOrUnion(true);
> >
> > - if (D->isCompleteDefinition())
> > + if (!Importer.isMinimalImport() && D->isCompleteDefinition())
> > if (Error Err = ImportDefinition(D, D2, IDK_Default))
> > return std::move(Err);
> >
> > @@ -3438,6 +3438,9 @@ ExpectedDecl
> ASTNodeImporter::VisitFieldDecl(FieldDecl *D) {
> > if (ToInitializer)
> > ToField->setInClassInitializer(ToInitializer);
> > ToField->setImplicit(D->isImplicit());
> > + if (CXXRecordDecl *FieldType = D->getType()->getAsCXXRecordDecl())
> > + if (Error Err = ImportDefinitionIfNeeded(FieldType))
> > + return std::move(Err);
> > LexicalDC->addDeclInternal(ToField);
> > return ToField;
> > }
> > @@ -5307,7 +5310,7 @@ ExpectedDecl
> ASTNodeImporter::VisitClassTemplateSpecializationDecl(
> >
> > D2->setTemplateSpecializationKind(D->getTemplateSpecializationKind());
> >
> > - if (D->isCompleteDefinition())
> > + if (!Importer.isMinimalImport() && D->isCompleteDefinition())
> > if (Error Err = ImportDefinition(D, D2))
> > return std::move(Err);
> >
> >
> >
> > —— lldb session illustrating the unnecessary imports —-
> > This shows that evaluation of “c2.x” after evaluation “c1.x” and “c0.x”
> calls to LayoutRecordType for C2, C1 and C0.
> > $ lldb a.out
> > (lldb) b h.cc:10
> > Breakpoint 1: where = a.out`main + 44 at h.cc:10:3, address = ...
> > (lldb) r
> > ... Process stopped ...
> > (lldb) log enable lldb expr
> > (lldb) p c2.x
> > ...
> > LayoutRecordType[6] ... for (RecordDecl*)0x... [name = 'C2']
> > ...
> > (lldb) p c1.x
> > ...
> > LayoutRecordType[7] ... for (RecordDecl*)0x... [name = 'C1']
> > ...
> > (lldb) p c0.x
> > ...
> > LayoutRecordType[8] ... for (RecordDecl*)0x... [name = 'C0']
> > ...
> > (lldb) p c2.x
> > ...
> > LayoutRecordType[9] ... for (RecordDecl*)0x... [name = 'C2']
> > LayoutRecordType[10] ... for (RecordDecl*)0x... [name = 'C1']
> > LayoutRecordType[11] ... for (RecordDecl*)0x... [name = 'C0']
> > ...
> >
> > _______________________________________________
> > lldb-dev mailing list
> > lldb-dev@lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>
> _______________________________________________
> lldb-dev mailing list
> lldb-dev@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
>
_______________________________________________
lldb-dev mailing list
lldb-dev@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
