[WIP][lldb] Use forward decls with redeclared definitions instead of minimal import for records

[Michael137]

This begins our upstreaming efforts for the changes described in following RFC: https://discourse.llvm.org/t/rfc-lldb-more-reliable-completion-of-record-types/77442

This is a combined patch with all the changes, but the actual implementation that's of interest for reviewers is in the last commit 2d90c9f of this patch (the rest is "mostly" NFC). The main caveat currently is that the more aggressive completion of definitions causes the expression evaluator performance to noticeably regress (mainly because we're doing many more DWARF searches than we used to). We're currently in the process of looking for ways to improve this.

Motivation
The current approach for 'completing types' in LLDB is brittle because we're constructing clang Decls and filling out their definitions in ways in which they weren't designed for. This frequently manifests in crashes where Clang expected a DefinitionData to exist on a Decl and thus unconditionally dereferences it, but LLDB didn't fill it out (i.e., complete it) in time. The purpose of this patch is to ensure we allocate a definition for each decl that LLDB creates by the time that Clang needs it.

Details
At the moment we create record types ([CXX]RecordDecls and ObjCInterfaceDecls) and we always pretend that a type potentially has a definition that eventually would be lazily pulled in. However, Clang doesn't have any interface (at least none that is consistently called) that turns a RecordDecl without DefinitionData into a RecordDecl with DefinitionData. The only relevant API in the ExternalASTSource is CompleteType, which suffers from the fact that it's essentially not used by generic Clang code.

The part of the ExternalASTSource API that is consistently called to pull in a potential definition is CompleteRedeclChain (which unconditionally gets invoked when calling getDefinition on a RecordDecl).

The approach is as follows:

• Instead of creating partially defined records that have fields but possibly no definition, we only create forward decls when parsing DWARF into ASTs.
• We then bump the CurrentGeneration counter of the current ASTs ExternalASTSource to force rebuilding of the redeclaration chain.
• When Clang asks for the definition of the record (e.g., via getDefinition()), it will ask the ExternalASTSource to complete the redeclaration chain. At this point we build the definition and add it as a second decl.

This means the ASTImporter can now also stop using the Minimal mode for records as there is no reason anymore. It just imports first the forward declaration and then the definition when needed.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[github-actions]

⚠️ Python code formatter, darker found issues in your code. ⚠️

You can test this locally with the following command:

darker --check --diff -r 6258b5f610d51d37a79456d660b12c2d8e98500b...caacb57a46f34bf663fa5ab2190b361ce29b255b lldb/test/API/lang/cpp/reference-to-outer-type/TestCppReferenceToOuterClass.py

View the diff from darker here.

--- TestCppReferenceToOuterClass.py	2024-06-24 18:15:54.000000 +0000
+++ TestCppReferenceToOuterClass.py	2024-06-24 18:35:42.173115 +0000
@@ -4,11 +4,13 @@
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 
 class TestCase(TestBase):
-    @expectedFailureAll(setting=('plugin.typesystem.clang.experimental-redecl-completion', 'false'))
+    @expectedFailureAll(
+        setting=("plugin.typesystem.clang.experimental-redecl-completion", "false")
+    )
     def test(self):
         self.build()
         self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
         test_var = self.expect_expr("test_var", result_type="In")
         nested_member = test_var.GetChildMemberWithName("NestedClassMember")

[labath]

Thank you for working on this. I'm very interested in the results of this effort, as it appears I may end up dabbling into these parts of lldb in the near future. For now just a couple of quick questions (with hopefully not too long answers).

• 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?
• 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?
• 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)?

[Michael137]

• 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.

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

• 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

[labath]

• 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 (CXXMethodDecls) 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.

[Michael137]

"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).

Sorry I was a bit too loose in my terminology. You're correct, LLDB currently adds all methods to the containing class, and it's pretty cheap, because we don't fully complete arguments/return types. With this patch it becomes expensive when the ASTImporter gets involved. The core idea here is that we no longer do minimal import for records. So when we get to importing a class, the ASTImporter imports all the methods that LLDB attached, which in turn import all the parameters and return types, fully. That's where type explosion comes in.

completing all argument/result types seems rather unnecessary (I don't think C++ requires that for anything) and very expensive.

True, and AFAICT the ASTImporter has no notion of that atm (I'd need to confirm this again), so I played around with the idea of not adding all methods to the containing class, and instead add it when the Clang parser asks us (which is tricky to do well because we usually don't know that we're parsing a method call). Another idea was to add more control over which types the ASTImporter imports fully, though I never got a great implementation going for that.

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)

It isn't too bad, but having two implementations of this mechanism seemed like a maintenance burden (e.g., runs the risk of not being removed in the long run, need to run the test-suite with and without the setting, etc.). If the community is open to it, I don't mind. Incremental bugfixing/improvements is a big plus. I guess it might be easier to decide on this once we ironed out the remaining TODOs. For reference, here's what it looks like with the setting: swiftlang#8222 (approximately 50 references to UseRedeclCompletion() when I check my local branch)

[labath]

"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).

Sorry I was a bit too loose in my terminology. You're correct, LLDB currently adds all methods to the containing class, and it's pretty cheap, because we don't fully complete arguments/return types. With this patch it becomes expensive when the ASTImporter gets involved. The core idea here is that we no longer do minimal import for records. So when we get to importing a class, the ASTImporter imports all the methods that LLDB attached, which in turn import all the parameters and return types, fully. That's where type explosion comes in.

I see. Yes, that sounds like a pretty big problem.

completing all argument/result types seems rather unnecessary (I don't think C++ requires that for anything) and very expensive.

True, and AFAICT the ASTImporter has no notion of that atm (I'd need to confirm this again), so I played around with the idea of not adding all methods to the containing class, and instead add it when the Clang parser asks us (which is tricky to do well because we usually don't know that we're parsing a method call). Another idea was to add more control over which types the ASTImporter imports fully, though I never got a great implementation going for that.

Unfortunately, I don't know much about this, but I think that something like this will be necessary, as importing ~everything recursively does not seem feasible.

A possibly silly question, but my understanding was that this effort will better align us with the interfaces that are used within clang (for compiling modules I think). Is that correct? And if so, does it mean that clang does this explosive import whenever it imports a type from a (precompiled) module?

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)

It isn't too bad, but having two implementations of this mechanism seemed like a maintenance burden (e.g., runs the risk of not being removed in the long run, need to run the test-suite with and without the setting, etc.).

Yes, that's a risk I am acutely aware of, and I don't really have satisfying answer to that. FWIW, we do have some history of "successfully" removing parts of lldb that were a maintenance burden (from lldb-mi to Go support), so I think we could pull that off if there is someone who is sufficiently motivated (impacted) by this.

If the community is open to it, I don't mind. Incremental bugfixing/improvements is a big plus. I guess it might be easier to decide on this once we ironed out the remaining TODOs.

Yeah, let's do that.

[dwblaikie]

Yeah, putting this behind a very-explicitly-temporary flag seems reasonable to me, FWIW.

& yeah, the analogy to modules re: member function expansion seems relevant as far as I understand things too.

[Michael137]

A possibly silly question, but my understanding was that this effort will better align us with the interfaces that are used within clang (for compiling modules I think). Is that correct?

Regarding Clang interfaces, one of the main benefits of this patch is that we can simplify LLDB's interaction with the ASTImporter and also align us with the more tested/robust way of completing types in Clang. E.g., calls to CompleteType such as the following might no longer be necessary (at least for LLDB, not sure if other external sources started relying on this):

llvm-project/clang/lib/Sema/SemaType.cpp

Lines 9078 to 9081 in ad702e0

	// Give the external source a chance to provide a definition of the type.
	// This is kept separate from completing the redeclaration chain so that
	// external sources such as LLDB can avoid synthesizing a type definition
	// unless it's actually needed.

The ASTReader external source implements the completion mechanism via CompleteRedeclChain/ExternalASTSource::IncrementGeneration, and this patch makes LLDB use the same set of interfaces. But I'm not familiar enough with the modules infrastructure to confidently say whether there'll be ways to re-use bits of the implementation in LLDB (I suspect not, but definitely worth exploring).

And if so, does it mean that clang does this explosive import whenever it imports a type from a (precompiled) module?

Good question, I haven't dug into the modules import mechanism yet, but if this is true, then that could give us an opportunity to introduce a "lazy method loading" mechanism on the ExternalASTSource that would fit both LLDB and modules? Will try to confirm this (unless someone else knows the answer to this already, e.g., @dwblaikie).

[adrian-prantl]

Very excited to see this happening!

[adrian-prantl]

while we're here: for (auto source : Sources)?

[adrian-prantl]

Maybe add a doxygen comment for this (or an override keyword)?

[dwblaikie]

Good question, I haven't dug into the modules import mechanism yet, but if this is true, then that could give us an opportunity to introduce a "lazy method loading" mechanism on the ExternalASTSource that would fit both LLDB and modules? Will try to confirm this (unless someone else knows the answer to this already, e.g., @dwblaikie).

Yeah, I don't have any great info here - only the vague assumption/understanding that this path was more modules-aligned, and so perf improvements for lldb could also be perf improvements (& test coverage) for modules... - but I could be wrong on that conception.