Hi Gabor,
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.
Cheers, Jaro
On Thu, Nov 7, 2019 at 6:14 PM Gábor Márton <martongab...@gmail.com> wrote:
> 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
>>
>
--
Jaroslav Sevcik | Software Engineer | ja...@google.com |
Google Germany GmbH
Erika-Mann-Str. 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado | Registergericht und
-nummer: Hamburg, HRB 86891 | Sitz der Gesellschaft: Hamburg
Diese E-Mail ist vertraulich. Wenn Sie nicht der richtige Adressat sind,
leiten Sie diese bitte nicht weiter, informieren Sie den Absender und
löschen Sie die E-Mail und alle Anhänge. Vielen Dank.
This e-mail is confidential. If you are not the right addressee please do
not forward it, please inform the sender, and please erase this e-mail
including any attachments. Thanks.
_______________________________________________
lldb-dev mailing list
lldb-dev@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
