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
