Re: [lldb-dev] Setting breakpoint on file and function name doesn't work as expected.

[Konrad Kleine via lldb-dev]

Jim,

thank you for the explanation. I'm trying to see the situation more from an
end user's perspective. When --file or -f have two different meanings
depending on how they are combined, that's bad IMHO.

>From what I read in your response I get the feeling that you assume a user
knows about the difference between CU and his or her source file and the
implications it can have when for example LTO is enabled and we make heavy
use of inlining. I see this as a problem because source-level debugging for
a function name and a file to an end user means exactly that, nomatter
where the function is inlined. Do you agree?

Konrad
___
lldb-dev mailing list
lldb-dev@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

Re: [lldb-dev] Setting breakpoint on file and function name doesn't work as expected.

[Jim Ingham via lldb-dev]

> On Nov 8, 2019, at 1:53 AM, Konrad Kleine  wrote:
> 
> Jim,
> 
> thank you for the explanation. I'm trying to see the situation more from an 
> end user's perspective. When --file or -f have two different meanings 
> depending on how they are combined, that's bad IMHO.

I don't think that it is bad that the file parameter in a "file and line" 
breakpoint and the file parameter in a function name breakpoint have different 
meanings.  That might very well make sense when you think about the kind of 
search the breakpoint is likely to do.  But this does raise a problem with the 
documentation. 

 One way to do it is to try to list all the meanings for each option ("when 
used in conjunction with...")  I don't think there are actually enough variants 
that this will bloat the documentation over much, but that's something to watch 
out for.

Another thing I've thought about doing is adding the ability to have help 
include one of the non-optional, non-overlapping options to the command, so you 
could say:

(lldb) help break set -n

and that would tell you that this is a "by function name" breakpoint, and in 
that case -n means...  That might help reduce the information overload, and 
give a better sense of what these complex commands do. 

As I said, it would have been better from a documentation standpoint to make 
all these different breakpoint commands sub-commands of "break set"("break set 
function", "break set file-and-line', etc...) but I think people would find 
that too verbose.

> 
> From what I read in your response I get the feeling that you assume a user 
> knows about the difference between CU and his or her source file and the 
> implications it can have when for example LTO is enabled and we make heavy 
> use of inlining. I see this as a problem because source-level debugging for a 
> function name and a file to an end user means exactly that, nomatter where 
> the function is inlined. Do you agree?
 
I am not sure what you are asking me to agree to.  

(lldb) break set -n foo -f bar.*

means "set the breakpoint on functions named foo DEFINED in the file "bar.*".  

It could mean other things in the context of inlining, for instance you might 
want to tell lldb to break on the function "foo" whenever it is inlined INTO 
the CU bar.*.  That's also a perfectly valid thing to do, and you might think 
"-n -f" was the combination to do that, but it is not what it does.  Again, the 
feature was intended to disambiguate between different functions with the same 
name by definition site which the current definition does.  So in this sense 
the user will have to know what the -f means (and we do need some good solution 
for documenting this more clearly.)

Back to your original query...  If the function is defined in a .h file, or 
gets inlined by LTO, this filtering is trickier, and I didn't implement that 
behavior when I implemented this breakpoint type.  So in that case, and in the 
case where LTO inlines a function, the feature isn't implemented correctly.  
The -n searche always looks for out of line and inline instances when doing the 
search.  So we already get the searcher to all the instances.   You would just 
have to widen the search beyond "Does the CU match" to try to figure out where 
the inlined instance was defined.

Jim




> 
> Konrad

___
lldb-dev mailing list
lldb-dev@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev

Re: [lldb-dev] Slow expression evaluation (ASTImporter is too eager?)

[Jaroslav Sevcik via lldb-dev]

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  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->getTemplateSpecializati