[Lldb-commits] [lldb] [WIP][lldb] Use forward decls with redeclared definitions instead of minimal import for records (PR #95100)

[Pavel Labath via lldb-commits]

labath wrote:

> > * when you say "slower", what exactly does that mean. How much slow down are we talking about?
> > * the "increased number of DWARF searches", is that due to clang asking for definitions of types more eagerly? If yes, do you have some examples of where are these extra definitions coming from?
> 
> For example, attaching LLDB to Clang and evaluating a method of the `ASTContext` class would take ~35s instead of the baseline ~6s. One of the main contributors to this is that completing a definition for a class would try to resolve all fields _and_ member functions of that class (see `DWARFASTParserClang::CompleteRecordType` where it resolves the member function DIEs). This in turn completes the parameters, etc. So we end up trying to pull in many more definitions than needed. The tricky part here is that anything that calls `getDefinition` (which we do in the `ASTImporter` for example) could trigger completion. So we can have situations where the user requests a `GetForwardCompilerType` but that ends up trying to `CompleteRedeclChain`. The current patch breaks the notion of `{Forward/Layout/Full}CompilerType`, but haven't found a good way of fixing that yet.
> 
> Another source of overhead are cases where we try to find the definition DIE using a commonly used name such as `__base`, which we find many matches for in the index, but we fail to find a correct match. Such false positive lookups are quite expensive. I forget the details at the moment but IIRC this patch just makes those lookups more frequent. And the actual failure to choose the correct name for lookup is a consequence of how templates are represented in the LLDB AST.

Ack. Thanks for explaining.

> 
> If we restrict LLDB to only complete fields (but complete methods only as needed), we get much closer to the baseline. Though "lazily" completing methods has other consequences (some features in LLDB rely on all methods being available, e.g., auto-complete, etc.). Apart from that, there still seem to be some redundant global lookups for namespaces and functions that add to the overhead. Once we get closer to the baseline performance I can provide some more accurate benchmark results. Also, happy to hear other suggestions/concerns regarding this.

I'm not sure by what you meant by "completing methods" in this paragraph. Does that mean "adding the methods (`CXXMethodDecl`s) to the containing class (`CXXRecordDecl`)", "completing (getting the definition) of the method's arguments and/or return types", or something else? My understanding is that we're currently always adding the declaration of the methods when completing the class, but that we don't eagerly complete the types arguments or results of the methods (except for covariant methods). I can see how some sort of lazy method addition could break tab completion, but I think that'd be pretty hard to implement (this is actually the idea I was looking into, but I sort of gave up on it because it didn't seem to bring much benefit). OTOH, completing all argument/result types seems rather unnecessary (I don't think C++ requires that for anything) and very expensive.

> 
> > * I see one test which tries to skip itself conditionally based on a setting enabling this, but I don't see the code for the handling the setting itself. Is the intention to add the setting, or will this be a flag day change?
> 
> Good catch! I cherry-picked these commits from the `apple/llvm-project` fork where we put these changes behind a setting. But upstream I would like to avoid doing so. I'll remove the decorator from the test (we can just XPASS it)

Might I ask why? Was it too unwieldy? I can certainly see how a change like this would be hard to keep under a flag, but on the other hand, a risky/wide-sweeping change like this is where a flag would be most useful (e.g., it would enable incremental implementation&bugfixes instead of revert-reapply Yoyo-ing)

> 
> > * I'm intrigued by this line "Instead of creating partially defined records that have fields but possibly no definition, ...". Until last week, I had assumed that types in lldb (and clang) can exist in only one of two states "empty/forward declared/undefined" and "fully defined". I was intrigued to find some support for loading only fields (`setHasLoadedFieldsFromExternalStorage`, et al.) in clang (but not in lldb, AIUI). Does this imply that the code for loading only the fields is broken (is the thing you're trying to remove)?
> 
> Good question, I've been having trouble getting small reproducers for this. We've had numerous workarounds around the ASTImporter since the original patch was proposed that addressed these types of issues. E.g., https://reviews.llvm.org/D71378 (which isn't quite the "definition but no fields" situation, but wouldn't be surprised if historically this could've happened in some corner cases of the import mechanism). A common issue I've seen in stacktraces in the past are situtations where we started an `ASTImporter::Import`, and within that called `LoadFieldsFromExternalStorage`, which would then eventually recursively call back into `ASTImporter::Import`, messing up the various bookkeeping structures. I'll try to dig up an example of this

Oh yes, I remember that one. You don't have to go out of your way to find these, but I would like to hear about them if you run into any. 

https://github.com/llvm/llvm-project/pull/95100